For column-major this is: A * B^t whereas for row-major: A^t * B
Details
Diff Detail
Event Timeline
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
1018–1023 | Can you mentioned scalar_matrix_transposed in the comment? | |
1027 | Can you adjust the name to camel case so it matches the style elsewhere in the file (isTiled is also an odd one out) | |
1358 | I think the crashes/test failures shown for the pre-merge checks may be related to not bailing out early if !DT. | |
1363 | Those changes seem quite isolated form the other code in the function. Might be worth to have them in a separate function and call it before LowerMatrixMultiplyFused? Otherwise I think it would be good to adjust the name/comment for the function. | |
llvm/test/Transforms/LowerMatrixIntrinsics/multiply-left-transpose-row-major.ll | ||
4 | no need to use the RM prefix? |
Address Florian's comments. Also fix the testcase that I copied mine from re: --check-prefix=RM
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
1363 | I think it's good to have this central function to dispatch all fusing opportunities for multiply. Let me update the comment. |
LGTM, thanks!
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
1363 | Sounds good. I still think it might help to keep things readable if the code for this new transform would be in a separate function, called from there. Or at least Move the shape variables & co inside the if() block where they are used? |
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
1363 | Thanks! I moved most of it under the if. I expect that the interface we want for these functions would have the matmul already cracked open (i.e. A, B, R, M, C) that is why I had this outside but that's probably too early to decide at this point. |
Can you mentioned scalar_matrix_transposed in the comment?