[PATCH] D80656: [mlir][SCF] Add utility to clone an scf.ForOp while appending new yield values.
Alex Zinenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 28 11:00:31 PDT 2020
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
More information about the llvm-commits
mailing list