This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Add canonicalization patterns for extractMap/insertMap
ClosedPublic

Authored by ThomasRaoux on Sep 30 2020, 4:07 PM.

Details

Summary

Add basic canonicalization patterns for the extractMap/insertMap to allow them to be folded into Transfer ops.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Sep 30 2020, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 4:07 PM
ThomasRaoux requested review of this revision.Sep 30 2020, 4:07 PM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2468

It may be useful to add the map attribute to the ops and already here fail if the map is not 1-D minor identity or something.
Problem is that the map is not yet designed so we are not 100% sure what form it will take.

Do what you think is best :)

2469

If you already pulled in EDSC for operator+ you might as well: vector_transfer_read(extract.getType() ...).

2491

same comment re vector_transfer_write

mlir/test/Dialect/Vector/vector-distribution.mlir
24

vector_add_read_write_at_id ?

Just trying to convey that this will only change 1 element in the vector.

mlir/test/lib/Transforms/TestVectorTransforms.cpp
132

For affine_apply ?

Yet more evidence, affine_apply is a math dialect concept and not an "affine dialect" concept.

132

s/math/arith

This revision is now accepted and ready to land.Sep 30 2020, 11:36 PM
aartbik added inline comments.Oct 1 2020, 8:38 AM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2499

just curious why you did not make this a regular folding optimization?
(which kicks in at many more places and thus does not rely on populating this rewriting pattern at specific places)

ThomasRaoux marked an inline comment as done.Oct 1 2020, 7:46 PM
ThomasRaoux added inline comments.
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2468

True, I'll wait to insert the map if it's okay. I'll introduce it in it's own patch.

2469

Changed it.

2499

Good point, I moved it to ExtractMap folder.

mlir/test/lib/Transforms/TestVectorTransforms.cpp
132

Yes..

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

(I think) Aart's point is that it could even be part of the op's folding mechanisms ?
In practice it would be a canonicalizer because there is a type change.
So it would go to vector/Ops.cpp and be accessible via getCanonicalizationPatterns.

ThomasRaoux added inline comments.Oct 2 2020, 9:21 AM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2499

I believe Aart was talking about the case where ExtractMap's source comes from an insertMap and they cancel each other. I moved it to extractMap folder. This just removes both instruction so it seems like it belongs to the folder, the type of the source of insert has to map the return of extract by construction (since the multiplicty matches)

%b = insertMap %a, %id, 32
extractMap %b, %id, 32
->
%a

I'll push it like that. I can change it post-commit if you think this is needed.

aartbik accepted this revision.Oct 2 2020, 9:33 AM

Yes, I am happy with this as folder, like you changed now. That looks much cleaner to me.

ThomasRaoux edited the summary of this revision. (Show Details)

Revert marking TransferReadOp as readmem as some unit tests are not ready and have dead transfer_read