This is an archive of the discontinued LLVM Phabricator instance.

[llvm] [MatrixIntrinsics] Add row-major support for llvm.matrix.transpose
ClosedPublic

Authored by aartbik on May 27 2020, 4:13 PM.

Details

Summary

Only column-major was supported so far. This adds row-major support as well.
Note that we probably also want very efficient SIMD implementations for the
various target platforms.

Bug:
https://bugs.llvm.org/show_bug.cgi?id=46085

Diff Detail

Event Timeline

aartbik created this revision.May 27 2020, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 4:13 PM
fhahn added a comment.May 28 2020, 5:18 AM

Thank you very much for the patch! A few suggestions inline.

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
1300

I think it might be possible to share most of the transpose implementation for both row/column major transpose, as we are iterating over the indices of the second dimension while extracting from the vectors of the first dimension in both cases.

Instead of using ArgShape.NumRows, we could just use the second dimension (InputMatrix.isColumnMajor() : ArgShape.NumRows : ArgShape.NumColumns and instead of iterating over columns/rows we could just iterate over the vectors of the leading dimension (InputMatrix.vectors()) and instead of ArgShape.NumColumns/NumRows, we could use ArgShape.getNumVectors().

llvm/test/Transforms/LowerMatrixIntrinsics/pr46085-row.ll
1 ↗(On Diff #266691)

for multiply, we just have row/column major test variants in multiply-i32-row-major.ll/multiply-i32.ll and so on. For consistency, it might be slightly preferable to extend the existing transpose-*.ll tests (if the cases in the new test are not covered) and call the row-major equivalent transpose-*-row-major.ll.

aartbik updated this revision to Diff 266960.May 28 2020, 11:15 AM
aartbik marked 4 inline comments as done.

addressed comments, thanks!

aartbik added inline comments.May 28 2020, 11:24 AM
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
1300

That is a good idea. Fits nicely with the provided utilities. Thanks!

llvm/test/Transforms/LowerMatrixIntrinsics/pr46085-row.ll
1 ↗(On Diff #266691)

Sounds good. I removed the pr* tests and used the existing transpose tests for RM.

fhahn accepted this revision.May 28 2020, 11:36 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 28 2020, 11:36 AM
This revision was automatically updated to reflect the committed changes.