Page MenuHomePhabricator

[mlir] loop::ForOp: provide builders with callbacks for loop body
ClosedPublic

Authored by ftynse on Sun, May 10, 5:25 AM.

Details

Summary

Thanks to a recent change that made ::build functions take an instance of
OpBuilder, it is now possible to build operations within a region attached to
the operaiton about to be created. Exercise this on loop::ForOp by taking a
callback that populates the loop body while the loop is being created.

Additionally, provide helper functions to build perfect nests of ForOps,
with support for iteration arguments. These functions provide the same
functionality as EDSC LoopNestBuilder with simpler implementation, without
relying on edsc::ScopedContext, and using OpBuilder in an unambiguous way.
Compatibility functions for EDSC are provided, but may be removed in the
future.

Diff Detail

Event Timeline

ftynse created this revision.Sun, May 10, 5:25 AM
Herald added a project: Restricted Project. · View Herald Transcript
mehdi_amini accepted this revision.Sun, May 10, 10:54 AM

Typo in the commit description: operaiton -> operation

mlir/lib/Dialect/LoopOps/LoopOps.cpp
56 ↗(On Diff #263069)

Can you document the condition?

I assume it is something like "if there are iterArgs provided, we don't create a default terminator as it must take the same argument types."

This revision is now accepted and ready to land.Sun, May 10, 10:54 AM
bondhugula requested changes to this revision.Sun, May 10, 5:06 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.h
72 ↗(On Diff #263069)

Did you mean "without iteration arguments (like for reductions)" instead of "without reductions"?

mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
496 ↗(On Diff #263069)

/*bodyBuilder=*/

570 ↗(On Diff #263069)

/*bodyBuilder=*/

mlir/lib/Dialect/GPU/Transforms/MemoryPromotion.cpp
82–83

Likewise.

mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp
152 ↗(On Diff #263069)

Likewise.

mlir/test/EDSC/builder-api-test.cpp
145

/*lbs=*/..., /*ubs=*/..., /*step=*/...)

This revision now requires changes to proceed.Sun, May 10, 5:06 PM

Mostly minor comments. On another note, I've never seen ValueVector before - I don't find it upstream either. Is this based on another pending revision?

nicolasvasilache accepted this revision.Mon, May 11, 6:31 AM
ftynse marked 8 inline comments as done.Mon, May 11, 6:59 AM

On another note, I've never seen ValueVector before - I don't find it upstream either.

It's defined in line 47 of LoopOps.h / SCF.h in this commit.

mlir/include/mlir/Dialect/LoopOps/LoopOps.h
72 ↗(On Diff #263069)

I think one implies the other, but okay to change.

mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
496 ↗(On Diff #263069)

We use argument-naming comments for literals when they can be interpreted ambiguously https://llvm.org/docs/CodingStandards.html#comment-formatting. (e.g. we don't add comment to getRegion(0).) This argument is neither a literal nor really ambiguous.

570 ↗(On Diff #263069)

Same as above, this defies the purpose of loop nest constructor.

mlir/test/EDSC/builder-api-test.cpp
145

Same as above, we don't systematically do this and I don't see a strong reason for it, especially in this commit.

ftynse updated this revision to Diff 263164.Mon, May 11, 7:07 AM
ftynse marked an inline comment as done.

Address reviews

ftynse updated this revision to Diff 263165.Mon, May 11, 7:09 AM

Drop debug output

bondhugula marked an inline comment as done.Tue, May 12, 3:58 AM
bondhugula added inline comments.
mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
496 ↗(On Diff #263069)

That URL is about comment formatting rules, and not about the necessity of such argument documentation. The LLVM style is actually silent on recommending its usage except a passing reference there, and so is the MLIR style. I think we should discuss this separately on discord --- using such documenting names where it's not obvious what the argument is (to a new user/browser) greatly improves call site readability, and in this instance as well. Unfortunately, a lot of the new code is missing out on this due to the lack of a guideline. Fine for this commit, but I'll bring this up for discussion on the list.

ftynse marked an inline comment as done.Tue, May 12, 4:30 AM
ftynse added inline comments.
mlir/lib/Conversion/VectorToLoops/ConvertVectorToLoops.cpp
496 ↗(On Diff #263069)

I linked the only relevant piece I found.

Please prefer discourse (forum) to discord (chat), I find it extremely annoying to use the chat because it prompts short immediate answers that must arrive during night time at my location, or are otherwise ignored due to the lack of context. I will spare my arguments for that discussion. I don't think this change needs to be blocked on it.

bondhugula resigned from this revision.Sun, May 17, 12:44 PM

Looks good for the parts I saw. Please go ahead and commit if the primary reviewer (@nicolasvasilache) is fine.

This revision is now accepted and ready to land.Sun, May 17, 12:44 PM
This revision was automatically updated to reflect the committed changes.