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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

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.

> 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? 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.

This patch regresses code that was perfectly good over years.

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.


https://reviews.llvm.org/D35851