[PATCH] D80696: [MLIR][BufferPlacement] Support functions that return Memref typed results

[Stephan Herhut via Phabricator via llvm-commits]

herhut added a comment.

Just some nits.



================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:109
+/// Converts the source `ReturnOp` to target `ReturnOp`. The target `ReturnOp`
+/// should not contain any converted memref operands which are the ones that had
+/// been non-memref types and have been converted to memref types. If an operand
----------------
Maybe rephrase in what it does rather than what it should have done.

//Rewrites the `ReturnOp` to conform with the changed function signature. Operands that correspond to return values that have been rewritten from tensor results to memref arguments are dropped. In their place, a corresponding copy operation from the operand to the new function argument is inserted.//


================
Comment at: mlir/include/mlir/Transforms/BufferPlacement.h:145
+           "The number of operands of return operation is more than the "
+           "number of function argument.");
+    unsigned destArgNum = numFuncArgs - needCopyOperands.size();
----------------
Nit: `argument` -> `arguments`


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:396
+        continue;
       if (deallocs.size()) {
         (*deallocs.begin())->moveBefore(nextOp);
----------------
If it escapes, there should be no dealloc. Is that worth asserting? Can we have a case where such a dealloc exists? What should be done in such cases?


================
Comment at: mlir/test/Transforms/buffer-placement.mlir:471
+module {
+  func @memref_in_function_results(%arg0: memref<5xf32>, %arg1: memref<10xf32>, %arg2: memref<5xf32>) -> (memref<10xf32>, memref<15xf32>) {
+    %0 = alloc() : memref<15xf32>
----------------
This is missing the `CHECK` lines.


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:54
         auto type = result.getType().cast<ShapedType>();
-        if (!type)
-          op.emitOpError()
-              << "tensor to buffer conversion expects ranked results";
+        assert(type && "tensor to buffer conversion expects ranked results");
         if (!type.hasStaticShape())
----------------
`ShapedType` does not require rank. I do not understand this assertion. If it is not a `ShapedType` then there is simply nothing to do. As this is a test pass, I am fine with not supporting scalars, though. With a better assertion message.


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:89
+      BlockAndValueMapping mapping;
+      for (unsigned i = 0; i < oldBlock.getNumArguments(); i++)
+        mapping.map(oldBlock.getArgument(i), newBlock->getArgument(i));
----------------
Before adding the extra arguments, we know that `oldBlock` and `newBlock` have the same number of arguments here. So why not do this earlier and use the version of map with iterators instead of the loop? Something like `mapping.map(oldBlock.getArguments(), newBlock->getArguments())`


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:153
       // Applying full conversion
-      return failed(applyFullConversion(function, target, patterns, &converter))
-                 ? WalkResult::interrupt()
-                 : WalkResult::advance();
+      return WalkResult(
+          applyFullConversion(function, target, patterns, &converter));
----------------
Is the `WalkResult` needed here or would implicit conversion just work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80696