[PATCH] D29252: [NewGVN] Update dominator tree for unreachable blocks.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D29252#659701, @dberlin wrote:

> Yeah, we discovered this yesterday.
>
> The underlying issue is twofold
>
> 1. changeToUnreachable is supposed to be local, but modifies the CFG :(
> 2. You can't insert unreachableinst except as a terminator.
>
>   I'm torn here. Chandler's suggestion was to preserve the CFG. That will work, actually, right up until we add PRE. Then we are going to split critical edges for sure. (though we definitely miss some optimizations due to not splitting critical edges right now, it's just more common with PRE).
>
>   Current GVN does not preserve CFG. If we want to preserve CFG, we should do a store of undef to null (which simplifycfg will transform into unreachable) and revert the original patch.
>
>   NewGVN is also an analysis with an elimination pass tacked on (and built so we can eventually split it up that way). The analysis should not modify the CFG for sure, but it isn't right now, so that's okay. This is about the elimination part, and whether non-PRE should try to preserve CFG.
>
>   Chandler, your thoughts welcome on whether it's worth trying to preserve CFG in the case we don't do PRE


There is no fundamental reason you should preserve the CFG. The reason I suggest it whenever possible is only because it allows us to cache many more analyses. You can update domtree, but the number of analyses you get to preserve by preserving the CFG goes up *substantially* and you don't want to try to update all of them.

That said,, as long as this is one big pass, as you say, you'll want to split critical edges in part of it, etc. Don't stress about it in that context.

If you're going to split this, I think it is fairly likely that some reduced transformation passes will want to preserve the CFG so they can be relatively non-disruptive to analyses. Some use cases off the top of my head:

- GVNHoist style code motion seems likely to want to leave analyses in tact
- I'd love to have a fast, small LoopGVN that could be run in the middle of the loop pipeline to catch incremental improvements that allow subsequent loop passes to be more effective
- Some of the places we currently use EarlyCSE

Anyways, all of this seems pretty forward-looking, so I don't think it should necessarily decide what you do right now, just something to keep in mind as things progress. If there are easy ways to set the pass up for this long term, might be worthwhile.


https://reviews.llvm.org/D29252