[PATCH] D79850: [mlir] Extended BufferPlacement to support more sophisticated scenarios in which allocations cannot be moved freely and can remain in divergent control flow.

[Marcel Koester via Phabricator via llvm-commits]

dfki-mako marked 6 inline comments as done.
dfki-mako added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:203
+      // (possibly existing) deallocation ops.
+      assert(allocateResultEffects.size() == 1 &&
+             "multiple allocations are not allowed");
----------------
mehdi_amini wrote:
> dfki-mako wrote:
> > mehdi_amini wrote:
> > > dfki-mako wrote:
> > > > herhut wrote:
> > > > > This should not be an assert. Instead either ignore the `alloc` (which AFAIK was the mode before) or fail the transformation and report an error. 
> > > > > 
> > > > > Generally, the reason this was not supported before was that multiple results creates situations where the alloc cannot be moved. Can the new system not tolerate this with copies?
> > > > > 
> > > > > I am OK with this not being supported, as a multi-result `alloc` seems a very boutique use.
> > > > In principal yes; however, we currently remove all `Dealloc` nodes in the beginning of the transformation. If we keep them for unsupported `Alloc` ops, this should not be an issue.
> > > Can we be *safe* in this case instead of failing entirely?
> > @mehdi_amini If we do not remove `Dealloc` nodes (or nodes with `free` semantics) that are related to unsupported `alloc` nodes, we can be sure that the program will not be "worse" than before with respect to these buffers.
> Right, so we shouldn't need to emit an error here then? We can just skip here?
The latest version skips these allocation nodes without aborting the transformation process.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:281
+        auto blockArg = value.cast<BlockArgument>();
+        if (blockArgsToFree.count(blockArg) < 1 &&
+            !dominators.dominates(definingBlock, blockArg.getOwner())) {
----------------
herhut wrote:
> Aren't there two cases here? If this immediate alias is not dominated, then a copy is inserted, the alias turns effectively into an allocation and also needs to be processed. If we do not insert a copy, then the alias still has to be processed, using the original allocation as the block that needs to dominate.
> 
> I think the second case is missing here.
We have added a separate test case to simulate such a case and changed the CL accordingly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79850/new/

https://reviews.llvm.org/D79850