Page MenuHomePhabricator

[mlir] Allow for patterns to match any root kind.
ClosedPublic

Authored by rriddle on Wed, Jun 17, 5:55 PM.

Details

Summary

Traditionally patterns have always had the root operation kind hardcoded to a specific operation name. This has worked well for quite some time, but it has certain limitations that make it undesirable. For example, some lowering have the same implementation for many different operations types with a few lowering entire dialects using the same pattern implementation. This problem has led to several "solutions":
a) Provide a template implementation to the user so that they can instantiate it for each operation combination, generally requiring the inclusion of the auto-generated operation definition file.
b) Use a non-templated pattern that allows for providing the name of the operation to match

  • No one ever does this, because enumerating operation names can be cumbersome and so this quickly devolves into solution a.

This revision removes the restriction that patterns have a hardcoded root type, and allows for a class patterns that could match "any" operation type. The major downside of root-agnostic patterns is that they make certain pattern analyses more difficult, so it is still very highly encouraged that an operation specific pattern be used whenever possible.

Depends On D81985

Diff Detail

Event Timeline

rriddle created this revision.Wed, Jun 17, 5:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jun 17, 5:55 PM
ftynse accepted this revision.Thu, Jun 18, 2:28 AM

I wanted this for long time, thanks!

One minor suggestion: if we want to make people aware they create non-op-specific patterns, we can require to pass in a tag type (struct MatchAnyOperation{};) instead of the operation name in the Pattern constructor. Arguably, if it is explicit and doesn't save keystrokes, there will be less temptation to use "the constructor with shorter argument list" as the first choice.

mlir/include/mlir/IR/PatternMatch.h
468

Nit: I don't see ConstructorArg here

mlir/lib/Transforms/DialectConversion.cpp
1558

Ultra-nit: capitalize Legalizer in anyOplegalizerPatterns

This revision is now accepted and ready to land.Thu, Jun 18, 2:28 AM

Thanks for adding this @rriddle .

mlir/include/mlir/IR/PatternMatch.h
535

Is the failure case expected to be more commonly specialized by the user which justifies onFailure is before onSuccess in the API ?

mlir/lib/IR/PatternMatch.cpp
261

typo: then

mlir/lib/Transforms/DialectConversion.cpp
1528

Seeing the extra complexity that appear when using this with legalization I am wondering whether it would be better to conservatively enforce a root in such cases?
Do you foresee that we are going to more generally use this in conjunction with A->B->C legalizations?
I can definitely see the benefit for individual/greedy pattern application but the A->B->C behavior looks iffy.

I can't claim I understand all the ramifications here, do you see this as a "nah it's fine don't worry" or as opening potential cans of worms ?

rriddle updated this revision to Diff 271809.Thu, Jun 18, 12:31 PM
rriddle marked 7 inline comments as done.

Address comments

I wanted this for long time, thanks!

One minor suggestion: if we want to make people aware they create non-op-specific patterns, we can require to pass in a tag type (struct MatchAnyOperation{};) instead of the operation name in the Pattern constructor. Arguably, if it is explicit and doesn't save keystrokes, there will be less temptation to use "the constructor with shorter argument list" as the first choice.

Sounds great, added.

mlir/include/mlir/IR/PatternMatch.h
535

I couldn't say really. There are only two pattern drivers in core MLIR, and not any interesting ones internal to google. There are no use cases right now that would only specialize onSuccess, but not onFailure(or vice-versa). If many come up, we could swap but it is premature at this point given that there are relatively few drivers.

mlir/lib/Transforms/DialectConversion.cpp
1528

There are two parts of the A->B->C that are affected:

  • This function, which mostly just does filtering of patterns that will "never" generate a legal result.

Negatively affecting this function isn't something I'm terribly concerned about ATM. Its only an optimization, and I'm of the opinion that we can do much better if we rewrote this and/or exposed at a higher level.

  • The cost model

This one is a bit more concerning, but given the current set of usages that I've seen (expect) I'm not terribly concerned at the moment. I'm taking the same "wait and see" approach that I took when first writing the cost model. Even though the patterns *could* match any operation, in general I don't think these patterns will largely affect the cost model of others so I've kept it relatively simple. If this ever becomes a problem and we need a better cost analysis, we can rewrite and adapt. Until then, if everything runs relatively smoothly I don't see a need to over-engineer.

This revision was automatically updated to reflect the committed changes.

Do you have any thoughts on matching roots based on their properties such as a supported interface? I could imagine patterns that match on any op that has memory effects, for example.

Do you have any thoughts on matching roots based on their properties such as a supported interface? I could imagine patterns that match on any op that has memory effects, for example.

That is what this patch allows. You can add whatever kind of matching you want.