[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 11 inline comments as done.
dfki-mako added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:37
+// post dominator. However, this requires a new copy buffer for %arg1 that will
+// contain the actual contents. Using the class BufferPlacementAliasAnalysis, we
+// will find out that %new_value has a potential alias %arg1. In order to find
----------------
herhut wrote:
> In other words, the analysis uses the post-dominator to free the allocated memory. If no such post-dominator exists, aliases are removed by inserting allocs and copies. Is that the idea?
Yep, that's the general idea.


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


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:293
+    // Detect possibly unsafe aliases starting from all block arguments.
     for (Region &region : operation->getRegions())
       for (Block &block : region)
----------------
herhut wrote:
> This does not account for nested blocks in nested regions. Also, why is this needed?
Obsolete; we have added a new fix-point iteration.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:313
+                .slice(blockArg.getArgNumber(), 1);
+        Value sourceValue = ((OperandRange)successorOperand)[0];
+
----------------
herhut wrote:
> Somewhat sad that `MutableOperandRange` does not support to get values out...
Unfortunately, it seems to be the only way at the moment...


================
Comment at: mlir/test/Transforms/buffer-placement.mlir:87
+// CHECK-NEXT: br ^bb3
+// CHECK-NEXT: ^bb3(%[[ALLOC02:.*]]:{{.*}})
+//      CHECK: linalg.copy(%[[ALLOC02]],
----------------
herhut wrote:
> The numbering is strange of `ALLOC02`
The name `ALLOC02` should indicate that the allocations `ALLOC0` and `ALLOC2` will be freed at this location.


================
Comment at: mlir/test/Transforms/buffer-placement.mlir:211
 
 // CHECK-NEXT: %[[FIRST_ALLOC:.*]] = alloc()
 // CHECK-NEXT: linalg.generic
----------------
herhut wrote:
> I cannot follow this without the block numbers in the checks. Why do we get copies here?
The updated algorithm does not insert any copies in these cases any more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79850