This is an archive of the discontinued LLVM Phabricator instance.

Add case to handle 0-D vectors in FlattenContiguousRowMajorTransferWritePattern and FlattenContiguousRowMajorTransferReadPattern.
ClosedPublic

Authored by harsh on Feb 7 2022, 4:09 PM.

Details

Summary

For 0-D as well as 1-D vectors, both these patterns should
return a failure as there is no need to collapse the shape
of the source. Currently, only 1-D vectors were handled. This
patch handles the 0-D case as well.

Diff Detail

Event Timeline

harsh created this revision.Feb 7 2022, 4:09 PM
harsh requested review of this revision.Feb 7 2022, 4:09 PM
harsh updated this revision to Diff 406648.Feb 7 2022, 4:37 PM

Added unit test

ThomasRaoux added inline comments.
mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
376–377

Shouldn't the condition be vectorType.getRank() <= 1?

mlir/test/Dialect/Vector/vector-transfer-flatten.mlir
60

does the problem not happen if memref is a higher rank but vector is still rank 0.

Benoit accepted this revision.Feb 7 2022, 7:43 PM
This revision is now accepted and ready to land.Feb 7 2022, 7:43 PM

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

Benoit added a comment.Feb 7 2022, 7:49 PM

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

Better ask @nicolasvasilache ! In https://reviews.llvm.org/D114993 I had originally written a larger number of tests. Nicolas took over and disabled/removed tests.

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

Better ask @nicolasvasilache ! In https://reviews.llvm.org/D114993 I had originally written a larger number of tests. Nicolas took over and disabled/removed tests.

ok, what about the failure condition, why do we have to care about the memref rank?

harsh added inline comments.Feb 8 2022, 9:58 AM
mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
376–377

If the vector rank is 0 and the source rank is 1, then the baseline code (before this patch) works. This is only an issue when
the source rank is 0 and the vector rank is 0. In that case, the assertion I run into is

mlir::AffineExpr mlir::makeCanonicalStridedLayoutExpr(ArrayRef<int64_t>, ArrayRef<mlir::AffineExpr>, mlir::MLIRContext *): Assertion `!sizes.empty() && !exprs.empty() && "expected non-empty sizes and exprs"' failed.
mlir/test/Dialect/Vector/vector-transfer-flatten.mlir
60

yes, for example if the memref is 1d (memref<8xi8> vs memref<i8>), then it works even though the vector is rank 0 in both cases.

harsh added inline comments.Feb 8 2022, 10:02 AM
mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
376–377

In the case that the source rank is 0, I guess we don't need to enforce the vector rank since vector.transfer_read/write semantics enforce that the vector rank will also have to be 0D. So do you want to just check for sourceType.getRank() == 0? Or did you have another idea?

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

Better ask @nicolasvasilache ! In https://reviews.llvm.org/D114993 I had originally written a larger number of tests. Nicolas took over and disabled/removed tests.

ok, what about the failure condition, why do we have to care about the memref rank?

Good question, I think I just wrote this condition without really thinking about it.
I made the edit, rebuilt and reran, and so I can confirm, that it does not make a difference for my own usage patterns. Specifically: this diff does not matter to me:

--- a/mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
+++ b/mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
@@ -373,7 +373,7 @@ class FlattenContiguousRowMajorTransferReadPattern
     // Contiguity check is valid on tensors only.
     if (!sourceType)
       return failure();
-    if (vectorType.getRank() == 1 && sourceType.getRank() == 1)
+    if (vectorType.getRank() == 1)

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

Better ask @nicolasvasilache ! In https://reviews.llvm.org/D114993 I had originally written a larger number of tests. Nicolas took over and disabled/removed tests.

ok, what about the failure condition, why do we have to care about the memref rank?

Good question, I think I just wrote this condition without really thinking about it.
I made the edit, rebuilt and reran, and so I can confirm, that it does not make a difference for my own usage patterns. Specifically: this diff does not matter to me:

--- a/mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
+++ b/mlir/lib/Dialect/Vector/Transforms/VectorTransferOpTransforms.cpp
@@ -373,7 +373,7 @@ class FlattenContiguousRowMajorTransferReadPattern
     // Contiguity check is valid on tensors only.
     if (!sourceType)
       return failure();
-    if (vectorType.getRank() == 1 && sourceType.getRank() == 1)
+    if (vectorType.getRank() == 1)

Ok then I think we should change the condition to vectorType.getRank() <=1 for simplicity

harsh added a comment.Feb 8 2022, 10:42 AM

Sounds good. I will change the condition to vectorType.getRank() <=1

harsh updated this revision to Diff 406902.Feb 8 2022, 11:14 AM

Updated based on Thomas' comments

ThomasRaoux accepted this revision.Feb 8 2022, 11:33 AM

Thanks Harsh!

harsh added a comment.Feb 8 2022, 11:44 AM

Thank you! :)

This revision was landed with ongoing or failed builds.Feb 8 2022, 12:00 PM
This revision was automatically updated to reflect the committed changes.