[Mlir-commits] [mlir] [MLIR][SCF] Sink scf.if from scf.while before region into after region. (PR #165216)

[Mehdi Amini]

joker-eph wrote:

> So please tell me what are the principles arguments for what should be a canonicalization.

I believe I did, multiple times, but I can re-iterate. It turns out we actually have a documentation here: https://mlir.llvm.org/docs/Canonicalization/ ; in particular it links to an pretty good [an article](https://sunfishcode.github.io/blog/2018/10/22/Canonicalization.html) on the topic.
The section [General Design](https://mlir.llvm.org/docs/Canonicalization/#general-design) lays down the principles to consider for canonicalization. Happy to iterate further on this if we need to refine.

> I have provided an actual concrete reason why this shouldnt be a canonicalization in my opinion (not a matter of taste).

You indeed elaborate a little bit in your previous message, thanks for this. That said it feels a bit fuzzy to me still, let me quote the only concrete sentence:

> But in any case, the pattern is doing a whole bunch of code motion and moving instructions around. That is an extremely heavy weight transformation. This is not a targeted "get to a better representation of an operation" transformation.

You're here repeating the use of "heavy weight transformation" which is ambiguous to me. I understand it as "expensive" in terms of compile-time, which is why I asked before: 

> Which part? The transformation itself? As long as the matching phase has a "fail fast" path it seems fine to me actually.
The most important thing is to do as little work as possible in the pattern when there is nothing to do.

But you didn't specifically followed-up on this.

So now that you're prefixing it with "doing a whole bunch of code motion and moving instructions around", I suspect you may intended to address something about the scope of the transformation to talk about "heavyweight"?
I'm not sure about the relevance of it for canonicalization though?

> Now can you provide a counter argument as to why this should be a canonicalization.

Sure: 
- It does not seem costly to apply.
- I don't see any useful information being lost here
- We found an equivalence of the program toward one that eliminates the `scf.if` entirely, and reduce the `scf.while` condition block to one that is more easily understandable by subsequent analyses. The test case actually demonstrates this: the while-to-for conversion manages to match this form post-transformation whereas it does not otherwise. Turning a `scf.while` into a `scf.for` is also more canonical to me as it encodes structurally more information: the `scf.for` defines a fixed iteration space that must recovered by analysis on the `scf.while`, allowing subsequent transformation to get access to this information in a straightforward way on the `scf.for`.



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