[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:190
+    introduceCopies();
+    // Finds all associated dealloc nodes.
+    findDeallocs();
----------------
Nit: `Finds` -> `Find`?


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:279
+    auto findUnsafeValues = [&](Value source, Block *definingBlock) {
+      for (Value value : aliases.resolve(source)) {
+        auto blockArg = value.dyn_cast<BlockArgument>();
----------------
With this change, you consider all transitive aliases of a value. However, as soon as you insert a copy (by adding a blockarg to the list of blocks to free) that set would need updating, as you now have fewer transitive aliases. 

In you example, consider the case where there is another aliasing block (say b7) after b6. Then we would get a copy from b5 to b6, as b2 does not dominate b6. This would also take care of the block b7 after b6, as long as b6 dominates it. However, with the current approach, you would still check whether b2 dominates this additional block b7 and like insert a copy from b6 to b7. 

I think that, when adding a blockarg to the free list, you have to use that block as the dominating block (as it now becomes the allocating place) when processing aliases of the new freed blockarg.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:366
+            // Try to find a free effect that is applied to one of our values or
+            // an involved block argument that will be automatically freed by
+            // our pass.
----------------
I do not understand what the block argument refers to here.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:375
+          });
+      // Assign the associated delloc operation (if any).
+      if (userIt != entry.allocValue.user_end())
----------------
Nit: `delloc` -> `dealloc`


================
Comment at: mlir/test/Transforms/buffer-placement.mlir:146
+//      CHECK: cond_br
+//      CHECK: br ^bb5(%[[ALLOC1]]{{.*}})
+//      CHECK: br ^bb5(%[[ALLOC1]]{{.*}})
----------------
Could you add all block numbers here. That makes it easier to follow where the `br` operations belong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79850