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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:203
+      // (possibly existing) deallocation ops.
+      assert(allocateResultEffects.size() == 1 &&
+             "multiple allocations are not allowed");
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79850