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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:194
     for (Value alias : possibleValues)
-      for (Operation *user : alias.getUsers()) {
-        if (isa<DeallocOp>(user))
-          result.insert(user);
+      for (OpOperand user : alias.getUsers()) {
+        // Check for an existing memory effect interface
----------------
This should still be Operation* and not OpOperand?


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:200
+          continue;
+        // Check whether our value will be freed
+        SmallVector<MemoryEffects::EffectInstance, 2> effects;
----------------
Please make the comments complete sentences.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:361
+      if (allocateEffectInstance == effects.end())
+        return WalkResult::advance();
+
----------------
pifon2a wrote:
> 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()` .
> 
> 
The walk methods are allowed to have void return, so you can strip out the `WalkResult::advance`. You only need that if you want to interrupt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78993