[PATCH] D78484: Providing buffer assignment for MLIR

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

Thanks! Left some stylistic comments for now, will review in more detail later.



================
Comment at: mlir/include/mlir/Transforms/BufferAssignment.h:74
+
+// Converts only the tensor-type function and block arguments to memref-type.
+class FunctionAndBlockSignatureConverter
----------------
Please use /// for top-level comments.


================
Comment at: mlir/include/mlir/Transforms/BufferAssignment.h:91
+
+// This pattern converter transforms a non-void ReturnOpSourceTy into a void
+// return of type ReturnOpTargetTy. It uses a copy operation of type CopyOpTy to
----------------
Same here.


================
Comment at: mlir/include/mlir/Transforms/BufferAssignment.h:102
+
+  // Performs the actual return-op conversion step.
+  LogicalResult
----------------
Same here.


================
Comment at: mlir/include/mlir/Transforms/BufferAssignment.h:107
+    auto numReturnValues = returnOp.getNumOperands();
+    auto funcOp = returnOp.template getParentOfType<FuncOp>();
+    auto numFuncArgs = funcOp.getNumArguments();
----------------
Do we need to hard-code FuncOp here? Could we instead just check the number of arguments in the entry block of the function? Or is there some better way to make this more general?


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:41
+//
+// TODO(dfki):
+// The current implementation does not support loops. The only thing that
----------------
Please drop the `(username)` from the TODO.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:105
+        // corresponding block arguments values.
+        for (auto pred : block.getPredecessors()) {
+          // Determine the current successor index of the current predecessor.
----------------
Use the predecessor iterators instead, so that you get the successor index for free:

```
for (auto it = block.pred_begin(), e = block.pred_end(); it !=e; ++it) {
  unsigned successorIndex = it.getSuccessorIndex();
}
```


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:113
+          // Get the terminator and the values that will be passed to our block.
+          if (auto branchInterface =
+                  dyn_cast<BranchOpInterface>(pred->getTerminator())) {
----------------
Please prefer early exit when possible, e.g.:

```
auto branchInterface = ...;
if (!branchInterface)
  continue;
```


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:197
+    auto possibleValues = aliases.resolve(alloc);
+    for (auto alias : possibleValues)
+      for (auto user : alias.getUsers()) {
----------------
nit: auto -> Value

Please use the full type if possible.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:198
+    for (auto alias : possibleValues)
+      for (auto user : alias.getUsers()) {
+        if (isa<DeallocOp>(user))
----------------
nit: auto -> Operation *


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:330
+/// the right positions. It uses the algorithm described at the top of the file.
+// TODO(dfki): create a templated version that allows to match dialect-specific
+// alloc/dealloc nodes and to insert dialect-specific dealloc node.
----------------
nit: Please remove the `(username)`


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:330
+/// the right positions. It uses the algorithm described at the top of the file.
+// TODO(dfki): create a templated version that allows to match dialect-specific
+// alloc/dealloc nodes and to insert dialect-specific dealloc node.
----------------
rriddle wrote:
> nit: Please remove the `(username)`
Generally using traits and interfaces are how you should generalize a pass, instead of templating.


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:384
+BufferAssignmentPlacer::computeAllocPosition(Value value) {
+  Operation *insertOp = value.getDefiningOp();
+  assert(insertOp && "There is not a defining operation for the input value");
----------------
Please just construct an InsertionPoint directly if you need one. 


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:446
+    bool legality = true;
+    for (auto &block2 : funcOp.getBlocks()) {
+      legality &= llvm::all_of(block2.getArguments(), isLegalBlockArg);
----------------
Why block2? Can we just use block?


================
Comment at: mlir/lib/Transforms/BufferAssignment.cpp:460
+namespace mlir {
+void registerBufferAssignmentPass() {
+  PassRegistration<BufferAssignmentPass>(
----------------
PassRegistration should use the declarative tablegen backend, i.e. you should add your pass here:

https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Transforms/Passes.td


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78484