Page MenuHomePhabricator

[mlir][Linalg] Make LinalgBaseTilingPattern not delete the original operation.
ClosedPublic

Authored by mravishankar on Sep 8 2020, 11:28 AM.

Details

Summary

The LinalgTilingPattern class dervied from the base deletes the
original operation. This allows for the use case where the more
transformations are necessary on the original operation after
tiling. In such cases the pattern can derive from
LinalgBaseTilingPattern instead of LinalgTilingPattern.

Diff Detail

Event Timeline

mravishankar created this revision.Sep 8 2020, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2020, 11:28 AM
mravishankar requested review of this revision.Sep 8 2020, 11:28 AM

Could you please describe the use case a bit?
Should this be an abstract base that never deletes and a default derived that deletes + other derived that do X and delete?

If you have a separate CL here or in IREE that relies on the behavior it would be useful to help get an idea where this is going.

Thanks!

The specific IREEs use case might not be very relevant here (though this is part of this PR). Without going into those details, I think the expectation is that any user of Linalg tiling must be able to just use the LinalgTilingPattern. In IREE, I have a pattern class that inherits from this class. After tile (and distribute), I need the original operation to generate some IREE specific information, but the op is deleted. The alternative would be that I copy the guts of the matchAndRewrite method of LinalgBaseTilingPattern and use that directly, so that I can generate the information I need from the op before deleting it. I would prefer not to do that. My thought here is that this option is an opt-in for use cases where the original op is still needed after the tiling transformation. So a flag that allows you to opt-in to this made sense to me.

The alternative would be that I copy the guts of the matchAndRewrite method of LinalgBaseTilingPattern

Right so my suggestion here would be to make LinalgBaseTilingPattern to *never* erase the op and the existing derived LinalgTilingPattern override matchAndRewrite to call the base and then eraseOp.
Then your patterns can do the same: override matchAndRewrite, call the parent matchAndRewrite, do X and eraseOp.

It seems we can do that without adding a new option.
The reason I'd prefer to avoid new options is that any new knob is more complexity to precondition/search.

Addressing comments

The alternative would be that I copy the guts of the matchAndRewrite method of LinalgBaseTilingPattern

Right so my suggestion here would be to make LinalgBaseTilingPattern to *never* erase the op and the existing derived LinalgTilingPattern override matchAndRewrite to call the base and then eraseOp.
Then your patterns can do the same: override matchAndRewrite, call the parent matchAndRewrite, do X and eraseOp.

It seems we can do that without adding a new option.
The reason I'd prefer to avoid new options is that any new knob is more complexity to precondition/search.

This works too. Made the change. THanks!

mravishankar retitled this revision from [mlir][Linalg] Add option to LinalgTilingOptions to not delete the operation after tiling. to [mlir][Linalg] Make LinalgBaseTilingPattern not delete the original operation..Sep 10 2020, 9:53 PM
mravishankar edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Sep 10 2020, 11:53 PM
This revision was landed with ongoing or failed builds.Sep 11 2020, 12:39 AM
This revision was automatically updated to reflect the committed changes.