[Local] Do not introduce a new `llvm.trap` before `unreachable`

[jdoerfert]

Summary

This is the second attempt to remove the llvm.trap insertion after
https://reviews.llvm.org/rGe14e7bc4b889dfaffb7180d176a03311df2d4ae6
reverted the first one. It is not clear what the exact issue was back
then and it might already be gone by now, it has been >5 years after
all.

Replaces D106299.

[lebedev.ri]

LG

[JonChesterfield]

I like this lots more than the abandoned patch. Thanks!

[lebedev.ri]

In D106308#2888519, @JonChesterfield wrote:

I like this lots more than the abandoned patch. Thanks!

Me too!

[jdoerfert]

Adjust tests, including 2 XFAILS.

[jdoerfert]

I will XFAIL llvm/test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll as part of this. The test is full of UB and runs O2, it is not surprising it will eventually break. I tried to make the pointers arguments but that changed the output. The author (@iteratee) seem to be inactive by now and there was no review on phab, I think.
I made a similar comment here: https://reviews.llvm.org/D53450#2889172

[efriedma]

You can probably repair the ARM/Thumb tests using -arm-atomic-cfg-tidy=0... not a great solution, but if it works, better than XFAILing the test.

[jdoerfert]

Replace XFAIL

[efriedma]

LGTM

llvm/include/llvm/Transforms/Utils/Local.h

 /// is the number of instructions (excluding debug info instrinsics) that have
 /// been removed, and the second element is the number of debug info intrinsics
 /// that have been removed.
 std::pair<unsigned, unsigned>
 removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB);
 
 /// Insert an unreachable instruction before the specified
 /// instruction, making it and the rest of the code in the block dead.
-unsigned changeToUnreachable(Instruction *I, bool UseLLVMTrap,
+unsigned changeToUnreachable(Instruction *I,
                              bool PreserveLCSSA = false,
                              DomTreeUpdater *DTU = nullptr,
                              MemorySSAUpdater *MSSAU = nullptr);
 
 /// Convert the CallInst to InvokeInst with the specified unwind edge basic
 /// block.  This also splits the basic block where CI is located, because
 /// InvokeInst is a terminator instruction.  Returns the newly split basic
 /// block.

llvm/lib/CodeGen/WinEHPrepare.cpp

for (BasicBlock *BB : BlocksInFunclet) {
         // This call site was not part of this funclet, remove it.
         if (isa<InvokeInst>(CB)) {
           // Remove the unwind edge if it was an invoke.
           removeUnwindEdge(BB);
           // Get a pointer to the new call.
           BasicBlock::iterator CallI =
               std::prev(BB->getTerminator()->getIterator());
           auto *CI = cast<CallInst>(&*CallI);
-          changeToUnreachable(CI, /*UseLLVMTrap=*/false);
+          changeToUnreachable(CI);
         } else {
-          changeToUnreachable(&I, /*UseLLVMTrap=*/false);
+          changeToUnreachable(&I);
         }
 
         // There are no more instructions in the block (except for unreachable),
         // we are done.
         break;
       }
 
       Instruction *TI = BB->getTerminator();
       // CatchPadInst and CleanupPadInst can't transfer control to a ReturnInst.
       bool IsUnreachableRet = isa<ReturnInst>(TI) && FuncletPad;
       // The token consumed by a CatchReturnInst must match the funclet token.
       bool IsUnreachableCatchret = false;
       if (auto *CRI = dyn_cast<CatchReturnInst>(TI))
         IsUnreachableCatchret = CRI->getCatchPad() != CatchPad;
       // The token consumed by a CleanupReturnInst must match the funclet token.
       bool IsUnreachableCleanupret = false;
       if (auto *CRI = dyn_cast<CleanupReturnInst>(TI))
         IsUnreachableCleanupret = CRI->getCleanupPad() != CleanupPad;
       if (IsUnreachableRet || IsUnreachableCatchret ||
           IsUnreachableCleanupret) {
-        changeToUnreachable(TI, /*UseLLVMTrap=*/false);
+        changeToUnreachable(TI);
       } else if (isa<InvokeInst>(TI)) {
         if (Personality == EHPersonality::MSVC_CXX && CleanupPad) {
           // Invokes within a cleanuppad for the MSVC++ personality never
           // transfer control to their unwind edge: the personality will
           // terminate the program.
           removeUnwindEdge(BB);
         }
       }

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

void CoroCloner::create() {
   // In these ABIs, the cloned functions always return 'void', and the
   // existing return sites are meaningless.  Note that for unique
   // continuations, this includes the returns associated with suspends;
   // this is fine because we can't suspend twice.
   case coro::ABI::Switch:
   case coro::ABI::RetconOnce:
     // Remove old returns.
     for (ReturnInst *Return : Returns)
-      changeToUnreachable(Return, /*UseLLVMTrap=*/false);
+      changeToUnreachable(Return);
     break;
 
   // With multi-suspend continuations, we'll already have eliminated the
   // original returns and inserted returns before all the suspend points,
   // so we want to leave any returns in place.
   case coro::ABI::Retcon:
     break;
   // Async lowering will insert musttail call functions at all suspend points

llvm/lib/Transforms/Coroutines/Coroutines.cpp

for (AnyCoroSuspendInst *CS : CoroSuspends) {
       CS->replaceAllUsesWith(UndefValue::get(CS->getType()));
       CS->eraseFromParent();
       if (auto *CoroSave = CS->getCoroSave())
         CoroSave->eraseFromParent();
     }
 
     // Replace all coro.ends with unreachable instruction.
     for (AnyCoroEndInst *CE : CoroEnds)
-      changeToUnreachable(CE, /*UseLLVMTrap=*/false);
+      changeToUnreachable(CE);
 
     return;
   }
 
   auto Id = CoroBegin->getId();
   switch (auto IdIntrinsic = Id->getIntrinsicID()) {
   case Intrinsic::coro_id: {
     auto SwitchId = cast<CoroIdInst>(Id);

llvm/lib/Transforms/IPO/Attributor.cpp

for (Instruction *I : TerminatorsToFold) {
     CGModifiedFunctions.insert(I->getFunction());
     ConstantFoldTerminator(I->getParent());
   }
   for (auto &V : ToBeChangedToUnreachableInsts)
     if (Instruction *I = dyn_cast_or_null<Instruction>(V)) {
       if (!isRunOn(*I->getFunction()))
         continue;
       CGModifiedFunctions.insert(I->getFunction());
-      changeToUnreachable(I, /* UseLLVMTrap */ false);
+      changeToUnreachable(I);
     }
 
   for (auto &V : ToBeDeletedInsts) {
     if (Instruction *I = dyn_cast_or_null<Instruction>(V)) {
       if (auto *CB = dyn_cast<CallBase>(I)) {
         if (!isRunOn(*I->getFunction()))
           continue;
         if (!isa<IntrinsicInst>(CB))

llvm/lib/Transforms/IPO/PruneEH.cpp

for (BasicBlock::iterator I = BB->end(), E = BB->begin(); I != E; ) {
     }
 
     if (!I->use_empty())
       I->replaceAllUsesWith(UndefValue::get(I->getType()));
   }
 
   if (TokenInst) {
     if (!TokenInst->isTerminator())
-      changeToUnreachable(TokenInst->getNextNode(), /*UseLLVMTrap=*/false);
+      changeToUnreachable(TokenInst->getNextNode());
   } else {
     // Get the list of successors of this block.
     std::vector<BasicBlock *> Succs(succ_begin(BB), succ_end(BB));
 
     for (unsigned i = 0, e = Succs.size(); i != e; ++i)
       Succs[i]->removePredecessor(BB);
 
     BB->eraseFromParent();
   }
 }

llvm/lib/Transforms/Scalar/SCCP.cpp

for (Function &F : M) {
     }
 
     DomTreeUpdater DTU = Solver.getDTU(F);
     // Change dead blocks to unreachable. We do it after replacing constants
     // in all executable blocks, because changeToUnreachable may remove PHI
     // nodes in executable blocks we found values for. The function's entry
     // block is not part of BlocksToErase, so we have to handle it separately.
     for (BasicBlock *BB : BlocksToErase) {
       NumInstRemoved +=
-          changeToUnreachable(BB->getFirstNonPHI(), /*UseLLVMTrap=*/false,
+          changeToUnreachable(BB->getFirstNonPHI(),
                               /*PreserveLCSSA=*/false, &DTU);
     }
     if (!Solver.isBlockExecutable(&F.front()))
       NumInstRemoved += changeToUnreachable(F.front().getFirstNonPHI(),
-                                            /*UseLLVMTrap=*/false,
                                             /*PreserveLCSSA=*/false, &DTU);
 
     for (BasicBlock &BB : F)
       MadeChanges |= removeNonFeasibleEdges(Solver, &BB, DTU);
 
     for (BasicBlock *DeadBB : BlocksToErase)
       DTU.deleteBB(DeadBB);
 

llvm/lib/Transforms/Utils/InlineFunction.cpp

for (Function::iterator BB = FirstNewBlock->getIterator(),
       }
 
       // It is problematic if the inlinee has a cleanupret which unwinds to
       // caller and we inline it into a call site which doesn't unwind but into
       // an EH pad that does.  Such an edge must be dynamically unreachable.
       // As such, we replace the cleanupret with unreachable.
       if (auto *CleanupRet = dyn_cast<CleanupReturnInst>(BB->getTerminator()))
         if (CleanupRet->unwindsToCaller() && EHPadForCallUnwindsLocally)
-          changeToUnreachable(CleanupRet, /*UseLLVMTrap=*/false);
+          changeToUnreachable(CleanupRet);
 
       Instruction *I = BB->getFirstNonPHI();
       if (!I->isEHPad())
         continue;
 
       if (auto *CatchSwitch = dyn_cast<CatchSwitchInst>(I)) {
         if (isa<ConstantTokenNone>(CatchSwitch->getParentPad()))
           CatchSwitch->setParentPad(CallSiteEHPad);

llvm/lib/Transforms/Utils/Local.cpp

if (isa<DbgInfoIntrinsic>(Inst))
       ++NumDeadDbgInst;
     else
       ++NumDeadInst;
     Inst->eraseFromParent();
   }
   return {NumDeadInst, NumDeadDbgInst};
 }
 
-unsigned llvm::changeToUnreachable(Instruction *I, bool UseLLVMTrap,
+unsigned llvm::changeToUnreachable(Instruction *I,
                                    bool PreserveLCSSA, DomTreeUpdater *DTU,
                                    MemorySSAUpdater *MSSAU) {
   BasicBlock *BB = I->getParent();
 
   if (MSSAU)
     MSSAU->changeToUnreachable(I);
 
   SmallSet<BasicBlock *, 8> UniqueSuccessors;
 
   // Loop over all of the successors, removing BB's entry from any PHI
   // nodes.
   for (BasicBlock *Successor : successors(BB)) {
     Successor->removePredecessor(BB, PreserveLCSSA);
     if (DTU)
       UniqueSuccessors.insert(Successor);
   }
-  // Insert a call to llvm.trap right before this.  This turns the undefined
-  // behavior into a hard fail instead of falling through into random code.
-  if (UseLLVMTrap) {
-    Function *TrapFn =
-      Intrinsic::getDeclaration(BB->getParent()->getParent(), Intrinsic::trap);
-    CallInst *CallTrap = CallInst::Create(TrapFn, "", I);
-    CallTrap->setDebugLoc(I->getDebugLoc());
-  }
   auto *UI = new UnreachableInst(I->getContext(), I);
   UI->setDebugLoc(I->getDebugLoc());
 
   // All instructions after this are dead.
   unsigned NumInstrsRemoved = 0;
   BasicBlock::iterator BBI = I->getIterator(), BBE = BB->end();
   while (BBI != BBE) {
     if (!BBI->use_empty())
for (Instruction &I : *BB) {
           auto IntrinsicID = F->getIntrinsicID();
           // Assumptions that are known to be false are equivalent to
           // unreachable. Also, if the condition is undefined, then we make the
           // choice most beneficial to the optimizer, and choose that to also be
           // unreachable.
           if (IntrinsicID == Intrinsic::assume) {
             if (match(CI->getArgOperand(0), m_CombineOr(m_Zero(), m_Undef()))) {
               // Don't insert a call to llvm.trap right before the unreachable.
-              changeToUnreachable(CI, false, false, DTU);
+              changeToUnreachable(CI, false, DTU);
               Changed = true;
               break;
             }
           } else if (IntrinsicID == Intrinsic::experimental_guard) {
             // A call to the guard intrinsic bails out of the current
             // compilation unit if the predicate passed to it is false. If the
             // predicate is a constant false, then we know the guard will bail
             // out of the current compile unconditionally, so all code following
             // it is dead.
             //
             // Note: unlike in llvm.assume, it is not "obviously profitable" for
             // guards to treat `undef` as `false` since a guard on `undef` can
             // still be useful for widening.
             if (match(CI->getArgOperand(0), m_Zero()))
               if (!isa<UnreachableInst>(CI->getNextNode())) {
-                changeToUnreachable(CI->getNextNode(), /*UseLLVMTrap=*/false,
+                changeToUnreachable(CI->getNextNode(),
                                     false, DTU);
                 Changed = true;
                 break;
               }
           }
         } else if ((isa<ConstantPointerNull>(Callee) &&
                     !NullPointerIsDefined(CI->getFunction())) ||
                    isa<UndefValue>(Callee)) {
-          changeToUnreachable(CI, /*UseLLVMTrap=*/false, false, DTU);
+          changeToUnreachable(CI, false, DTU);
           Changed = true;
           break;
         }
         if (CI->doesNotReturn() && !CI->isMustTailCall()) {
           // If we found a call to a no-return function, insert an unreachable
           // instruction after it.  Make sure there isn't *already* one there
           // though.
           if (!isa<UnreachableInst>(CI->getNextNode())) {
             // Don't insert a call to llvm.trap right before the unreachable.
-            changeToUnreachable(CI->getNextNode(), false, false, DTU);
+            changeToUnreachable(CI->getNextNode(), false, DTU);
             Changed = true;
           }
           break;
         }
       } else if (auto *SI = dyn_cast<StoreInst>(&I)) {
         // Store to undef and store to null are undefined and used to signal
         // that they should be changed to unreachable by passes that can't
         // modify the CFG.
 
         // Don't touch volatile stores.
         if (SI->isVolatile()) continue;
 
         Value *Ptr = SI->getOperand(1);
 
         if (isa<UndefValue>(Ptr) ||
             (isa<ConstantPointerNull>(Ptr) &&
              !NullPointerIsDefined(SI->getFunction(),
                                    SI->getPointerAddressSpace()))) {
-          changeToUnreachable(SI, true, false, DTU);
+          changeToUnreachable(SI, false, DTU);
           Changed = true;
           break;
         }
       }
     }
 
     Instruction *Terminator = BB->getTerminator();
     if (auto *II = dyn_cast<InvokeInst>(Terminator)) {
       // Turn invokes that call 'nounwind' functions into ordinary calls.
       Value *Callee = II->getCalledOperand();
       if ((isa<ConstantPointerNull>(Callee) &&
            !NullPointerIsDefined(BB->getParent())) ||
           isa<UndefValue>(Callee)) {
-        changeToUnreachable(II, true, false, DTU);
+        changeToUnreachable(II, false, DTU);
         Changed = true;
       } else if (II->doesNotThrow() && canSimplifyInvokeNoUnwind(&F)) {
         if (II->use_empty() && II->onlyReadsMemory()) {
           // jump to the normal destination branch.
           BasicBlock *NormalDestBB = II->getNormalDest();
           BasicBlock *UnwindDestBB = II->getUnwindDest();
           BranchInst::Create(NormalDestBB, II);
           UnwindDestBB->removePredecessor(II->getParent());

llvm/lib/Transforms/Utils/LoopSimplify.cpp

for (Loop::block_iterator BB = L->block_begin(), E = L->block_end();
     // Delete each unique out-of-loop (and thus dead) predecessor.
     for (BasicBlock *P : BadPreds) {
 
       LLVM_DEBUG(dbgs() << "LoopSimplify: Deleting edge from dead predecessor "
                         << P->getName() << "\n");
 
       // Zap the dead pred's terminator and replace it with unreachable.
       Instruction *TI = P->getTerminator();
-      changeToUnreachable(TI, /*UseLLVMTrap=*/false, PreserveLCSSA,
+      changeToUnreachable(TI, PreserveLCSSA,
                           /*DTU=*/nullptr, MSSAU);
       Changed = true;
     }
   }
 
   if (MSSAU && VerifyMemorySSA)
     MSSAU->getMemorySSA()->verifyMemorySSA();
 

llvm/lib/Transforms/Utils/LoopUnroll.cpp

for (unsigned i = 0, e = Info.ExitingBlocks.size(); i != e; ++i) {
         continue;
 
       SetDest(Info.ExitingBlocks[i], *KnownWillExit, Info.ExitOnTrue);
     }
   }
 
   // When completely unrolling, the last latch becomes unreachable.
   if (!LatchIsExiting && CompletelyUnroll)
-    changeToUnreachable(Latches.back()->getTerminator(), /* UseTrap */ false,
+    changeToUnreachable(Latches.back()->getTerminator(),
                         PreserveLCSSA, &DTU);
 
   // Merge adjacent basic blocks, if possible.
   for (BasicBlock *Latch : Latches) {
     BranchInst *Term = dyn_cast<BranchInst>(Latch->getTerminator());
     assert((Term ||
             (CompletelyUnroll && !LatchIsExiting && Latch == Latches.back())) &&
            "Need a branch as terminator, except when fully unrolling with "

llvm/lib/Transforms/Utils/LoopUtils.cpp

void llvm::breakLoopBackedge(Loop *L, DominatorTree &DT, ScalarEvolution &SE,
 
   std::unique_ptr<MemorySSAUpdater> MSSAU;
   if (MSSA)
     MSSAU = std::make_unique<MemorySSAUpdater>(MSSA);
 
   auto *BackedgeBB = SplitEdge(Latch, Header, &DT, &LI, MSSAU.get());
 
   DomTreeUpdater DTU(&DT, DomTreeUpdater::UpdateStrategy::Eager);
-  (void)changeToUnreachable(BackedgeBB->getTerminator(), /*UseTrap*/false,
+  (void)changeToUnreachable(BackedgeBB->getTerminator(),
                             /*PreserveLCSSA*/true, &DTU, MSSAU.get());
 
   // Erase (and destroy) this loop instance.  Handles relinking sub-loops
   // and blocks within the loop as needed.
   LI.erase(L);
 
   // If the loop we broke had a parent, then changeToUnreachable might have
   // caused a block to be removed from the parent loop (see loop_nest_lcssa
   // test case in zero-btc.ll for an example), thus changing the parent's

llvm/unittests/Transforms/Utils/LocalTest.cpp

TEST(Local, ChangeToUnreachable) {
   Function &F = *cast<Function>(M->getNamedValue("foo"));
 
   BasicBlock &BB = F.front();
   Instruction &A = BB.front();
   DebugLoc DLA = A.getDebugLoc();
 
   ASSERT_TRUE(isa<ReturnInst>(&A));
   // One instruction should be affected.
-  EXPECT_EQ(changeToUnreachable(&A, /*UseLLVMTrap*/false), 1U);
+  EXPECT_EQ(changeToUnreachable(&A), 1U);
 
   Instruction &B = BB.front();
 
   // There should be an uncreachable instruction.
   ASSERT_TRUE(isa<UnreachableInst>(&B));
 
   DebugLoc DLB = B.getDebugLoc();
   EXPECT_EQ(DLA, DLB);