This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Allow Tile transform op to take dynamic sizes
ClosedPublic

Authored by ftynse on Jul 6 2022, 9:21 AM.

Details

Summary

Extend the definition of the Tile structured transform op to enable it
accepting handles to operations that produce tile sizes at runtime. This is
useful by itself and prepares for more advanced tiling strategies. Note that
the changes are relevant only to the transform dialect, the tiling
transformation itself already supports dynamic sizes.

Depends On D129216

Diff Detail

Event Timeline

ftynse created this revision.Jul 6 2022, 9:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 9:21 AM
ftynse requested review of this revision.Jul 6 2022, 9:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 9:21 AM
ftynse updated this revision to Diff 442890.Jul 7 2022, 6:38 AM

Fix include.

ftynse updated this revision to Diff 442910.Jul 7 2022, 7:32 AM

I rebased on the requested commit, but this is a non-functional change. If
rebasing implied using applyToOne, it is not applicable because TileOp now
takes several operand handles, which is not supported by applyToOne.

ftynse updated this revision to Diff 443204.Jul 8 2022, 4:40 AM

Rebase.

Nice improvements!

mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
415

Can you please add a pedantic `

Return modes:
=============
doc section that resembles the other ones and describes all handles manipulation to a 5-y-o ?
440

seems like you want a SmallVector<OpFoldResult> getMixedSizes or something here to uniformize usage?

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
185

does this need an error message or is this reasonably handled by errors from below ?

632–633

The fact that 0 encodes "do not tile along this dim" would ideally remain deeper and not leak up to here.
If we really want to manipulate at this level, we could use an interface but given you're just resizing here, I am wondering whether you can just rewrite:

for (const auto &en2 : llvm::enumerate(tiledOp->loops))
  loops[en2.index()].push_back(en2.value());

to something that only uses tiledOp->loops and which has already taken care of 0.

If that does not work it means that we have significant trickiness in the impl and much more doc is required (and prob an interface is warranted too)

634

SmallVector<Operation*> and friends usually resolve to 6, do you really care about setting a 4 here or does it fail to compile for some reason ?

652

this would moslty go away if you had a SmallVector<OpFoldResult>getMixedSizes and you would just
do vector of materializeOpFoldResult.

716

Ok I see you are actually making use of that 0 magic number knowledge.
In that case we should find a way to add it to the TilingInterface and use that more pervasively.

I rebased on the requested commit, but this is a non-functional change. If
rebasing implied using applyToOne, it is not applicable because TileOp now
takes several operand handles, which is not supported by applyToOne.

Nope just rebase because that change was somewhat tricky to land and potentially had merge conflicts

ftynse updated this revision to Diff 443909.Jul 12 2022, 3:52 AM
ftynse marked 7 inline comments as done.

Address review.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
185

It probably needs a better message, but this is out of the scope of this patch: the code is in FuseOp and is not touched by this commit.

634

Just decreasing the stack usage a bit here, the default-size computation was not designed for that.

652

It's more involved than that. The Value returned by getMixedSizes is the handle, not the payload op that produces the size, it cannot be just materialized.

716

Except that this tiling implementation doesn't use the interface.

nicolasvasilache accepted this revision.Jul 12 2022, 4:11 AM
This revision is now accepted and ready to land.Jul 12 2022, 4:11 AM
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
716

doesn't *yet* use the interface :)

This revision was landed with ongoing or failed builds.Jul 12 2022, 5:22 AM
This revision was automatically updated to reflect the committed changes.