This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add a TileToForeachThread transform.
ClosedPublic

Authored by nicolasvasilache on Jul 12 2022, 10:25 AM.

Details

Summary

This revision adds a new transformation to tile a TilingInterface op to a tiled scf.foreach_thread, applying
tiling by num_threads.
If non-empty, the threadDimMapping is added as an attribute to the resulting scf.foreach_thread.
0-tile sizes (i.e. tile by the full size of the data) are used to encode
that a dimension is not tiled.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jul 12 2022, 10:25 AM
ftynse requested changes to this revision.Jul 13 2022, 2:34 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
568–569
574–575

Please use the appropriate header. Underscore === is a h1 and it wrecks the generated documentation.

592–593

I find it confusing to have "tile op" and "tiled op". Can one of these rather be "foreach_thread_op" ?

595

Non-blocking: could we rather adopt the view-like syntax $target[$num_threads] directly because we will have to switch to that to support dynamic number of threads as the TODO above indicates.

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

It looks like the code in tileToForeachThreadOp will have always reported an error, so it's better to just return DiagnosedSilenceableFailure::definiteFailure() here to avoid a second error message.

Also, this doesn't seem to match the description of the op that says it fails silently. reportUnknownTransformError produces a LogicalResult::failure(), which is converted into definite failure by DiagnosedSilenceableFailure and not into a silenceable one.

808

I don't see why we would ever need it. Patterns are only adding complexity here.

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
172–191

Please add top-level docs for these.

210

Nit: you no longer need to specify the size in llvm::to_vector. In this particular case, because you specified 4, it will force a copy from a 4-element vector on the RHS to the 6-element vector on the LHS.

250–253

I find the Range interface confusing. Isn't "size" intended as the distance between the lower bound (offset) and the upper bound (offset + size) of the iteration space dimension? If so, it should not be necessary to subtract the offset here. If not, it would be much better off called "upper bound" than "size". Note that it is impossible to catch now because all ops have zero iteration space offsets.

276
This revision now requires changes to proceed.Jul 13 2022, 2:34 AM
nicolasvasilache marked 10 inline comments as done.

Rebase and address review.

nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
807

Used return emitDefaultSilenceableFailure(target);
I find that nested error messages are usually not informative enough and like to see where in the transform dialect IR this happens.
Lmk if I am missing something ..

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
250–253

It is indeed the size, thanks!

springerm accepted this revision.Jul 19 2022, 3:57 AM
springerm added inline comments.
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
579

properly as in "without failure"?

581–582

Not sure what this means. If no tiled op was produced, the transform must have failed. In that case we would not continue executing the sequence anyway.

585

Not necessarily a linalg op

mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
527

maybe replace with e.g. as not every tiledOp is a linalg op

nicolasvasilache marked 4 inline comments as done.Jul 19 2022, 4:50 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
581–582

The op may always be given handles to things that are all ignored and it may return and propagate the empty list.
This is the generic wording that we have been using to make the user aware of the fact that results may always be empty.

nicolasvasilache marked an inline comment as done.

Rebase and address last comments.

Update doc comments.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 19 2022, 4:59 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.