Page MenuHomePhabricator

[mlir][vector] Remove usage of shapecast to remove unit dim
ClosedPublic

Authored by ThomasRaoux on Nov 18 2021, 4:16 PM.

Details

Summary

Instead of using shape_cast op in the pattern removing leading unit
dimensions we use extract/broadcast ops. This is part of the effort to
restrict ShapeCastOp further in the future and only allow them to
convert to or from 1D vector.

This also adds extra canonicalization to fill the gaps in simplifying
broadcast/extract ops.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Nov 18 2021, 4:16 PM
ThomasRaoux requested review of this revision.Nov 18 2021, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2021, 4:16 PM
ThomasRaoux edited the summary of this revision. (Show Details)Nov 18 2021, 4:17 PM
nicolasvasilache accepted this revision.Nov 19 2021, 1:28 AM

Cool, thanks much!

Could you also move the patterns from mlir/lib/Dialect/Vector/VectorTransforms.cpp into an appropriately named file with a name consistent with its siblings?

mlir/lib/Dialect/Vector/VectorOps.cpp
1216

Did you want to keep SplatOp here ?
Seems like it is redundant with the pattern below ?

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

Seems a little small granularity?
Saves you len("SmallVector") + 3 chars vs inlining and loses the type info at each call sites.
You decide if it is worth it :)

This revision is now accepted and ready to land.Nov 19 2021, 1:28 AM

Could you also move the patterns from mlir/lib/Dialect/Vector/VectorTransforms.cpp into an appropriately named file with a name consistent with its siblings?

Let me send another NFC patch right after I land this one.

mlir/lib/Dialect/Vector/VectorOps.cpp
1216

I actually do need it, the pattern below is for splat constant, this is for splatOp. This is needed to fill the gap now that we use extract/broadcast instead of shape cast. Without that IREE would fail to canonicalize some cases.

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

Yeah I was thinking the same, however I went with this because it makes things slightly less verbose and make it more explicitly. This can be changed in the next refactor :)