Page MenuHomePhabricator

[mlir][Pattern] Add better support for using interfaces/traits to match root operations in rewrite patterns
ClosedPublic

Authored by rriddle on Mar 19 2021, 1:47 PM.

Details

Summary

To match an interface or trait, users currently have to use the MatchAny tag. This tag can be quite problematic for compile time for things like the canonicalizer, as the MatchAny patterns may get applied to *every* operation. This revision adds better support by bucketing interface/trait patterns based on which registered operations have them registered. This means that moving forward we will only attempt to match these patterns to operations that have this interface registered. Two simplify defining patterns that match traits and interfaces, two new utility classes have been added: OpTraitRewritePattern and OpInterfaceRewritePattern.

Diff Detail

Event Timeline

rriddle created this revision.Mar 19 2021, 1:47 PM
rriddle requested review of this revision.Mar 19 2021, 1:47 PM
mehdi_amini added inline comments.Mar 19 2021, 5:56 PM
mlir/include/mlir/IR/PatternMatch.h
193

Wouldn't RootKind fit in 2-bits? Could this be the low-bits of the rootValue?

ftynse accepted this revision.Mon, Mar 22, 8:41 AM

Nice!

Please fix the linter messages and consider folding RootKind into some pointer.

mlir/include/mlir/IR/PatternMatch.h
73

Nit: why uint16_t specifically?

This revision is now accepted and ready to land.Mon, Mar 22, 8:41 AM
rriddle marked an inline comment as done.Mon, Mar 22, 12:04 PM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
193

We can't use a PointerIntPair here because OperationName already uses a bit (internally it is a PointerUnion of AbstractionOperation/Identifier).

mehdi_amini accepted this revision.Mon, Mar 22, 1:22 PM
mehdi_amini added inline comments.
mlir/include/mlir/IR/PatternMatch.h
193

Ah! Maybe worth a comment?

Also would it fit within contextAndHasBoundedRecursion? (I don't know if there are 2 bits left there).

rriddle marked 2 inline comments as done.Mon, Mar 22, 1:23 PM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
193

I considered fitting it into the context, but given that there is already padding for the benefit(it is only 2 bytes) I just slotted it in next to that.

Nice, any performance tests you did?

jpienaar accepted this revision.Mon, Mar 22, 5:30 PM

Nice, thanks

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

Does the filter not have a context?

rriddle marked 2 inline comments as done.Tue, Mar 23, 2:04 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
667

Not that I could find.

This revision was landed with ongoing or failed builds.Tue, Mar 23, 2:12 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.