[PATCH] D78484: Providing buffer assignment for MLIR

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:354
+                  "Not supported number of associated dealloc operations");
+        return signalPassFailure();
+      }
----------------
Can you change this walk to return WalkResult? That allows for interrupting the walk early.

You can return WalkResult::interrupt()(or failure()) to stop the walk, and WalkResult::advance()/success() to continue the walk.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:437
+  target.addDynamicallyLegalOp<FuncOp>([&](FuncOp funcOp) {
+    if (!llvm::all_of(funcOp.getBlocks().front().getArguments(),
+                      isLegalBlockArg))
----------------
An easy way of doing this is just:

```
target.addDynamicallyLegalOp<FuncOp>([&](FuncOp funcOp) {
  return typeConverter.isSignatureLegal(funcOp.getType());
});
```


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:29
+/// the BufferPlacement pass that can be applied afterwards.
+struct TestBufferPlacementPreparationPass
+    : mlir::PassWrapper<TestBufferPlacementPreparationPass, FunctionPass> {
----------------
A function pass is not allowed to mutate the public type of the function, so this should be a module pass.


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:130
+    BufferAssignmentTypeConverter converter;
+    converter.addConversion(
+        BufferAssignmentTypeConverter::ConvertRankedTensorToMemref);
----------------
Can you just do this inside of the converter constructor? Otherwise, you don't need a specific converter class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78484