[Mlir-commits] [mlir] [mlir][scf] Add simple LICM pattern for `scf.while` (PR #76370)

[Mehdi Amini]

joker-eph wrote:

> But then I cannot access the existing canonicalization patterns so I would have to rewrite them which is obviously not practical.

But that's already the case, I'm saying: nothing new here.

> Because canonicalization doesn't have knobs if we widen what we consider to be canonicalizations we lose in control.

This comment seems to me to apply to any addition of a canonicalization pattern from day-1... 

> This reduce the modularity of MLIR patterns which I think is going to be a downside for users.

I feel you're mixing up the "addition of a new pattern you don't like", with the fundamental design that "canonicalization patterns are an opaque bag and you don't get to choose".

That is: there is no design change here. You just happen to realize that canonicalization patterns is a growing set of transformations and you can't cherry-pick. 

Unless we freeze the set of patterns, we're necessarily in the situation you're describing, as far as I understand.


> It's confusing, because I agree with your statement and still think LICM should be LICM, not canonicalization. Not because the two snippets you show are different (they're not), but because these are well known passes (like CSE, DCE) which are arguably all forms of canonicalization.

Yes and we run DCE during canonicalization :p 
(actually we don't even have a DCE pass in MLIR I think? But maybe we should just for specific "quick cleanups"...)
The fact that these transformations have historically got a name does not make them out-of-scope of canonicalization in nature IMO. The name just refer to the technique itself, not much more.
The reason we don't do CSE during canonicalization for example has more to do with "we don't know how to do it efficiently" than anything else, if CSE was O(1) it would likely be part of the canonicalization pass I believe.

So for canonicalization candidates I rather look at:
- is the transformation performed with only local information?
- is the transformation "cheap" enough or does it require extensive computation?
- is the transformation undoable easily (with similar local information), that is "no information is lost".

Some of the impact of keeping a transformation within canonicalization is the cross-transformation impact: you need to perform these interleaved in the same IR traversal to converge. You can often craft input IR where performing one before the other does not lead to the desired simplification.


https://github.com/llvm/llvm-project/pull/76370