[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 4 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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79850