[llvm] dadedc9 - [InstCombine] visitUnreachableInst(): iteratively erase instructions leading to unreachable

[Roman Lebedev via llvm-commits]

Author: Roman Lebedev
Date: 2021-07-02T17:20:21+03:00
New Revision: dadedc99e9b276cfd0f2ebf9eb553650b07b4ca4

URL: https://github.com/llvm/llvm-project/commit/dadedc99e9b276cfd0f2ebf9eb553650b07b4ca4
DIFF: https://github.com/llvm/llvm-project/commit/dadedc99e9b276cfd0f2ebf9eb553650b07b4ca4.diff

LOG: [InstCombine] visitUnreachableInst(): iteratively erase instructions leading to unreachable

In the original review D87149 it was mentioned that this approach was tried,
and it lead to infinite combine loops, but i'm not seeing anything like that now,
neither in the `check-llvm`, nor on some codebases i tried.

This is a recommit of d9d65527c289fb27a9f92f150723bbb3c58e413f,
which i immediately reverted because i have messed up something
during branch switch, and 597ccc92ce4b0f90883406d1f78d9d776f602804
accidentally ended up being pushed, which was very much not the intention.

Reviewed By: spatel

Differential Revision: https://reviews.llvm.org/D105339

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 8f75a7eac6f95..d29527f3a0dcb 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2874,23 +2874,24 @@ Instruction *InstCombinerImpl::visitUnreachableInst(UnreachableInst &I) {
   // Try to remove the previous instruction if it must lead to unreachable.
   // This includes instructions like stores and "llvm.assume" that may not get
   // removed by simple dead code elimination.
-  Instruction *Prev = I.getPrevNonDebugInstruction();
-  if (Prev && !Prev->isEHPad() &&
-      isGuaranteedToTransferExecutionToSuccessor(Prev)) {
+  while (Instruction *Prev = I.getPrevNonDebugInstruction()) {
+    if (Prev->isEHPad() || !isGuaranteedToTransferExecutionToSuccessor(Prev))
+      return nullptr; // Can not drop any more instructions. We're done here.
     // Temporarily disable removal of volatile stores preceding unreachable,
     // pending a potential LangRef change permitting volatile stores to trap.
     // TODO: Either remove this code, or properly integrate the check into
     // isGuaranteedToTransferExecutionToSuccessor().
     if (auto *SI = dyn_cast<StoreInst>(Prev))
       if (SI->isVolatile())
-        return nullptr;
+        return nullptr; // Can not drop this instruction. We're done here.
 
     // A value may still have uses before we process it here (for example, in
     // another unreachable block), so convert those to poison.
     replaceInstUsesWith(*Prev, PoisonValue::get(Prev->getType()));
     eraseInstFromFunction(*Prev);
-    return &I;
   }
+  assert(I.getParent()->sizeWithoutDebug() == 1 && "The block is now empty.");
+  // FIXME: recurse into unconditional predecessors?
   return nullptr;
 }