[PATCH] D78484: Providing buffer assignment for MLIR

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Overall LGTM, it'd be nice if someone else could also also approve though (@silvas ?)



================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:120
+      auto returnArgNumber = firstReturnParameter + operand.index();
+      auto dstBuffer = entryBlock.getArgument(returnArgNumber);
+      if (dstBuffer == operand.value())
----------------
Almost all the uses of auto above could benefit from using the actual types.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:368
+      }
+    });
+  };
----------------
dfki-mako wrote:
> mehdi_amini wrote:
> > Isn't this all assuming a particular model with a single allocation / deallocation per buffer?
> > What about a pattern like the following with two allocs feeding into one dealloc? (we can imagine more complex cases)
> > 
> > ```
> >     cond_br %cond, ^bb1, ^bb2
> >   ^bb1:
> >     %buffer1 = alloc ...
> >     br ^exit(%buffer1 : ....)
> >   ^bb2:
> >     %buffer2 = alloc ...
> >     br ^exit(%buffer2 : ...)
> >   ^exit(%buffer: ...):
> >     dealloc %buffer
> > ```
> This pass currently assumes a single-allocation/deallocation model that usually appears during straight-forward legalization (lowering) of operations. We wanted to keep the first version simple and would like to significantly extend the functionality in one of the follow-up CLs.
Sure, but please document this restriction in the TableGen pass description and add a TODO in the code so the reader is aware that this is a known limitation 
(in general it helps me reviewing code when the known limitations are spelled out and there is explicit acknowledgement of what will be addressed in followup revisions)

Even in single-allocation/deallocation, would the pass pessimizes case of conditional allocation? For example in the following (pseudo) code I suspect you'd increase the dynamic memory consumption by always allocating both buffers:

```
  if (cond)
   %buffer1 = alloc ...
  else
   %buffer2 = alloc ...
  ...
  if (cond) {
   consume(buffer1)
   dealloc(buffer1)
  } else {
   consume(buffer2)
   dealloc(buffer2)
  }
```




================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:252
+    auto placementBlock = findPlacementBlock(result, aliases, dominators);
+    auto livenessInfo = liveness.getLiveness(placementBlock);
+
----------------
Can you spell the types above?


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:283
+    auto placementBlock = findPlacementBlock(result, aliases, postDominators);
+    auto livenessInfo = liveness.getLiveness(placementBlock);
+
----------------
(here as well)


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:292
+    for (Value alias : aliases) {
+      auto aliasEndOperation =
+          livenessInfo->getEndOperation(alias, endOperation);
----------------
`Operation *` ?


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:128
+
+    auto function = getFunction();
+    BufferAssignmentPlacer placer(function);
----------------
FuncOp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78484