This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Allow unroll of vector.contract, vector.transfer_read/write in arbitrary order
ClosedPublic

Authored by christopherbate on Jun 3 2022, 2:41 PM.

Details

Summary

Adds supprot for vector unroll transformations to unroll in different
orders. For example, the vector.contract can be unrolled into a
smaller set of contractions. There is a choice of how to unroll the
decomposition based on the traversal order of (dim0, dim1, dim2).
The choice of traversal order can now be specified by a callback which
given by the caller of the transform. For now, only the
vector.contract, vector.transfer_read/transfer_write operations provide
support for the callback.

Diff Detail

Event Timeline

christopherbate created this revision.Jun 3 2022, 2:41 PM
christopherbate requested review of this revision.Jun 3 2022, 2:41 PM
christopherbate added a comment.EditedJun 3 2022, 2:46 PM

Thomas had some comments from offline discussion (paraphrase)

  • The functor utility DecomposeShapeIterator is more efficient but we could just use getVectorOffset be pre-permuting the shapes.

Personally I feel that the functor utility is more readable than the getVectorOffset which links into helper functions in a different file. Can we keep this and then slowly remove the getVectorOffset utility?

  • Add vector.transfer_read/transfer_write support in this commit as well?

Add support for changing unroll order of transfer_read/transfer_write

Update commit message.

christopherbate retitled this revision from [mlir][vector] Allow unroll of contraction in arbitrary order to [mlir][vector] Allow unroll of vector.contract, vector.transfer_read/write in arbitrary order.Jun 3 2022, 3:42 PM
christopherbate edited the summary of this revision. (Show Details)Jun 4 2022, 2:48 PM
ThomasRaoux accepted this revision.Jun 5 2022, 9:56 PM

Thanks! Looks good to me, I added few style related comments.

mlir/lib/Dialect/Vector/Transforms/VectorUnrollDistribute.cpp
92

I think having an explicit function name would make it easier to read the code than overloading operator().

mlir/test/lib/Dialect/Vector/TestVectorTransforms.cpp
522
This revision is now accepted and ready to land.Jun 5 2022, 9:56 PM
christopherbate marked 2 inline comments as done.

Address reviewer comments and cleanup dangling comments in code.

okkwon added a subscriber: okkwon.Jun 7 2022, 11:55 AM

Hi, just for heads-up, this change caused many failures in IREE tests including compilation errors and accuracy errors.

Hi, just for heads-up, this change caused many failures in IREE tests including compilation errors and accuracy errors.

Ok, thanks. Do you mean C++ compilation or mlir compilation errors?

There were build failures on some configs/compilers that I fixed with one-line change a392a39f75af [mlir][vector] fix typo in vector unroll transform.

Do we need t o revert?
Let me know if you can share the compilation/accuracy errors.

okkwon added a comment.Jun 7 2022, 1:27 PM

It would be nice if you don't mind reverting.

There are compilation errors in compiling some mlir sources. There is a crash too.

The PR for integration: https://github.com/google/iree/pull/9369
(You can find the failing tests there.)

Crash
https://source.cloud.google.com/results/invocations/727a0eb1-7000-4b8e-ab2f-36012c00f858/targets/iree%2Fgcp_ubuntu%2Fcmake%2Flinux%2Fx86-swiftshader%2Fpresubmit/log

It would be nice if you don't mind reverting.

There are compilation errors in compiling some mlir sources. There is a crash too.

The PR for integration: https://github.com/google/iree/pull/9369
(You can find the failing tests there.)

Crash
https://source.cloud.google.com/results/invocations/727a0eb1-7000-4b8e-ab2f-36012c00f858/targets/iree%2Fgcp_ubuntu%2Fcmake%2Flinux%2Fx86-swiftshader%2Fpresubmit/log

Ok yeah definitely looks like I made a mistake somewhere. I'll revert now, thanks for sharing those tests

christopherbate reopened this revision.Jun 8 2022, 11:57 AM
This revision is now accepted and ready to land.Jun 8 2022, 11:57 AM

Fix mistake in default unroll permutation for vector.contract

ThomasRaoux accepted this revision.Jun 8 2022, 3:03 PM

Thanks, LGTM