This is an archive of the discontinued LLVM Phabricator instance.

[mlir][TilingInterface]NFC Refactor of tile and fuse using `TilingInterface`.
ClosedPublic

Authored by mravishankar on Sep 18 2022, 1:02 PM.

Details

Summary

This patch refactors the tiling and tile + fuse implementation using
TilingInterface. Primarily, it exposes the functionality as simple
utility functions instead of as a Pattern to allow calling it from a
pattern as it is done in the test today or from within the transform
dialect (in the future). This is a step towards deprecating similar
methods in Linalg dialect.

  • The utility methods do not erase the root operations.
  • The return value provides the values to use for replacements.
  • Along the way also simplify the implementation and remove unnecessary methods. In particular, it is not legal to replace all uses of values made iter_args within the loop body. That replacement needs to happen in a more targeted manner only when fusion happens through operands that are destination operands. Gaurd this change in the utility method to avoid doing that during the tile (+ fuse) transformation.

Diff Detail

Event Timeline

mravishankar created this revision.Sep 18 2022, 1:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
mravishankar requested review of this revision.Sep 18 2022, 1:02 PM
nicolasvasilache requested changes to this revision.Sep 21 2022, 7:31 AM

This is getting more reviewable but it is still trying to sneak a load-bearing behavior through an NFC commit :)

Please drop anything related to destinationIterArg to make this truly NFC then we can evaluate that part separately.

Since this is the 3rd time we go through this, if this is getting too annoying, please lmk and I'll commander.

mlir/include/mlir/Dialect/SCF/Transforms/TileUsingInterface.h
76–86

tiling is by itself oblivious to the existent of a consumer, please drop this from the name as it is confusing to bring in fusion-related concepts here

95

guaranteed

mlir/include/mlir/IR/OpDefinition.h
243 ↗(On Diff #461094)

please drop this change, this PR does should not touch core IR

mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
284

This feels a lot like premature optimization where the APIs is made significantly more complex for efficiency reasons that I'd like to see measured.
I strongly prefer we get the simplest possible API first, really understand the problem in its simplest form, then measure and then invest in removing overhead.

287

It would be better and more future-proof for this yieldTiledValues to take a SubsetExtractOpInterface and return an instance of the symmetrical SubsetInsertOpInterface.
Depending on the op, we can then perform the getMixedOffsets() inside that.
This will make it easy to support other extract / insert op pairs and in particular parallel_insert ops.

You can approximate this by just passing ArrayRef<Operation*> sliceOps for now and switch to dyn_cast<tensor::ExtractSliceOp> + add a TODO.

This revision now requires changes to proceed.Sep 21 2022, 7:31 AM

Added https://reviews.llvm.org/D134411 to separate out the functional changes from this patch. Once that lands, Ill rebase on top of that to make this a true NFC.

Rebase on top of D134411 to make this truly NFC.

mravishankar retitled this revision from [mlir][TilingInterface] Mostly NFC Refactor of tile and fuse using `TilingInterface`. to [mlir][TilingInterface]NFC Refactor of tile and fuse using `TilingInterface`..Sep 26 2022, 12:16 PM
mravishankar marked 2 inline comments as done.Sep 26 2022, 12:20 PM

@nicolasvasilache this change is now just NFC.

mlir/include/mlir/IR/OpDefinition.h
243 ↗(On Diff #461094)

This helps with debugging. All dump methods are const. This seems to be an error. Ok to add this as part of this (now NFC) change?

mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
287

Agreed! That would be clean. Added a TODO to move to using the interface that will come as part of the overall change to move TilingInterface to use SubsetExtractOpInterface.

Address comments.

Drop changes to OpDefinition.h.

Thanks @mravishankar !
Minor comment but this is otherwise GTG!

mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
391

we usually dump debug to dbgs, any reason to go to errs here ?

This revision is now accepted and ready to land.Sep 28 2022, 12:43 PM
mravishankar marked an inline comment as done.Sep 28 2022, 1:26 PM
This revision was landed with ongoing or failed builds.Sep 28 2022, 1:26 PM
This revision was automatically updated to reflect the committed changes.