Page MenuHomePhabricator

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

Authored by nicolasvasilache on May 27 2020, 12:39 PM.

Details

Summary

This utility factors out the machinery required to add iterArgs and yield values to an scf.ForOp.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 12:39 PM

Drop spurious changes to unrelated parts of the code.

Revert spurious changes.

herhut added inline comments.May 28 2020, 2:53 AM
mlir/include/mlir/Transforms/LoopUtils.h
297

Nit: newYieldedValues -> newIterOperands

mlir/lib/Transforms/Utils/LoopUtils.cpp
2534

Why not create this in the right block to start with?

2541

Would bvm.map(loop.region().front().getArguments(), newLoop.region().front().getArguments().take_front(loop.region().front().getNumArguments())) work?

2546

Same here?

2551

How about using without_terminator()here?

2555

bvm.map(o.getResults(), cloned->getResults())?

ftynse requested changes to this revision.May 28 2020, 11:00 AM

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

mlir/lib/Transforms/Utils/LoopUtils.cpp
2530

Would loop.body().getTerminator() work instead?

2541

Could we make it a bit shorter by, e.g., caching the result of loop.region().front() in a variable loopBody?

2555

I think this is already done inside clone, no need to do it manually

2567

This is not allowed if OpBuilder is a ConversionRewriter, use b.eraseOp instead

This revision now requires changes to proceed.May 28 2020, 11:00 AM
bondhugula added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
2567

Hmm... but there is no OpBuilder::eraseOp; it only exists in PatternRewriters.

bondhugula added inline comments.May 28 2020, 12:19 PM
mlir/lib/Transforms/Utils/LoopUtils.cpp
2541–2543

Since there's only one region and one block, region().front() -> getBody() / body(). I'm surprised there isn't an accessor. For eg. spirv::LoopOp has a body().

2560

Nit: Replace loop's results if ...

nicolasvasilache marked 15 inline comments as done.May 28 2020, 4:00 PM
nicolasvasilache added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
2534

Because I would need to manually replace the values, so I just let the clone do it then erase the op.
If you have strong feelings about this I can change to the other form.

2567

added one, let's see what @rriddle says

nicolasvasilache marked 2 inline comments as done.

Address review comments.

ftynse added inline comments.May 29 2020, 1:34 AM
mlir/include/mlir/IR/Builders.h
465 ↗(On Diff #267075)

OpBuilder was refactored to use the Listener pattern instead of virtual methods. If we agree that we want to have eraseOp on OpBuilder, it should also go through the listener.

mlir/include/mlir/Transforms/LoopUtils.h
323

My commit-level comment might have gone unnoticed, so reiterating here: "It feels like this should live in SCF rather than LoopUtils, it's not anyhow generic."

mlir/lib/Transforms/Utils/LoopUtils.cpp
2567

Good point, @bondhugula !

@nicolasvasilache do you intend to use this in a place where no PatternRewriter is available? If not, just take a PatternRewriter& instead of OpBuilder& and be done with it.

Address review.

nicolasvasilache marked 5 inline comments as done.May 29 2020, 3:56 AM
nicolasvasilache added inline comments.
mlir/include/mlir/IR/Builders.h
465 ↗(On Diff #267075)

reverted this for now, it can be done separately.

mlir/lib/Transforms/Utils/LoopUtils.cpp
2567

For now I reverted to a comment, when I need it in a PatternRewriter I'll update.
Thanks!

ftynse accepted this revision.May 29 2020, 4:07 AM
This revision is now accepted and ready to land.May 29 2020, 4:07 AM
This revision was automatically updated to reflect the committed changes.
nicolasvasilache marked 2 inline comments as done.