From 4ce16022d3ef27d65d53534cfd9f99ef6a487e25 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Mon, 19 Jul 2021 15:31:10 -0500 Subject: [PATCH] [Local] Do not introduce a new `llvm.trap` before `unreachable` 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. Differential Revision: https://reviews.llvm.org/D106308 --- include/llvm/Transforms/Utils/Local.h | 3 +-- lib/CodeGen/WinEHPrepare.cpp | 6 ++--- lib/Transforms/Coroutines/CoroSplit.cpp | 2 +- lib/Transforms/Coroutines/Coroutines.cpp | 2 +- lib/Transforms/IPO/Attributor.cpp | 2 +- lib/Transforms/IPO/PruneEH.cpp | 2 +- lib/Transforms/Scalar/SCCP.cpp | 6 ++--- lib/Transforms/Utils/InlineFunction.cpp | 2 +- lib/Transforms/Utils/Local.cpp | 25 ++++++------------- lib/Transforms/Utils/LoopSimplify.cpp | 2 +- lib/Transforms/Utils/LoopUnroll.cpp | 3 +-- lib/Transforms/Utils/LoopUtils.cpp | 4 +-- test/CodeGen/ARM/vmul.ll | 7 ------ test/CodeGen/Hexagon/swp-art-deps-rec.ll | 6 +++++ test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll | 2 +- test/Transforms/SimplifyCFG/invoke.ll | 6 ++--- test/Transforms/SimplifyCFG/trap-debugloc.ll | 4 +-- .../SimplifyCFG/trapping-load-unreachable.ll | 1 - unittests/Transforms/Utils/LocalTest.cpp | 2 +- 19 files changed, 35 insertions(+), 52 deletions(-) diff --git a/include/llvm/Transforms/Utils/Local.h b/include/llvm/Transforms/Utils/Local.h index 56dac033990..0102aa9ef3c 100644 --- a/include/llvm/Transforms/Utils/Local.h +++ b/include/llvm/Transforms/Utils/Local.h @@ -328,8 +328,7 @@ 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, - bool PreserveLCSSA = false, +unsigned changeToUnreachable(Instruction *I, bool PreserveLCSSA = false, DomTreeUpdater *DTU = nullptr, MemorySSAUpdater *MSSAU = nullptr); diff --git a/lib/CodeGen/WinEHPrepare.cpp b/lib/CodeGen/WinEHPrepare.cpp index 0e5d5e4d936..4564aa1c127 100644 --- a/lib/CodeGen/WinEHPrepare.cpp +++ b/lib/CodeGen/WinEHPrepare.cpp @@ -984,9 +984,9 @@ void WinEHPrepare::removeImplausibleInstructions(Function &F) { BasicBlock::iterator CallI = std::prev(BB->getTerminator()->getIterator()); auto *CI = cast(&*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), @@ -1007,7 +1007,7 @@ void WinEHPrepare::removeImplausibleInstructions(Function &F) { IsUnreachableCleanupret = CRI->getCleanupPad() != CleanupPad; if (IsUnreachableRet || IsUnreachableCatchret || IsUnreachableCleanupret) { - changeToUnreachable(TI, /*UseLLVMTrap=*/false); + changeToUnreachable(TI); } else if (isa(TI)) { if (Personality == EHPersonality::MSVC_CXX && CleanupPad) { // Invokes within a cleanuppad for the MSVC++ personality never diff --git a/lib/Transforms/Coroutines/CoroSplit.cpp b/lib/Transforms/Coroutines/CoroSplit.cpp index c73bc5df63f..b6932dbbfc3 100644 --- a/lib/Transforms/Coroutines/CoroSplit.cpp +++ b/lib/Transforms/Coroutines/CoroSplit.cpp @@ -955,7 +955,7 @@ void CoroCloner::create() { 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 diff --git a/lib/Transforms/Coroutines/Coroutines.cpp b/lib/Transforms/Coroutines/Coroutines.cpp index fde758ca6db..ae2d9e192c8 100644 --- a/lib/Transforms/Coroutines/Coroutines.cpp +++ b/lib/Transforms/Coroutines/Coroutines.cpp @@ -361,7 +361,7 @@ void coro::Shape::buildFrom(Function &F) { // Replace all coro.ends with unreachable instruction. for (AnyCoroEndInst *CE : CoroEnds) - changeToUnreachable(CE, /*UseLLVMTrap=*/false); + changeToUnreachable(CE); return; } diff --git a/lib/Transforms/IPO/Attributor.cpp b/lib/Transforms/IPO/Attributor.cpp index 4155b0d6f93..a976032f925 100644 --- a/lib/Transforms/IPO/Attributor.cpp +++ b/lib/Transforms/IPO/Attributor.cpp @@ -1705,7 +1705,7 @@ ChangeStatus Attributor::cleanupIR() { if (!isRunOn(*I->getFunction())) continue; CGModifiedFunctions.insert(I->getFunction()); - changeToUnreachable(I, /* UseLLVMTrap */ false); + changeToUnreachable(I); } for (auto &V : ToBeDeletedInsts) { diff --git a/lib/Transforms/IPO/PruneEH.cpp b/lib/Transforms/IPO/PruneEH.cpp index 3f3b18771cd..39de19ca9e9 100644 --- a/lib/Transforms/IPO/PruneEH.cpp +++ b/lib/Transforms/IPO/PruneEH.cpp @@ -251,7 +251,7 @@ static void DeleteBasicBlock(BasicBlock *BB, CallGraphUpdater &CGU) { if (TokenInst) { if (!TokenInst->isTerminator()) - changeToUnreachable(TokenInst->getNextNode(), /*UseLLVMTrap=*/false); + changeToUnreachable(TokenInst->getNextNode()); } else { // Get the list of successors of this block. std::vector Succs(succ_begin(BB), succ_end(BB)); diff --git a/lib/Transforms/Scalar/SCCP.cpp b/lib/Transforms/Scalar/SCCP.cpp index 3840e2f37f1..b09f896d015 100644 --- a/lib/Transforms/Scalar/SCCP.cpp +++ b/lib/Transforms/Scalar/SCCP.cpp @@ -526,13 +526,11 @@ bool llvm::runIPSCCP( // 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, - /*PreserveLCSSA=*/false, &DTU); + NumInstRemoved += 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) diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp index 9363e0dc9b2..792aa8208f2 100644 --- a/lib/Transforms/Utils/InlineFunction.cpp +++ b/lib/Transforms/Utils/InlineFunction.cpp @@ -2327,7 +2327,7 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI, // As such, we replace the cleanupret with unreachable. if (auto *CleanupRet = dyn_cast(BB->getTerminator())) if (CleanupRet->unwindsToCaller() && EHPadForCallUnwindsLocally) - changeToUnreachable(CleanupRet, /*UseLLVMTrap=*/false); + changeToUnreachable(CleanupRet); Instruction *I = BB->getFirstNonPHI(); if (!I->isEHPad()) diff --git a/lib/Transforms/Utils/Local.cpp b/lib/Transforms/Utils/Local.cpp index d95c053c25a..e36d108b747 100644 --- a/lib/Transforms/Utils/Local.cpp +++ b/lib/Transforms/Utils/Local.cpp @@ -2110,8 +2110,8 @@ llvm::removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB) { return {NumDeadInst, NumDeadDbgInst}; } -unsigned llvm::changeToUnreachable(Instruction *I, bool UseLLVMTrap, - bool PreserveLCSSA, DomTreeUpdater *DTU, +unsigned llvm::changeToUnreachable(Instruction *I, bool PreserveLCSSA, + DomTreeUpdater *DTU, MemorySSAUpdater *MSSAU) { BasicBlock *BB = I->getParent(); @@ -2127,14 +2127,6 @@ unsigned llvm::changeToUnreachable(Instruction *I, bool UseLLVMTrap, 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()); @@ -2271,7 +2263,7 @@ static bool markAliveBlocks(Function &F, 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; } @@ -2287,8 +2279,7 @@ static bool markAliveBlocks(Function &F, // still be useful for widening. if (match(CI->getArgOperand(0), m_Zero())) if (!isa(CI->getNextNode())) { - changeToUnreachable(CI->getNextNode(), /*UseLLVMTrap=*/false, - false, DTU); + changeToUnreachable(CI->getNextNode(), false, DTU); Changed = true; break; } @@ -2296,7 +2287,7 @@ static bool markAliveBlocks(Function &F, } else if ((isa(Callee) && !NullPointerIsDefined(CI->getFunction())) || isa(Callee)) { - changeToUnreachable(CI, /*UseLLVMTrap=*/false, false, DTU); + changeToUnreachable(CI, false, DTU); Changed = true; break; } @@ -2306,7 +2297,7 @@ static bool markAliveBlocks(Function &F, // though. if (!isa(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; @@ -2325,7 +2316,7 @@ static bool markAliveBlocks(Function &F, (isa(Ptr) && !NullPointerIsDefined(SI->getFunction(), SI->getPointerAddressSpace()))) { - changeToUnreachable(SI, true, false, DTU); + changeToUnreachable(SI, false, DTU); Changed = true; break; } @@ -2339,7 +2330,7 @@ static bool markAliveBlocks(Function &F, if ((isa(Callee) && !NullPointerIsDefined(BB->getParent())) || isa(Callee)) { - changeToUnreachable(II, true, false, DTU); + changeToUnreachable(II, false, DTU); Changed = true; } else if (II->doesNotThrow() && canSimplifyInvokeNoUnwind(&F)) { if (II->use_empty() && II->onlyReadsMemory()) { diff --git a/lib/Transforms/Utils/LoopSimplify.cpp b/lib/Transforms/Utils/LoopSimplify.cpp index 260ada85a1f..d2fd32c98d7 100644 --- a/lib/Transforms/Utils/LoopSimplify.cpp +++ b/lib/Transforms/Utils/LoopSimplify.cpp @@ -513,7 +513,7 @@ ReprocessLoop: // 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; } diff --git a/lib/Transforms/Utils/LoopUnroll.cpp b/lib/Transforms/Utils/LoopUnroll.cpp index 78b3c77607f..a91bf7b7af1 100644 --- a/lib/Transforms/Utils/LoopUnroll.cpp +++ b/lib/Transforms/Utils/LoopUnroll.cpp @@ -739,8 +739,7 @@ LoopUnrollResult llvm::UnrollLoop(Loop *L, UnrollLoopOptions ULO, LoopInfo *LI, // When completely unrolling, the last latch becomes unreachable. if (!LatchIsExiting && CompletelyUnroll) - changeToUnreachable(Latches.back()->getTerminator(), /* UseTrap */ false, - PreserveLCSSA, &DTU); + changeToUnreachable(Latches.back()->getTerminator(), PreserveLCSSA, &DTU); // Merge adjacent basic blocks, if possible. for (BasicBlock *Latch : Latches) { diff --git a/lib/Transforms/Utils/LoopUtils.cpp b/lib/Transforms/Utils/LoopUtils.cpp index 15a87dd6a40..e4d78f9ada0 100644 --- a/lib/Transforms/Utils/LoopUtils.cpp +++ b/lib/Transforms/Utils/LoopUtils.cpp @@ -723,8 +723,8 @@ void llvm::breakLoopBackedge(Loop *L, DominatorTree &DT, ScalarEvolution &SE, auto *BackedgeBB = SplitEdge(Latch, Header, &DT, &LI, MSSAU.get()); DomTreeUpdater DTU(&DT, DomTreeUpdater::UpdateStrategy::Eager); - (void)changeToUnreachable(BackedgeBB->getTerminator(), /*UseTrap*/false, - /*PreserveLCSSA*/true, &DTU, MSSAU.get()); + (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. diff --git a/test/CodeGen/ARM/vmul.ll b/test/CodeGen/ARM/vmul.ll index 4f7464c7e20..c1a63d7df52 100644 --- a/test/CodeGen/ARM/vmul.ll +++ b/test/CodeGen/ARM/vmul.ll @@ -731,11 +731,6 @@ define i16 @vmullWithInconsistentExtensions(<8 x i8> %vec) { define void @vmull_buildvector() nounwind optsize ssp align 2 { ; CHECK-LABEL: vmull_buildvector: ; CHECK: @ %bb.0: @ %entry -; CHECK-NEXT: mov r0, #0 -; CHECK-NEXT: cmp r0, #0 -; CHECK-NEXT: bxne lr -; CHECK-NEXT: .LBB55_1: @ %for.body33.lr.ph -; CHECK-NEXT: .inst 0xe7ffdefe entry: br i1 undef, label %for.end179, label %for.body.lr.ph @@ -812,7 +807,6 @@ declare <8 x i8> @llvm.arm.neon.vqmovnu.v8i8(<8 x i16>) nounwind readnone define void @no_illegal_types_vmull_sext(<4 x i32> %a) { ; CHECK-LABEL: no_illegal_types_vmull_sext: ; CHECK: @ %bb.0: @ %entry -; CHECK-NEXT: .inst 0xe7ffdefe entry: %wide.load283.i = load <4 x i8>, <4 x i8>* undef, align 1 %0 = sext <4 x i8> %wide.load283.i to <4 x i32> @@ -825,7 +819,6 @@ entry: define void @no_illegal_types_vmull_zext(<4 x i32> %a) { ; CHECK-LABEL: no_illegal_types_vmull_zext: ; CHECK: @ %bb.0: @ %entry -; CHECK-NEXT: .inst 0xe7ffdefe entry: %wide.load283.i = load <4 x i8>, <4 x i8>* undef, align 1 %0 = zext <4 x i8> %wide.load283.i to <4 x i32> diff --git a/test/CodeGen/Hexagon/swp-art-deps-rec.ll b/test/CodeGen/Hexagon/swp-art-deps-rec.ll index f89df6e5573..36ee3da4a6f 100644 --- a/test/CodeGen/Hexagon/swp-art-deps-rec.ll +++ b/test/CodeGen/Hexagon/swp-art-deps-rec.ll @@ -3,6 +3,12 @@ ; RUN: llc -march=hexagon -mcpu=hexagonv65 -O3 -debug-only=pipeliner \ ; RUN: < %s 2>&1 -pipeliner-experimental-cg=true | FileCheck %s +; As part of https://reviews.llvm.org/D106308 this test broke. +; It is not surprising as the tts is full of UB and run with O3. +; FIXME: It is unclear what to do with this test now, replacing null/undef +; with pointer arguments could be a way to go. +; XFAIL: * + ; Test that the artificial dependences are ignored while computing the ; circuits. diff --git a/test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll b/test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll index 116084b276f..0230edfce7c 100644 --- a/test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll +++ b/test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll @@ -1,4 +1,4 @@ -; RUN: llc -O2 -o - %s | FileCheck %s +; RUN: llc -O2 -arm-atomic-cfg-tidy=0 -o - %s | FileCheck %s target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64" target triple = "thumbv8-unknown-linux-gnueabihf" diff --git a/test/Transforms/SimplifyCFG/invoke.ll b/test/Transforms/SimplifyCFG/invoke.ll index a287c0b2e30..48f3d90520b 100644 --- a/test/Transforms/SimplifyCFG/invoke.ll +++ b/test/Transforms/SimplifyCFG/invoke.ll @@ -13,7 +13,6 @@ declare i32 @fn() define i8* @f1() nounwind uwtable ssp personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) { ; CHECK-LABEL: @f1( ; CHECK-NEXT: entry: -; CHECK-NEXT: call void @llvm.trap() ; CHECK-NEXT: unreachable ; entry: @@ -34,7 +33,6 @@ lpad: define i8* @f2() nounwind uwtable ssp personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) { ; CHECK-LABEL: @f2( ; CHECK-NEXT: entry: -; CHECK-NEXT: call void @llvm.trap() ; CHECK-NEXT: unreachable ; entry: @@ -63,7 +61,7 @@ define i8* @f2_no_null_opt() nounwind uwtable ssp #0 personality i8* bitcast (i3 ; CHECK-NEXT: [[TMP0:%.*]] = landingpad { i8*, i32 } ; CHECK-NEXT: filter [0 x i8*] zeroinitializer ; CHECK-NEXT: [[TMP1:%.*]] = extractvalue { i8*, i32 } [[TMP0]], 0 -; CHECK-NEXT: tail call void @__cxa_call_unexpected(i8* [[TMP1]]) #[[ATTR6:[0-9]+]] +; CHECK-NEXT: tail call void @__cxa_call_unexpected(i8* [[TMP1]]) #[[ATTR5:[0-9]+]] ; CHECK-NEXT: unreachable ; entry: @@ -138,7 +136,7 @@ define i32 @f5(i1 %cond, i8* %a, i8* %b) personality i8* bitcast (i32 (...)* @__ ; CHECK: lpad: ; CHECK-NEXT: [[TMP0:%.*]] = landingpad { i8*, i32 } ; CHECK-NEXT: filter [0 x i8*] zeroinitializer -; CHECK-NEXT: tail call void @__cxa_call_unexpected(i8* [[A:%.*]]) #[[ATTR6]] +; CHECK-NEXT: tail call void @__cxa_call_unexpected(i8* [[A:%.*]]) #[[ATTR5]] ; CHECK-NEXT: unreachable ; entry: diff --git a/test/Transforms/SimplifyCFG/trap-debugloc.ll b/test/Transforms/SimplifyCFG/trap-debugloc.ll index f17183d41da..2a3b896eefa 100644 --- a/test/Transforms/SimplifyCFG/trap-debugloc.ll +++ b/test/Transforms/SimplifyCFG/trap-debugloc.ll @@ -1,8 +1,8 @@ ; RUN: opt -S -simplifycfg -simplifycfg-require-and-preserve-domtree=1 < %s | FileCheck %s ; Radar 9342286 -; Assign DebugLoc to trap instruction. +; Assign DebugLoc to unreachable instruction. define void @foo() nounwind ssp !dbg !0 { -; CHECK: call void @llvm.trap(), !dbg +; CHECK: unreachable, !dbg store i32 42, i32* null, !dbg !5 ret void, !dbg !7 } diff --git a/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll b/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll index e437f40cbe7..7f110c1cfb9 100644 --- a/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll +++ b/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll @@ -52,7 +52,6 @@ return: ; preds = %entry define void @test2() nounwind { ; CHECK-LABEL: @test2( ; CHECK-NEXT: entry: -; CHECK-NEXT: call void @llvm.trap() ; CHECK-NEXT: unreachable ; entry: diff --git a/unittests/Transforms/Utils/LocalTest.cpp b/unittests/Transforms/Utils/LocalTest.cpp index 720472dc24d..be9d9e52da1 100644 --- a/unittests/Transforms/Utils/LocalTest.cpp +++ b/unittests/Transforms/Utils/LocalTest.cpp @@ -623,7 +623,7 @@ TEST(Local, ChangeToUnreachable) { ASSERT_TRUE(isa(&A)); // One instruction should be affected. - EXPECT_EQ(changeToUnreachable(&A, /*UseLLVMTrap*/false), 1U); + EXPECT_EQ(changeToUnreachable(&A), 1U); Instruction &B = BB.front();