Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EwBinaryMat kernel mishandles broadcasting where lhs is broadcast (and not scalar) #924

Open
AlexRTer opened this issue Nov 29, 2024 · 1 comment
Labels
bug A mistake in the code. good first issue Non-urgent, simple task with limited scope, suitable for getting started as a contributor.

Comments

@AlexRTer
Copy link
Collaborator

The current implementation of EwBinaryMat.h for DenseMatrix assumes that the operand that is broadcast is the rhs operand. However, if both operands are matrices there is no canonicalizer or logic elsewhere to guarantee this. Thus, the following script mishandles the broadcasting and returns a wrong result:

lhsMat = [1, 2, 3, 4, 5, 6](2,); // 2x3
rhsMat = [10, 20, 30](1,);       // 1x3

print(rhsMat + lhsMat);          // 1x3 matrix instead of 2x3
print(lhsMat + rhsMat);          // correct result

Since rhs is assumed to be broadcast and the kernel only checks whether broadcasting is generally allowed, the inferred dimension for the result is wrong and the kernel broadcasts the wrong matrix (see lines 72-97).

Dimensions are not always known at compile time, so instead of/complementary to a canonicalizer pass that shifts the broadcast matrix to be the rhs operand, a new utility function to handle broadcasting in general would be very helpful and could be reused by other kernels that support similar broadcasting I believe. E.g. it could be run at the beginning of the kernel call and verify whether the dimensions are valid, then swap lhs and rhs if necessary.

@AlexRTer AlexRTer added bug A mistake in the code. good first issue Non-urgent, simple task with limited scope, suitable for getting started as a contributor. labels Nov 29, 2024
@AlexRTer
Copy link
Collaborator Author

I noticed this is the same problem as #803 but more general as the inputs can be any matrices with matching shapes for broadcasting where the lhs operand is broadcast to rhs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A mistake in the code. good first issue Non-urgent, simple task with limited scope, suitable for getting started as a contributor.
Projects
None yet
Development

No branches or pull requests

1 participant