[PATCH] D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

I have an intern starting in two weeks to do incremental dominator algorithms :)

In https://reviews.llvm.org/D32740#752838, @chandlerc wrote:

> In https://reviews.llvm.org/D32740#748894, @davide wrote:
>
> > The algorithm you have, I think, it's correct to start (although as pointed out by Sanjoy doesn't really compute the dominance frontier).
> >  Have you considered computing the iterated dominance frontier every time instead? If there are technical difficulties (or it's just slower) I'd add a comment to the code explaining why we can't use it.
>
>
> Added some comments about why the existing IDF calculator seems like a bad fit for what we want here.


TBQH, both of the comment seem quite wrong.

1. Yes it computes dom tree levels each time. This is trivially fixable, but historically has never been a time sink.
2. It includes extra complexity and logic to allow PHI placement which is //    somewhat fundamentally a harder problem than the one we're trying to //    solve here, so we can get away with a simpler approach. Specifically, //    we don't need to do any live-in pruning.

No it doesn't and not it isn't :)
At least, the issues you describe are ... not a good reason to avoid it.
The problem of proper phi placement  is not "fundamentally harder ". it's quite literally identical. :)
Second, the live-in pruning is completely optional, and costs nothing when it is off.
As an example, MemorySSA does not use the live-in pruning.

If you do not set the live in blocks, it simply does not use them.
This is why the comment says "/// By default, liveness is not used to prune the IDF computation."


https://reviews.llvm.org/D32740