Revert "[InstCombine] erase instructions leading up to unreachable"

[stephan.yichao.zhao]

Summary

This reverts commit b22910daab95be1ebc6ab8a74190e38130b0e6ef.

See the discussion from https://reviews.llvm.org/D87149

1. it is fine to remove instructions that can reach an unreachable instruction because running unreachable is undefined, and undefined behavior is retroactive.

2. however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

It is possible to extend llvm::isGuaranteedToTransferExecutionToSuccessor to make this work.

Revert the change temporarily.

[nikic]

however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

As already explained in the referenced revision, this is not correct. Volatile accesses are not permitted to trap. LangRef is unambiguous on this point:

The compiler may assume execution will continue after a volatile operation, so operations which modify memory or may have undefined behavior can be hoisted past a volatile operation.

[chandlerc]

In D87399#2263996, @nikic wrote:

however, a volatile access can abort a program. In that case, an unreachable instruction after it is not reachable. So undefined behavior does not happen, and the volatile access cannot be removed.

As already explained in the referenced revision, this is not correct. Volatile accesses are not permitted to trap. LangRef is unambiguous on this point:

The compiler may assume execution will continue after a volatile operation, so operations which modify memory or may have undefined behavior can be hoisted past a volatile operation.

I'll follow up on this point in the original review thread for continuity.

[nikic]

Just to be clear, it is okay to revert this, but you need to adjust your rationale in the description to be one of two things:

• This broke BigProject due to reliance on undefined behavior, we need a day to fix it. Will reapply once done.
• I plan to (expeditiously) submit an RFC to llvm-dev that changes the behavior of volatile operations in LLVM, undoing the decision from D53184. Will reapply once that discussion is finished.

[spatel]

Admittedly, zapping stores (volatile or not) was not in the original motivation for the patch, but it seemed nice to see that enhancement.
Reverting until things are sorted out is ok with me, but I agree with @nikic - since the LangRef is clear today about the behavior and that was hashed out on llvm-dev, somebody (not me) needs to rehash that on llvm-dev. Also, you'll need to reopen https://llvm.org/PR47416 and/or propose an alternate fix.

[stephan.yichao.zhao]

Abandoned this one after @nikic applied https://github.com/llvm/llvm-project/commit/4e413e16216d0c94ada2171f3c59e0a85f4fa4b6
Thank you for everyone's help.

[lebedev.ri]

In D87399#2265991, @stephan.yichao.zhao wrote:

Abandoned this one after @nikic applied https://github.com/llvm/llvm-project/commit/4e413e16216d0c94ada2171f3c59e0a85f4fa4b6
Thank you for everyone's help.

I echo what @nikic said in https://reviews.llvm.org/D87399#2264195
This is a *temporary* stop-gap measure, it will be reverted *soon* unless someone who cares actively comes forward soon.

[lebedev.ri]

Alright, i think 3+ months was plenty time,
but as far as i can tell, no one stepped up forwards to deal with it.

@stephan.yichao.zhao unless someone would like to do so now,
i'm going to escalate this: i'll be reverting the temporary patch soon.

llvm/lib/Transforms/InstCombine/InstCombineInternal.h

public:
   Instruction *visitLoadInst(LoadInst &LI);
   Instruction *visitStoreInst(StoreInst &SI);
   Instruction *visitAtomicRMWInst(AtomicRMWInst &SI);
   Instruction *visitUnconditionalBranchInst(BranchInst &BI);
   Instruction *visitBranchInst(BranchInst &BI);
   Instruction *visitFenceInst(FenceInst &FI);
   Instruction *visitSwitchInst(SwitchInst &SI);
   Instruction *visitReturnInst(ReturnInst &RI);
-  Instruction *visitUnreachableInst(UnreachableInst &I);
   Instruction *
   foldAggregateConstructionIntoAggregateReuse(InsertValueInst &OrigIVI);
   Instruction *visitInsertValueInst(InsertValueInst &IV);
   Instruction *visitInsertElementInst(InsertElementInst &IE);
   Instruction *visitExtractElementInst(ExtractElementInst &EI);
   Instruction *visitShuffleVectorInst(ShuffleVectorInst &SVI);
   Instruction *visitExtractValueInst(ExtractValueInst &EV);
   Instruction *visitLandingPadInst(LandingPadInst &LI);

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

Instruction *InstCombinerImpl::visitReturnInst(ReturnInst &RI) {
   KnownBits Known = computeKnownBits(ResultOp, 0, &RI);
   if (Known.isConstant())
     return replaceOperand(RI, 0,
         Constant::getIntegerValue(VTy, Known.getConstant()));
 
   return nullptr;
 }
 
-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)) {
-    eraseInstFromFunction(*Prev);
-    return &I;
-  }
-  return nullptr;
-}
-
 Instruction *InstCombinerImpl::visitUnconditionalBranchInst(BranchInst &BI) {
   assert(BI.isUnconditional() && "Only for unconditional branches.");
 
   // If this store is the second-to-last instruction in the basic block
   // (excluding debug info and bitcasts of pointers) and if the block ends with
   // an unconditional branch, try to move the store to the successor block.
 
   auto GetLastSinkableStore = [](BasicBlock::iterator BBI) {

llvm/test/Transforms/InstCombine/assume.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -instcombine -S -instcombine-infinite-loop-threshold=2 | FileCheck %s
+; RUN: opt < %s -instcombine -S | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare void @llvm.assume(i1) #1
 
 ; Check that the alignment has been upgraded and that the assume has not
 ; been removed:
;
   ret i8 %add
 }
 
 ; Another case of conflicting assumptions. This would crash because we'd
 ; try to set more known bits than existed in the known bits struct.
 
 define void @PR36270(i32 %b) {
 ; CHECK-LABEL: @PR36270(
+; CHECK-NEXT:    tail call void @llvm.assume(i1 false)
 ; CHECK-NEXT:    unreachable
 ;
   %B7 = xor i32 -1, 2147483647
   %and1 = and i32 %b, 3
   %B12 = lshr i32 %B7, %and1
   %C1 = icmp ult i32 %and1, %B12
   tail call void @llvm.assume(i1 %C1)
   %cmp2 = icmp eq i32 0, %B12

 ; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i32 [[X]], 1
 ; CHECK-NEXT:    br i1 [[CMP2]], label [[IF:%.*]], label [[EXIT:%.*]]
 ; CHECK:       if:
 ; CHECK-NEXT:    [[A:%.*]] = and i32 [[Y]], -2
 ; CHECK-NEXT:    [[CMP3:%.*]] = icmp ne i32 [[A]], 104
 ; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP3]])
 ; CHECK-NEXT:    br label [[EXIT]]
 ; CHECK:       exit:
+; CHECK-NEXT:    [[CMP4:%.*]] = icmp eq i32 [[X]], 2
+; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP4]])
 ; CHECK-NEXT:    unreachable
 ;
 entry:
   %cmp0 = icmp sgt i32 %x, 1
   %cmp1 = icmp eq i32 %y, 1
   %or = or i1 %cmp0, %cmp1
   tail call void @llvm.assume(i1 %or)
   %cmp2 = icmp eq i32 %x, 1

 ; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i32 [[X]], 1
 ; CHECK-NEXT:    br i1 [[CMP2]], label [[IF:%.*]], label [[EXIT:%.*]]
 ; CHECK:       if:
 ; CHECK-NEXT:    [[A:%.*]] = and i32 [[Y]], -2
 ; CHECK-NEXT:    [[CMP3:%.*]] = icmp ne i32 [[A]], 104
 ; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP3]])
 ; CHECK-NEXT:    br label [[EXIT]]
 ; CHECK:       exit:
+; CHECK-NEXT:    [[CMP4:%.*]] = icmp eq i32 [[X]], 2
+; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP4]])
+; CHECK-NEXT:    [[CMP5:%.*]] = icmp ugt i32 [[Y]], 42
+; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP5]])
+; CHECK-NEXT:    store i32 [[X]], i32* [[P:%.*]], align 4
 ; CHECK-NEXT:    unreachable
 ;
 entry:
   %cmp0 = icmp sgt i32 %x, 1
   %cmp1 = icmp eq i32 %y, 1
   %or = or i1 %cmp0, %cmp1
   tail call void @llvm.assume(i1 %or)
   %cmp2 = icmp eq i32 %x, 1

llvm/test/Transforms/InstCombine/pr33689_same_bitwidth.ll

 
 define void @f(i1 %cond) {
 ; CHECK-LABEL: @f(
 ; CHECK-NEXT:  bb0:
 ; CHECK-NEXT:    [[T12:%.*]] = alloca [2 x i32], align 8
 ; CHECK-NEXT:    [[T12_SUB:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[T12]], i16 0, i16 0
 ; CHECK-NEXT:    br i1 [[COND:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
 ; CHECK:       bb1:
+; CHECK-NEXT:    [[T8:%.*]] = ptrtoint [2 x i32]* [[T12]] to i16
+; CHECK-NEXT:    store i16 [[T8]], i16* @a, align 2
 ; CHECK-NEXT:    unreachable
 ; CHECK:       bb2:
 ; CHECK-NEXT:    [[T9:%.*]] = load i16*, i16** @b, align 2
 ; CHECK-NEXT:    store i16 0, i16* [[T9]], align 2
 ; CHECK-NEXT:    [[T10:%.*]] = load i32, i32* [[T12_SUB]], align 8
 ; CHECK-NEXT:    [[T11:%.*]] = add i32 [[T10]], -1
 ; CHECK-NEXT:    store i32 [[T11]], i32* [[T12_SUB]], align 8
 ; CHECK-NEXT:    ret void