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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

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


https://reviews.llvm.org/D29252