[PATCH] D35851: [Dominators] Include infinite loops in PostDominatorTree

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D35851#838768, @grosser wrote:

> In https://reviews.llvm.org/D35851#836771, @davide wrote:
>
> > I think this patch {c,sh}ould be considered for commit as is.
>
>
> OK, another person is voting against me. I certainly won't single handedly block this this. As I said, I would like to have a brief opinion from Hal, as we discussed extensively. I will ask him. If he agrees with you guys, I am out.
>
> > The reason is twofold:
> > 
> > 1. The current postdom behaviour has some known brokeness that caused several bugs. PDSE is just an example, but try to fuzz LLVM and you'll find all this sort of craziness. I've reviewed the original change in January to change this behaviour, which was reverted because you disagreed. I'm afraid that until somebody sits down and implements what you propose (probably you :) the discussion can go on and on indefinitely.
>
> I implemented an alternative (and proposed a second which I implemented earlier). I am still waiting for a test-case to break them.


There is nothing in llvm today that will break.  It's been designed for the things we do today, which already completely ignore large classes of types of blocks.  As Chandler said, i'm not sure that's a good use case to base the future on.
 It's about what we want to implement in the future, and looking at what others have done to make that happen.
Your other approach is based on what you consider to be reasonable invariants: You are okay with, given one of the infinite loop examples

      A
   /    \
  B     C

Where C is an infinite loop, saying that all paths to exit go through B.
That's definitely going to break optimizations we want to implement. You want me to go through, in excruciating detail, how.   You want implementations of those optimizations to read. 
Rather than come up with endless papers and examples here, and some point i think my suggestion is:   Compile a GCC or Open64 where you've disabled connecting the infinite loops to exit before postdom, and see what breaks, and understand that.  Because it does break!
In gcc, you'd have to hack up calc_dfs_tree a bit, since it's done inside post-dominators itself, besides some optimizations.
In Open64, you should be able to edit CFG::Process_multi_entryexit to not connect the infinite loops to exit (it does it all the time before computing postdom)
That's likely to lead to cfgs and understanding a lot easier than me trying to explain PDSE to you from first principles :)

> 
> 
>> 2. I'd say this patch is an improvement on the current state, so I don't think we should hold on forever.
> 
> So you are explicitly not concerned about the fact that these virtual edges weaken the post-dominance relation?

See, your view is they weaken it, while others have a perspective that it is making it conservatively correct.

> If a pass inserts certain checks at post-dominance points and later wants to verify this invariant still holds, the invariant may be broken by **deleting** dead edges. I think this is a real concern.

They are not forward-dead, actually.   That's part of the whole problem. As Jakub pointed out long ago, the fact that they are reverse unreachable does not mean they cannot be executed.

> This patch regresses code that was perfectly good over years.

This code has known be broken for many years, and we have patches waiting for review that implement optimizations that even try to disable themselves in the presence of this these postdom bugs. There is external code that does the same.  I know you disagree, so i'm going to leave this alone past that.  I don't think we will agree here.

> One path to go quickly forward would be to commit my alternative (which does not regress much code), continue to develop code based on this alternative until we find test cases that actually break. It seems Daniel has some, I need to go over his mail now.

Please don't.
Look.
One of my big problem with all of this is that you are suggesting we are treading on fresh snow. From your perspective, no literature deals with this,  explicitly, and we are on our own, and can do what we want, so we should try to do something good. That's awesome. We should strive to do the best we can.
Except: we aren't treading on fresh snow.

They had infinite loops in the 1980's and 1990's too, and, for example, the authors of papers i cited will happily tell you their compilers either

1. Inserted fake edges in the CFG
2. Pretended those fake edges existed during postdominators.

They didn't just do nothing.

Your own research showed that other compilers take this approach, and *no compiler or literature takes your approach* (and FWIW, at least one of the ones you say does nothing actually carefully ensures the CFG represents infinite loops in a certain way).
Even "building a compiler" books say that you should insert fake edges from infinite loops to exit or you will break global optimizations.

At some point, i think it's unreasonable to say "all of this is inapplicable".
I think at some point, the onus is on you to prove that your approach won't break these things, even if we don't implement them yet, since the whole point of doing this is for us to be able to implement them.
There are patches outstanding for some of these optimizations if you want to try them (but i'd recommend using gcc or open64, see below)
In part simply disagree about what regions should look like, and it's unlikely we'll ever agree.
So i'm going to stick with "does it break optimizations".
We are suggesting doing a thing we *know* works with *all* of the algorithms, today's, and the future ones, because it's implemented.
(I again, am cognizant of the fact you consider the test case changes to be regressions). I don't believe they are, and they are the same as other compilers generate)

If you want to try your way in those things, and see what will break, that's awesome.  I'm pretty sure the answer is "a bunch".   I suggest those over applying existing opt patches out for review because those compilers have worked this way for years, so "most" bugs have been worked out :)  In particular, Open64 has probably the most complete implementation of things using post-dominators.


https://reviews.llvm.org/D35851