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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

In https://reviews.llvm.org/D30667#702834, @sanjoy wrote:

> In https://reviews.llvm.org/D30667#700016, @hfinkel wrote:
>
> > There is? We could propagate the unreachableness of this function to its callsites, but I don't see what that has to do with the value of the function argument.
>
>
>
>
> In https://reviews.llvm.org/D30667#701680, @dberlin wrote:
>
> > Okay, so to close this out, what would we like to do about isGuaranteedToTransferExecutionToSuccessor?
> >  Continue have it return false?
> >
> > Past that, it's clear additional bugs should be filed against gvnhoist.
>
>
> My inclination would be to have it explicitly (with a nice comment) return true.  That would make bugs like the GVNHoist one we're talking about here more obviously buggy.


In the same function isGuaranteedToTransferExecutionToSuccessor, line: ~3783, there is a comment.

  // If there is no successor, then execution can't transfer to it.
  if (const auto *CRI = dyn_cast<CleanupReturnInst>(I))
    return !CRI->unwindsToCaller();

Since a basic block with unreachable instruction will not have any successor, shouldn't it follow the same logic. I agree that in the example that you gave it would be better if gvnhoist would hoist the instruction, and there is a bug in gvnhoist (that I'm working on to fix), but isGuaranteedToTransferExecutionToSuccessor is called from other places as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D30667