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

[Ehsan via Phabricator via llvm-commits]

dfki-ehna marked 11 inline comments as done.
dfki-ehna added inline comments.


================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:396
+        continue;
       if (deallocs.size()) {
         (*deallocs.begin())->moveBefore(nextOp);
----------------
herhut wrote:
> 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?
Asserts now if there is any `DeallocOp`. The other possibility is erasing that `DeallocOp` instead of asserting it. If a `DeallocOp` exists before the last user (`ReturnOp`), then it is already in the input IR and it has not been created by the Buffer Placement. Although, In this case, the rewriter also produces an error that the value is not dominated.


================
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())
----------------
herhut wrote:
> `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.
The Assert message was changed.


================
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));
----------------
herhut wrote:
> 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())`
Thanks.


================
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));
----------------
herhut wrote:
> Is the `WalkResult` needed here or would implicit conversion just work?
Unfortunately, the implicit conversion produces a compile error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80696