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

[Mehdi Amini]

joker-eph wrote:

> I have tried to describe the issue I have with canonicalization too, 

Sure, I just plainly disagree with your approach to it.

> I think it is completely justified ask for people adding something to a canonicalization, to justify why in all possible scenarios something is canonical, but that is not done today. Instead people pushing back against it are asked to justify why something shouldnt be a canonicalization. That seems backwards to how basic proofs work. I cant just claim some mathematical fact. It needs to be proved. The proof cant be "no one gave me a counter-example, so I must be right".

You seem to put what seems to me to be an impossible bar to reach for any practical development in LLVM/MLIR historically speaking: I don't see how anyone can "prove" this. The system isn't defined in terms of mathematical lattice that we can reason about, a lot of it is ad-hoc engineering: this is obviously imperfect but this is what we have to work with. 

> Another piece of evidence of canonicalizations being used as kitchen sink is the huge compilations times canonicalizations take.

I don't quite see what conclusion you're trying to make from this observation, I can imagine many root causes which have nothing to do with what you're trying to convey here.

>  I dont know how else to state it. As it stands, the proclivity of everyone just adding to canonicalization without justification or analysis is a bad practice being promoted which is ultimately going to be a major issue for anyone building a serious compiler stack using MLIR.

If this is what you believe, fine: just stop using canonicalization! Nobody forces you to bring these into your compiler.

> Specific to this example, to me the movement of operations from the while region to the do region without any side-effect analysis seems like a red flag. 

As far as I understand, this transformation is doing:

```
if (%cond) {
  foo()
}
cond_br %cond ^continue, ^end

^continue:
    ...
^end:
   ...
```

-> 

```
cond_br %cond ^continue, ^end

^continue:
    foo()
    ...
^end:
   ...
```

I don't quite get why we'd need any side-effect analysis here.


> The movement of operations itself is a compilation time overhead cause it will periodically trigger recomputation of the operation order within multiple blocks which is expensive depending on the program. Such factors should have already been accounted for when adding something to a canonicalization, but that is hardly ever done.

That's totally not the kind of things I would consider. First it's highly dependent on the particular pass pipeline and the shape of the IR, but also that would preclude a lot of obvious canonicalization, starting with folding `if (true) { <sequence> }` to `<sequence>`.



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