Page MenuHomePhabricator

Drop transfer_read inner most unit dimensions
ClosedPublic

Authored by asaadaldien on Oct 11 2021, 10:24 AM.

Details

Summary

Add a pattern to take a rank-reducing subview and drop inner most
contiguous unit dim.
This is useful when lowering vector to backends with 1d vector types.

Diff Detail

Event Timeline

asaadaldien created this revision.Oct 11 2021, 10:24 AM
asaadaldien requested review of this revision.Oct 11 2021, 10:24 AM
ThomasRaoux added inline comments.Oct 11 2021, 12:05 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
104

I wonder if we should have a more specific name. There is a different pattern called TransferOpReduceRank that does significantly different kind of transformation.

mlir/lib/Dialect/Vector/VectorTransforms.cpp
3542

That sounds too restrictive, we should allow minor identity. (and ideally add a test for it)

3545–3546

nit: srcType.getAffineMaps().size() <= 1?

3550

nit: use getVectorType()

3559–3564

Can we just iterate reverse through the strides and break whenever it is != 1?

3562

nit: should be break in the else case?

3582–3584

Should that be outside the loop?

asaadaldien marked 4 inline comments as done.

Comments..

mlir/lib/Dialect/Vector/VectorTransforms.cpp
3542

Don't we have a canonization that composes this map with the memref map?

3559–3564

we need to skip the first

asaadaldien added inline comments.Oct 11 2021, 12:40 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
104

about about reduceTransferOpsOperandRank ?

ThomasRaoux added inline comments.Oct 11 2021, 12:47 PM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
3542

minor identity transfer read is the canonical form as far as I know.

3559–3564

Can we just iterate reverse? just a suggestion though, feel free to ignore.
something like:

for(int64_t stride : llvm::reverse(scrStrides) {
  if(stride != 1)
    break;
   dimsToDrop++;
}
3586–3592

nit: can those two be merged? I realize this requires to change a bit how map is handled, just wondering if this makes the code a bit simpler, feel free to ignore.

asaadaldien added inline comments.Oct 11 2021, 1:02 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
104

what about reduceTransferOpsOperandRank ?

ThomasRaoux added inline comments.Oct 11 2021, 1:04 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
104

If this is meant to only drop inner unit dimension I would include it in the name. What do you think? Or is the idea to collapse all the contiguous inner dimension? I wonder if we can make the goal a bit more explicit in the name.

asaadaldien added inline comments.Oct 11 2021, 1:08 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
104

I agree, collapseInnerMostContiguousDims is the goal of these patterns. I will go with this naming. Thanks!

Use isMinorIdentitiy

ThomasRaoux added inline comments.Oct 11 2021, 1:48 PM
mlir/test/Dialect/Vector/vector-transfer-collapse-inner-most-dims.mlir
8

Can you make the dest vector<1x8x1xf32> to show how this works when vector rank < memref rank

Drop leading dim in test case

asaadaldien marked an inline comment as done.Oct 11 2021, 3:20 PM

update test pass name

This revision is now accepted and ready to land.Oct 19 2021, 5:25 PM
This revision was automatically updated to reflect the committed changes.