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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added inline comments.


================
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));
----------------
dfki-ehna wrote:
> herhut wrote:
> > Is the `WalkResult` needed here or would implicit conversion just work?
> Unfortunately, the implicit conversion produces a compile error.
Maybe making the return type of the lambda explicit would help? Like `[&](FuncOp function) -> WalkResult {...`?


================
Comment at: mlir/test/lib/Transforms/TestBufferPlacement.cpp:77
 
-      // Move regions from the old operation to the new one.
-      auto &region = linalgOp.region();
-      rewriter.inlineRegionBefore(op.region(), region, region.end());
-
-      // TODO: verify the internal memref-based linalg functionality.
-      auto &entryBlock = region.front();
-      for (auto result : results) {
-        auto type = result.getType().cast<ShapedType>();
-        entryBlock.addArgument(type.getElementType());
+      // Create a new block in the region of the new Generic Op.
+      Block &oldBlock = op.getRegion().front();
----------------
Looking at this again, would it not be easier to use `rewriter.inlineRegionBefore` to move over the entire region into the new op and then only add the new arguments to the first block? Sorry for not suggesting this earlier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80696