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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:281
+        auto blockArg = value.cast<BlockArgument>();
+        if (blockArgsToFree.count(blockArg) < 1 &&
+            !dominators.dominates(definingBlock, blockArg.getOwner())) {
----------------
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.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:291
+    for (auto &entry : blockMapping) {
+      findUnsafeValues(entry.first, entry.second);
+    }
----------------
Nit: Remove the `{` `}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79850