[mlir][vector] Refine vector.transfer_read hoisting/forwarding #65770
Conversation
banach-space
requested review from
dcaballe,
c-rhodes,
matthias-springer,
MacDue and
harsh-nod
banach-space
force-pushed
the
andrzej/fix_hoisting_with_collapse
branch
from
301a920 to
a2b9f12
Compare
banach-space
changed the title
There was a problem hiding this comment.
This fixes hoisting for collapse_shape, but it is still broken for other ops that introduce aliasing (e.g., expand_shape, arith.select, ...). We should query an alias analysis in noAliasingUseInLoop instead of hard-coding ops. Still, better than before so I'd say let's land it if it's blocking.
| // vector.transfer_write %vec, %collapse_shape | ||
| // vector.transfer_write %1, %subview | ||
| // Indeed, %alloca and %collapse_shape alias and hence %2 != %1. |
There was a problem hiding this comment.
There is no %2 in this example.
There was a problem hiding this comment.
Thanks, clarified in the latest fixup.
Thanks! Agreed that a more generic would be better, but right now this is causing our convs to be broken - I really appreciate you being flexible here 🙏🏻 . |
|
Btw, can you perform the hoisting on tensors instead of memrefs? There are no aliasing issues when hoisting before bufferization. I'm trying to see if it's worth generalizing the hoisting infrastructure, making it interface-based, and moving it to |
Tl;Dr Possibly. It looks like in many cases hoisting happens before bufferization, but not always. I'm experimenting with some optimisations for Linalg Convs and use IREE to drive this. Looking at
addBufferizePasses is usually run after e.g. createHoistRedundantVectorTransfersPass. However, in the case of convolutions, OptimizeVectorTransferPass is created after bufferization and that pass also includes I've only tried few pipelines so far, so might be missing something. Also, I have a few experiments "downstream" on top of this that trigger these aliasing issue. So, on hand, having this fix in-tree helps me focus on other issues, but I might be doing something daft myself. I'll add a few TODOs before landing this. Mostly as a reminder that a more generic approach should be implemented instead. |
Make sure that when analysing a `vector.transfer_read` that's a candidate
for either hoisting or store-to-load forwarding, memref.collapse_shape
Ops are correctly included in the alias analysis. This is done by
either:
* making sure that relevant users are taken into account, or
* source Ops are correctly identified (i.e. `memref.collapse_shape`
is not really the true source).
Note that this won't fix aliasing for other cases (e.g. with
memref.expand_shape). Ideally, we should query an alias analysis in e.g.
`noAliasingUseInLoop` instead of hard-coding ops
banach-space
force-pushed
the
andrzej/fix_hoisting_with_collapse
branch
from
50629b9 to
d91aee8
Compare
…65770) Make sure that when analysing a `vector.transfer_read` that's a candidate for either hoisting or store-to-load forwarding, `memref.collapse_shape` Ops are correctly included in the alias analysis. This is done by either * making sure that relevant users are taken into account, or * source Ops are correctly identified.
Make sure that when analysing a
vector.transfer_readthat's a candidate for either hoisting or store-to-load forwarding,memref.collapse_shapeOps are correctly included in the alias analysis. This is done by either