Page MenuHomePhabricator

[mlir] Remove unsafe tensor_to_memref -> load canonicalization pattern.
AbandonedPublic

Authored by mravishankar on Mar 11 2021, 9:07 PM.

Details

Reviewers
silvas
Summary

Folding tensor_to_memref -> load after bufferization results in
bufferization being undone. It can potentially violate dependencies,
if there was a store to the memref, followed by a load. This pattern
would return the value before the store. In general this pattern needs
to be applied selectively and adding it to canonicalizers would result
in subtle correctness errors.

Diff Detail

Event Timeline

mravishankar created this revision.Mar 11 2021, 9:07 PM
mravishankar requested review of this revision.Mar 11 2021, 9:07 PM

I did something similar for tensor_load(tensor_to_memref()).
The conservative way I came up with was to only fold when loadOp == tensorToMemRefOp->getNextNode(), this should also work here.
In my case it does provide useful canonicalizations.

It is also a good place to add a clear "// TODO: relax this once we have proper alias analysis support".

Can we just do the same ehre and avoid killing the pattern altogether?
This will also make it clear to whoever looking into this next that they should not reintroduce the pattern.

mlir/test/Dialect/Standard/canonicalize.mlir
90

this particular case is actually safe :)

silvas accepted this revision.Mar 12 2021, 11:09 AM

LGTM. Retaining this in the "next node" case doesn't fix the core problem of undoing bufferization which I think was the issue you were hitting (and is my main objection to this pattern).

For context, the safety issue is technically arguable given the current upstream state (not saying this is great) -- upstream currently has a (not yet formally documented) quasi-SSA memref step during bufferization, which arises naturally from the opaque nature of tensor_to_memref (cannot assume writable) as it is inserted by the type conversion framework, which makes this pattern safe (but it still "undoes bufferization", so I still want to delete the pattern even given that assumption :) ). There has been an attempt to document the property D95522, but nothing committed.

It sounds like Nicolas is experimenting with a different approach, which doesn't have the same invariants, and I'm fine with that -- I still haven't seen a coherent solution to the inplace update problem with the upstream approach (the most is my hand waving that a sufficiently smart "regalloc" can do it), so any solution in this space is welcome, and I'm happy to supercede the current bufferization stuff by what Nicolas comes up with (though I suspect that the current upstream stuff will still be relevant to some flows, such as those that represent shapes as small tensors/memrefs -- will be "fun" to disentangle that). In chats with Nicolas he has managed to convince me that the "quasi-SSA" memref situation doesn't feel long-term, and I'm working with him to bring a new solution upstream.

This revision is now accepted and ready to land.Mar 12 2021, 11:09 AM