This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] Fold the transpose into the matmul operand used to fetch scalars
ClosedPublic

Authored by anemet on May 3 2021, 8:55 AM.

Details

Summary
For column-major this is:
  A * B^t
whereas for row-major:
  A^t * B

Diff Detail

Event Timeline

anemet created this revision.May 3 2021, 8:55 AM
anemet requested review of this revision.May 3 2021, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 8:55 AM
fhahn added inline comments.May 5 2021, 1:19 PM
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?

anemet updated this revision to Diff 343525.May 6 2021, 4:18 PM
anemet marked 4 inline comments as done.

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.

fhahn accepted this revision.May 7 2021, 12:44 PM

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?

This revision is now accepted and ready to land.May 7 2021, 12:44 PM
anemet added inline comments.May 17 2021, 2:01 PM
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.