[PATCH] D79329: [MLIR] Update the FunctionAndBlockSignatureConverter and NonVoidToVoidReturnOpConverter of Buffer Assignment

[Alexander Belyaev via Phabricator via llvm-commits]

pifon2a added a comment.

nice! thanks!



================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:84
+/// this purpose. All the non-shaped type of the input function will be
+/// converted to memref.
 class FunctionAndBlockSignatureConverter
----------------
```
/// Converts the signature of the function using the type converter.
/// It adds an extra argument for each illegally-typed function
/// result to the function arguments. `BufferAssignmentTypeConverter`
/// is a helper `TypeConverter for this purpose. All the non-shaped types
/// of the input function will be converted to memref.
```


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:99
+/// the buffer operands from the operands list, and inserts CopyOps for all
+/// buffer operands instead.
 template <typename ReturnOpSourceTy, typename ReturnOpTargetTy,
----------------
```
/// Converts the source `ReturnOp` to target `ReturnOp`, removes all
/// the buffer operands from the operands list, and inserts `CopyOp`s
/// for all buffer operands instead.
```


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:126
+      if (!operand.getType().isa<MemRefType>())
+        newOperands.push_back(operand);
+
----------------
these 5 lines are just `llvm::erase_if`, right?


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:130
+    // inserted that copies the buffer contents from the operand to the target
+    // buffer.
+    unsigned numBufferOperands = operands.size() - newOperands.size();
----------------
```
// For each memref type operand of the source `ReturnOp`, a new `CopyOp` is
// inserted that copies the buffer content from the operand to the target.
```


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:136
+        // Insert the copy operation before the target Return operation.
+        rewriter.setInsertionPoint(returnOp);
+        rewriter.create<CopyOpTy>(loc, operand,
----------------
can this be moved outside of the loop?


================
Comment at: mlir/test/Transforms/buffer-placement-prepration.mlir:15
+                                        %arg2: tensor<5x5xf64>,
+                                        %arg3: f16) -> (i1, tensor<5x5xf64>, f16, tensor<4x8xf32>) {
+    return %arg1, %arg2, %arg3, %arg0 : i1, tensor<5x5xf64>, f16, tensor<4x8xf32>
----------------
please, reformat it so that it fits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79329