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 ↗(On Diff #266635)

Nit: newYieldedValues -> newIterOperands

mlir/lib/Transforms/Utils/LoopUtils.cpp
2534 ↗(On Diff #266635)

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

2541 ↗(On Diff #266635)

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

2546 ↗(On Diff #266635)

Same here?

2551 ↗(On Diff #266635)

How about using without_terminator()here?

2555 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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

2541 ↗(On Diff #266635)

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

2555 ↗(On Diff #266635)

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

2567 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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 ↗(On Diff #267075)

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 ↗(On Diff #266635)

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 ↗(On Diff #266635)

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.