[InstCombine] If inst in unreachable refers to an inst change it to poison (#65107)

[ParkHanbum]

Instructions in unreachable basic blocks are removed, but terminators
are not. In this case, even instructions that are only referenced by
a terminator, such as a return instruction, cannot be processed
properly.

This patch changes the operand of a return instruction in an
unreachable basic block to poison if it refers to the instruction,
allowing the instruction to be properly processed.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: hanbeom (ParkHanbum)

Changes

After we move instruction to an unreachable basic block, we need to
handle it appropriately. if not,
removeAllNonTerminatorAndEHPadInstructions is called within
prepareWorklist preparing for the second combining process, and
instructions in the unreachable block are processed.

in this case, we are considered to have modified instruction, so
message LLVM ERROR: Instruction Combining did not reach a fixpoint after 1 iterations is showing and exits.

---

Full diff: https://github.com/llvm/llvm-project/pull/78444.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineInternal.h (+1-1)
• (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+22-3)
• (modified) llvm/test/Transforms/InstCombine/sink_to_unreachable.ll (+72)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
index 21c61bd990184d..5a1e1726946510 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
+++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
@@ -736,7 +736,7 @@ class LLVM_LIBRARY_VISIBILITY InstCombinerImpl final
   bool removeInstructionsBeforeUnreachable(Instruction &I);
   void addDeadEdge(BasicBlock *From, BasicBlock *To,
                    SmallVectorImpl<BasicBlock *> &Worklist);
-  void handleUnreachableFrom(Instruction *I,
+  bool handleUnreachableFrom(Instruction *I,
                              SmallVectorImpl<BasicBlock *> &Worklist);
   void handlePotentiallyDeadBlocks(SmallVectorImpl<BasicBlock *> &Worklist);
   void handlePotentiallyDeadSuccessors(BasicBlock *BB, BasicBlock *LiveSucc);
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 7f2018b3a19958..f4411e52376ff3 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3082,22 +3082,23 @@ void InstCombinerImpl::addDeadEdge(BasicBlock *From, BasicBlock *To,
 
 // Under the assumption that I is unreachable, remove it and following
 // instructions. Changes are reported directly to MadeIRChange.
-void InstCombinerImpl::handleUnreachableFrom(
+bool InstCombinerImpl::handleUnreachableFrom(
     Instruction *I, SmallVectorImpl<BasicBlock *> &Worklist) {
+  bool Changed = false;
   BasicBlock *BB = I->getParent();
   for (Instruction &Inst : make_early_inc_range(
            make_range(std::next(BB->getTerminator()->getReverseIterator()),
                       std::next(I->getReverseIterator())))) {
     if (!Inst.use_empty() && !Inst.getType()->isTokenTy()) {
       replaceInstUsesWith(Inst, PoisonValue::get(Inst.getType()));
-      MadeIRChange = true;
+      Changed = true;
     }
     if (Inst.isEHPad() || Inst.getType()->isTokenTy())
       continue;
     // RemoveDIs: erase debug-info on this instruction manually.
     Inst.dropDbgValues();
     eraseInstFromFunction(Inst);
-    MadeIRChange = true;
+    Changed  = true;
   }
 
   // RemoveDIs: to match behaviour in dbg.value mode, drop debug-info on
@@ -3107,6 +3108,8 @@ void InstCombinerImpl::handleUnreachableFrom(
   // Handle potentially dead successors.
   for (BasicBlock *Succ : successors(BB))
     addDeadEdge(BB, Succ, Worklist);
+
+  return MadeIRChange = (Changed | MadeIRChange);
 }
 
 void InstCombinerImpl::handlePotentiallyDeadBlocks(
@@ -4396,6 +4399,22 @@ bool InstCombinerImpl::run() {
         for (Use &U : I->operands())
           if (Instruction *OpI = dyn_cast<Instruction>(U.get()))
             Worklist.push(OpI);
+
+        // When we sink instruction, we must consider the case
+        // where the baisc block to which the instruction is moved
+        // is unreachable. Otherwise, a problem occurs where the
+        // sinked instruction removes the instruction from the
+        // second loop.
+        if (all_of(predecessors(UserParent), [&](BasicBlock *Pred) {
+              return DeadEdges.contains({Pred, UserParent}) ||
+                     DT.dominates(UserParent, Pred);
+            })) {
+          SmallVector<BasicBlock *> DeadBlockCandidates;
+          if (handleUnreachableFrom(I, DeadBlockCandidates)) {
+            handlePotentiallyDeadBlocks(DeadBlockCandidates);
+            continue;
+          }
+        }
       }
     }
 
diff --git a/llvm/test/Transforms/InstCombine/sink_to_unreachable.ll b/llvm/test/Transforms/InstCombine/sink_to_unreachable.ll
index e788b634da8868..193b0d5a704093 100644
--- a/llvm/test/Transforms/InstCombine/sink_to_unreachable.ll
+++ b/llvm/test/Transforms/InstCombine/sink_to_unreachable.ll
@@ -157,3 +157,75 @@ bb3:
   %p = phi i32 [0, %bb1], [%a, %bb2]
   ret i32 %p
 }
+
+define i1 @src_sink_and_remove_bb(i16 %0) {
+; CHECK-LABEL: @src_sink_and_remove_bb(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    br i1 true, label [[BB1]], label [[UNREACHABLE:%.*]]
+; CHECK:       unreachable:
+; CHECK-NEXT:    ret i1 poison
+;
+entry:
+  br label %bb1
+bb1:
+  %unknown = icmp ult i16 255, %0
+  br i1 true, label %bb1, label %unreachable
+unreachable:
+  ret i1 %unknown
+}
+
+define i1 @src_sink_and_remove_entry(i16 %0) {
+; CHECK-LABEL: @src_sink_and_remove_entry(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    br i1 true, label [[BB1]], label [[UNREACHABLE:%.*]]
+; CHECK:       unreachable:
+; CHECK-NEXT:    ret i1 poison
+;
+entry:
+  %unknown = icmp ult i16 255, %0
+  br label %bb1
+bb1:
+  br i1 true, label %bb1, label %unreachable
+unreachable:
+  ret i1 %unknown
+}
+
+define i1 @src_sink_and_remove(i8 %0) {
+; CHECK-LABEL: @src_sink_and_remove(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    br i1 false, label [[UNREACHABLE:%.*]], label [[BB1]]
+; CHECK:       unreachable:
+; CHECK-NEXT:    ret i1 poison
+;
+entry:
+  %unknown = icmp ult i8 255, %0
+  br label %bb1
+bb1:
+  br i1 false, label %unreachable, label %bb1
+unreachable:
+  ret i1 %unknown
+}
+
+define i1 @src_sink_and_combine(i8 %0) {
+; CHECK-LABEL: @src_sink_and_combine(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    br i1 false, label [[BB1]], label [[UNREACHABLE:%.*]]
+; CHECK:       unreachable:
+; CHECK-NEXT:    ret i1 false
+;
+entry:
+  br label %bb1
+bb1:
+  %unknown = icmp ult i8 255, %0
+  br i1 false, label %bb1, label %unreachable
+unreachable:
+  ret i1 %unknown
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

As I wrote in #65107 (comment), I don't think this is the correct way to fix the issue.

[ParkHanbum]

As I wrote in #65107 (comment), I don't think this is the correct way to fix the issue.

I read your comment but I didn't know what that mean. what we do incorrectly exactly?

[nikic]

@ParkHanbum My suggestion is to fold

define i1 @f(i16 %0) {
  br label %BB1

BB1:                                              ; preds = %BB1, %BB
  %C = icmp ult i16 256, %0
  br i1 true, label %BB1, label %BB2

BB2:                                              ; preds = %BB1
  ret i1 %C
}

to

define i1 @f(i16 %0) {
  br label %BB1

BB1:                                              ; preds = %BB1, %BB
  %C = icmp ult i16 256, %0
  br i1 true, label %BB1, label %BB2

BB2:                                              ; preds = %BB1
  ret i1 poison
}

at which point %C becomes dead and can be removed, without having it to sink into BB2 first.

This is a more general solution, which allows us to handle additional cases. For example, cases where we wouldn't sink the instruction due to complex CFG, and cases where the instruction is used both in the dead block and somewhere else, in which case removing the use in the dead block may enable a one-use fold.

[ParkHanbum]

@nikic sounds reasonable. I'll change it like that!

[dtcxzyw]

Could you have a look at dtcxzyw/llvm-opt-benchmark#226 (comment)?
It is wired to replace an argument in a reachable invocation with a poison.

[nikic]

LGTM

[aaronpuchert]

As this fixes a crash, do you think we should port it back to LLVM 18? We're running into this with KDE 6 on aarch64.

CC @tstellar.

[nikic]

@aaronpuchert No, this change shouldn't be backported -- it's an optimization improvement, not a bug fix. The error is a development tool, not intended to surface to users.

You probably need to backport this mesa patch: https://gitlab.freedesktop.org/mesa/mesa/-/commit/99f0449987bec1f82cd42a06f40bb4a863a37792 We ran into a similar openQA failure in Fedora, and this is the fix for it.

[aaronpuchert]

Thanks, I was just confused by the crash tag on #65107. Will forward your suggestion.