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
2532

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

2539

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

2544

Same here?

2549

How about using without_terminator()here?

2553

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
2528

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

2539

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

2553

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

2565

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
2565

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
2539–2541

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().

2558

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
2532

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.

2565

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
2565

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
2565

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.