[PATCH] D80656: [mlir][SCF] Add utility to clone an scf.ForOp while appending new yield values.

[Alex Zinenko via Phabricator via llvm-commits]

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

It feels like this should live in SCF rather than LoopUtils, it's not anyhow generic.



================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2530
+  //   2. automatically apply the BlockAndValueMapping on its operand
+  auto yield = cast<scf::YieldOp>(loop.region().front().getTerminator());
+  b.setInsertionPoint(yield);
----------------
Would loop.body().getTerminator() work instead?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2541
+  // b. remap the BB args.
+  for (auto it : llvm::zip(loop.region().front().getArguments(),
+                           newLoop.region().front().getArguments().take_front(
----------------
herhut wrote:
> Would `bvm.map(loop.region().front().getArguments(), newLoop.region().front().getArguments().take_front(loop.region().front().getNumArguments()))` work?
Could we make it a bit shorter by, e.g., caching the result of loop.region().front() in a variable loopBody?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2555
+      auto *cloned = b.clone(o, bvm);
+      for (unsigned idx = 0, e = cloned->getNumResults(); idx < e; ++idx)
+        bvm.map(o.getResult(idx), cloned->getResult(idx));
----------------
herhut wrote:
> `bvm.map(o.getResults(), cloned->getResults())`?
I think this is already done inside `clone`, no need to do it manually


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2567
+
+  newYield.erase();
+  return newLoop;
----------------
This is not allowed if OpBuilder is a ConversionRewriter, use b.eraseOp instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80656