Page MenuHomePhabricator

[mlir][Linalg] Rewrite CodegenStrategy to populate a pass pipeline.
ClosedPublic

Authored by nicolasvasilache on Sep 28 2021, 10:57 PM.

Details

Summary

This revision retires a good portion of the complexity of the codegen strategy and puts the logic behind pass logic.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Sep 28 2021, 10:57 PM
gysit accepted this revision.Sep 29 2021, 12:16 AM

nice simplification!

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

nit: I think it is MatchAnyOpTypeTag but I think I prefer a comment similar to the promotion pattern that just says "entry point to match any LinalgOp ..."

820

nit: I think the second half of the comment got lost.

mlir/lib/Dialect/Linalg/Transforms/LinalgStrategyPasses.cpp
56

nit: seems like a corner case but I would probably put braces since the statements are fairly complex.

86

same here

116

same here

155

nit: here I would also put braces on the outer if.

This revision is now accepted and ready to land.Sep 29 2021, 12:16 AM
gysit added inline comments.Sep 29 2021, 1:11 AM
mlir/include/mlir/Dialect/Linalg/Transforms/CodegenStrategy.h
87

nit: LinalgOpType -> LinalgOp since the template parameter is gone.

gysit added inline comments.Sep 29 2021, 1:30 AM
mlir/lib/Dialect/Linalg/Transforms/LinalgStrategyPasses.cpp
85

promotionPattern?

pifon2a accepted this revision.Sep 29 2021, 1:43 AM

So much nicer now!

mlir/include/mlir/Dialect/Linalg/Passes.h
87

nit: filter instead of filt? opt is used commonly for options, but i am not sure about filt.

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

is there some special reason not to use OpRewritePattern for LinalgBaseTilingPattern?

nicolasvasilache marked 10 inline comments as done.Sep 29 2021, 5:09 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
597

Basically history: when this was written, OpRewritePattern did not work with interfaces.
When interface support was added, uses in Linalg were not updated.

Will keep as a followup cleanup or an intro task for someone who has cycles.

nicolasvasilache marked an inline comment as done.

Address comments.

Properly wire the anchorOp option to avoid interferences between transformations.

This revision was landed with ongoing or failed builds.Sep 29 2021, 6:39 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Oct 18 2021, 10:10 AM
mlir/lib/Dialect/Linalg/Transforms/CodegenStrategy.cpp
62

I'm concerned about this: we're not supporting running a pass manager from inside a pass manager: you need to use the dynamic pipeline feature for this: https://mlir.llvm.org/docs/PassManagement/#dynamic-pass-pipelines

Can you update this?

(we're also increasing the amount of asserts around these situations).

Actually, I have very strong concern with any Pass hooking to the infrastructure that isn't made "hermetic" : you have passes here that takes arbitrary C++ code here which breaks any pipeline serialization and will generate incorrect crash reproducer.
Didn't we talk about this?