[PATCH] D30667: GVNHoist: handle basic blocks with UnreachableInst

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D30667#699682, @efriedma wrote:

> This is definitely papering over the problem; for example, take the given testcase, and replace the "unreachable" with "br label %exit".


Right.
The only reason i approve it is because we currently say "nothing" about unreachable, and i felt we should say *something*.
As i said earlier, gvnhoist is clearly broken either way.

> I would say it makes more sense to return false, as opposed to true... but only because there aren't any successors to transfer execution to.

Thinking harder about it, i'm pretty sure if we answer true we get wrong answers in some cases. But they may not matter.

For example, it would make us think, in some cases,  it is okay to sink things from reachable parts to  past the unreachable, no?
But it's really not.
My gut feeling is it's possible to create a case where we do the wrong thing (IE turn defined behavior into undefined behavior), but perhaps we aren't that clever.

Staring at other places it is used, it looks like it would also think it okay to propagate various non-nullness attributes, etc past unreachable.
That looks more innocuous, at least at a glance.


Repository:
  rL LLVM

https://reviews.llvm.org/D30667