[mlir][docs] Add docs on canonicalizers being folders or patterns #129517
Conversation
Groverkss
requested review from
jpienaar,
rengolin,
ftynse,
MaheshRavishankar,
banach-space,
joker-eph and
kuhar
|
@llvm/pr-subscribers-mlir Author: Kunwar Grover (Groverkss) ChangesFull diff: https://github.com/llvm/llvm-project/pull/129517.diff 1 Files Affected:
diff --git a/mlir/docs/Canonicalization.md b/mlir/docs/Canonicalization.md
index 6e59a4128093a..c0553d6698c6a 100644
--- a/mlir/docs/Canonicalization.md
+++ b/mlir/docs/Canonicalization.md
@@ -275,3 +275,26 @@ Operation *MyDialect::materializeConstant(OpBuilder &builder, Attribute value,
...
}
```
+
+### Choosing between implementing a Folder or a `RewritePattern`
+
+When implementing a new canonicalization, an important thing to think about is
+if the canonicalization should be a folder or a `RewritePattern`. The generally
+accepted policy is:
+
+* If a transformation should be a canonicalizer is a different question from
+ if a transformation should be a folder or a `RewritePattern`. A
+ transformation is promoted to a canonicalization as defined by the General
+ Design of canonicalizations.
+
+* If a transformation is a canonicalization, there are two ways to implement
+ it: As a Folder or as a `RewritePattern`, both of which are implementation
+ details of how a canonicalization exists, with the difference being folders
+ have restrictions on what they can do. A `RewritePattern` can implement any
+ transformation a folder can.
+
+* A canonicalization should always be implemented as a Folder if it fits
+ the "local" definition of a folder.
+
+* If a canonicalization does not fit as a folder, it should be implemented
+ as a `RewritePattern`.
|
mlir/docs/Canonicalization.md
Outdated
| * If a transformation should be a canonicalizer is a different question from | ||
| if a transformation should be a folder or a `RewritePattern`. A | ||
| transformation is promoted to a canonicalization as defined by the General | ||
| Design of canonicalizations. |
There was a problem hiding this comment.
Maybe write this in a more imperative way? For example, you could add a 'NOTE' marker that says: 'To decide whether a transformation should be a canonicalization in the first place, see the General Design of Canonicalization.' (link)
There was a problem hiding this comment.
I'm not sure if I want it to be a NOTE. I'm trying to stress the point here that you cannot say that a transformation should be a RewritePattern canonicalization, but it shouldn't be a folder canonicalization based on what the transformation does. It's an implementation detail. The only thing you have to reason about is if a transformation should be a canonicalization or not. If it's a folder or a RewritePattern is dependent on it's implementation.
mlir/docs/Canonicalization.md
Outdated
| Design of canonicalizations. | ||
|
|
||
| * If a transformation is a canonicalization, there are two ways to implement | ||
| it: As a Folder or as a `RewritePattern`, both of which are implementation |
There was a problem hiding this comment.
I'd also demote this bullet to a new paragraph after the 'NOTE'
There was a problem hiding this comment.
Hey @Groverkss , thank you so much for sending this!
I've left a couple of comments inline, but I also have one high-level comment.
There are already two fairly dense paragraphs on the two approaches to implementing "canonicalizations":
I think that you can safely trim your addition to the following paragraph (I tweaked the title so that the naming aligns with what's currently used in this document):
### When to use the `fold` method vs `RewriterPattern`s for canonicalizations
A canonicalization should always be implemented as a Folder if it fits the "local" definition of a folder, otherwise it should be implemented as a `RewritePattern`.
From what I can tell, this is the main new bit of info added in this PR . The other items seem to repeat what's already here. For example:
If a transformation is a canonicalization, there are two ways to implement
it: As a Folder or as aRewritePattern, both of which are implementation
That's already covered by the paragraphs that I linked :) (i.e. that's why we have them)
Also, this part doesn't really get us closer to understanding what to prefer - the "fold" method vs RewriterPattern:
If a transformation should be a canonicalizer is a different question from
if a transformation should be a folder or aRewritePattern. A
transformation is promoted to a canonicalization as defined by the General
Design of canonicalizations.
One other point, could there be some other heuristics to consider here? For example, the cost and the frequency with which different "implementations" would be run.
Removed repeated lines and changed to a one liner. |
…vm#129517) If a transformation should be a canonicalization is an orthogonal question to if a transformation should be implemented as a `RewritePattern` or a `fold` method. The later is an implementation detail. This patch adds a suggestion to always implement a canonicalization as a `fold` pattern if possible, as they are a restricted subset of a `RewritePattern`. This has been a common source of confusion, as to when to implement a canonicalization as a fold method or a RewritePattern.
If a transformation should be a canonicalization is an orthogonal question to if a transformation should be implemented as a
RewritePatternor afoldmethod. The later is an implementation detail.This patch adds a suggestion to always implement a canonicalization as a
foldpattern if possible, as they are a restricted subset of aRewritePattern.This has been a common source of confusion, as to when to implement a canonicalization as a fold method or a RewritePattern.