[PATCH] D78993: [mlir] Removed tight coupling of BufferPlacement pass to Alloc and Dealloc operations by using MemoryEffectOpInterface queries.

[Alexander Belyaev via Phabricator via llvm-commits]

pifon2a added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:195
+      for (OpOperand user : alias.getUsers()) {
+        // Check for an existing memory effect interface
+        auto effectInstance =
----------------
nit: add period after "interface".


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:361
+      if (allocateEffectInstance == effects.end())
+        return WalkResult::advance();
+
----------------
herhut wrote:
> Would just `return` work?
not sure if `return` would work, but instead of 
```
  if (allocateEffectInstance == effects.end())
        return WalkResult::advance();
  auto allocResult = allocateEffectInstance->getValue().cast<OpResult>();
  placements.emplace_back(
         allocResult, analysis.computeAllocAndDeallocPositions(allocResult));
  return WalkResult::advance();
});
```
try doing
 
```
if (allocateEffectInstance != effects.end()) {
   auto allocResult = allocateEffectInstance->getValue().cast<OpResult>();
   placements.emplace_back(allocResult, 
     analysis.computeAllocAndDeallocPositions(allocResult));
}});
``` 
to get rid of `WalkResult::advance()` .




================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:394
         // If there is no dealloc node, insert one in the right place.
-        OpBuilder builder(alloc);
+        OpBuilder builder(alloc.getOwner());
         builder.setInsertionPointAfter(positions.getDeallocPosition());
----------------
I am a bit confused by these two lines:
```
  OpBuilder builder(alloc.getOwner());
  builder.setInsertionPointAfter(positions.getDeallocPosition());
```
the first line creates a builder and sets its insertion point, the second line changes insertion point again. Why not do that in "one hop"? I think i am just confused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78993