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

[Stephan Herhut via Phabricator via llvm-commits]

herhut added inline comments.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:297
 
+/// Creates a clone of `loop` with `newYieldedValues` added as new
+/// initialization values and `newYieldedValues` added as new yielded values.
----------------
Nit: `newYieldedValues` -> `newIterOperands`


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2534
+  yieldOperands.append(newYieldedValues.begin(), newYieldedValues.end());
+  auto newYield = b.create<scf::YieldOp>(yield.getLoc(), yieldOperands);
+
----------------
Why not create this in the right block to start with?


================
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(
----------------
Would `bvm.map(loop.region().front().getArguments(), newLoop.region().front().getArguments().take_front(loop.region().front().getNumArguments()))` work?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2546
+  // c. remap
+  for (auto it :
+       llvm::zip(newIterOperands,
----------------
Same here?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:2551
+  b.setInsertionPointToStart(&newLoop.region().front());
+  for (auto &o : loop.region().front().getOperations()) {
+    if (&o != yield.getOperation()) {
----------------
How about using `without_terminator()`here?


================
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));
----------------
`bvm.map(o.getResults(), cloned->getResults())`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80656