[Codegen] Migrate MapStoreOp to VectorizableOpInterface#23662
Conversation
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2c
branch
5 times, most recently
from
2579a16 to
766f652
Compare
hanhanW
requested review from
Groverkss,
IanWood1,
MaheshRavishankar,
Max191,
krzysz00,
kuhar,
nirvedhmeshram and
qedawkins
as code owners
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2b
branch
from
a23ccb1 to
c1ebfc1
Compare
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2c
branch
from
766f652 to
37cef0e
Compare
Max191
left a comment
Max191
left a comment
There was a problem hiding this comment.
Nice! I'm glad we're getting rid of all the ad-hoc vectorization passes
| options.enableCleanup = false; | ||
| options.foldCastIntoContract = true; | ||
| options.enableVectorMasking = enableMasking; | ||
| options.vectorizeMapStore = true; |
There was a problem hiding this comment.
FYI, we probably don't need the pass option, but it may still be good to keep to avoid untested code paths. The main reason this wasn't enabled elsewhere is that we don't see map_store anywhere else, so it should be fine to have on in other pipelines, but just not really used/tested. We would also need to add the decomposition pass to the other pipelines for it to work e2e.
To be clear, no action item here. Just providing some context. Maybe you can add a comment in case someone wants to enable it on other pipelines in the future.
There was a problem hiding this comment.
Thanks for the context, I tried that before (including adding DecomposeMapStoreOp to CPU pipeline). The performance is the same for one of the tracking models but the binary size regresses. Given that the op is not fully developed in other backend, I end up with having the flag for now; we can do the flip when other backends catch up.
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2b
branch
from
c1ebfc1 to
e5cd0c0
Compare
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2c
branch
from
37cef0e to
5aa5b62
Compare
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2b
branch
from
e5cd0c0 to
efd99c5
Compare
The revision also deletes VectorizeIREELinalgExtOps, because it is already covered in GenericVectorization pass. The tests from `vectorize_iree_linalg_ext_ops.mlir` are migrated to `generic_vectorization.mlir`. The map_store vectorization is only handled on LLVMGPU, which requires DecomposeMapStoreOp pass, so the vectorization is only enabled on LLVMGPU for now -- which matches the current behavior in IREE. It is a step towards https://lists.lfaidata.foundation/g/iree-technical-discussion/message/15 Assisted-by: Claude ci-extra: test_torch Signed-off-by: hanhanW <hanhan0912@gmail.com>
hanhanW
force-pushed
the
users/hanhanW/vec-iface-c2c
branch
from
5aa5b62 to
a84722f
Compare
3 participants
The revision also deletes VectorizeIREELinalgExtOps, because it is already covered in GenericVectorization pass. The tests from
vectorize_iree_linalg_ext_ops.mlirare migrated togeneric_vectorization.mlir.The map_store vectorization is only handled on LLVMGPU, which requires DecomposeMapStoreOp pass, so the vectorization is only enabled on LLVMGPU for now -- which matches the current behavior in IREE.
It is a step towards https://lists.lfaidata.foundation/g/iree-technical-discussion/message/15
Assisted-by: Claude
ci-extra: test_torch