Page MenuHomePhabricator

[mlir] Support dialect-wide canonicalization pattern registration
ClosedPublic

Authored by springerm on Wed, May 26, 10:15 PM.

Details

Summary
  • Add hasCanonicalizer option to Dialect.
  • Initialize canonicalizer with dialect-wide canonicalization patterns.
  • Add test case to TestDialect.

Dialect-wide canonicalization patterns are useful if a canonicalization pattern does not conceptually associate with any single operation, i.e., it should not be registered as part of an operation's getCanonicalizationPatterns function. E.g., this is the case for canonicalization patterns that match an op interface.

Diff Detail

Event Timeline

springerm created this revision.Wed, May 26, 10:15 PM
springerm requested review of this revision.Wed, May 26, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 26, 10:15 PM

LGTM, but please wait for at least a day to give a change to River to have a look as well.

mlir/include/mlir/IR/Dialect.h
77

The dialect already has a getContext() member I believe

mehdi_amini accepted this revision.Wed, May 26, 10:25 PM
This revision is now accepted and ready to land.Wed, May 26, 10:25 PM
rriddle accepted this revision.Wed, May 26, 10:35 PM

LGTM

mlir/include/mlir/IR/Dialect.h
71

Comments should be ///

77

Yes, the context parameter here isn't necessary. Dialect already has a context member.

mlir/test/lib/Dialect/Test/TestDialect.cpp
295
springerm updated this revision to Diff 348182.Thu, May 27, 1:27 AM
springerm marked 4 inline comments as done.

address comments

This revision was landed with ongoing or failed builds.Thu, May 27, 1:35 AM
This revision was automatically updated to reflect the committed changes.

So one thought that comes up for me with this patch is that while we have general ideas about what a canonicalization for an op should limit itself to (should be conceptually related to the op), I'm not sure there's restrictions in the code or clear documentation. I think this could lead to a weird scenario where just having registered a dialect will change the behavior of the canonicalize pass, even if that dialect is completely unused. This can already happen today with op canonicalizations if the canonicalization doesn't require the presence of the op it is registered on. I think that such a canonicalization is kind of obviously a bad idea (but we should maybe document that in https://mlir.llvm.org/docs/Canonicalization/). With the addition of dialect canonicalization patterns though, it seems like this scenario is more likely, especially with the stated use case of a canonicalization that operates on an interface. Since interfaces are not tied to dialects, this means someone could declare a canonicalization pattern that operates on CastOpInterface, which would make sense because it's something their dialect uses, but this could affect the behavior of other dialects. In some ways this is a powerful mechanism for declaring canonicalizations on other ops you don't own, but if we want to support that I think it should probably be more specific. I'm not sure there's much we can do in the code to enforce this, but maybe a dialect canonicalization pattern should ensure that it only matches when ops from that dialect are present (and we should document this and a similar restriction on op canonicalization patterns). Thoughts?