From 45a291b5f609fc7edd8c526772e491d68b210dbe Mon Sep 17 00:00:00 2001 From: Nick Desaulniers Date: Thu, 16 Feb 2023 17:44:02 -0800 Subject: [PATCH] [Dominators] check indirect branches of callbr This will be necessary to support outputs from asm goto along indirect edges. Test via: $ pushd llvm/build; ninja IRTests; popd $ ./llvm/build/unittests/IR/IRTests \ --gtest_filter=DominatorTree.CallBrDomination Also, return nullptr in Instruction::getInsertionPointAfterDef for CallBrInst as was recommened in https://reviews.llvm.org/D135997#3991427. The following phab review was folded into this commit: https://reviews.llvm.org/D140166 Link: Link: https://discourse.llvm.org/t/rfc-syncing-asm-goto-with-outputs-with-gcc/65453/8 Reviewed By: void, efriedma, ChuanqiXu, MaskRay Differential Revision: https://reviews.llvm.org/D135997 --- llvm/include/llvm/IR/Dominators.h | 2 +- llvm/lib/IR/Dominators.cpp | 14 ------- llvm/lib/IR/Instruction.cpp | 5 ++- .../Transforms/InstCombine/InstCombineCalls.cpp | 13 +++---- llvm/test/Transforms/Coroutines/coro-debug.ll | 3 +- llvm/test/Transforms/InstCombine/freeze.ll | 2 +- llvm/test/Transforms/Reassociate/callbr.ll | 6 ++- llvm/test/Verifier/callbr.ll | 5 +-- llvm/test/Verifier/dominates.ll | 5 +-- llvm/unittests/IR/DominatorTreeTest.cpp | 43 ++++++++++++++++++++++ 10 files changed, 63 insertions(+), 35 deletions(-) diff --git a/llvm/include/llvm/IR/Dominators.h b/llvm/include/llvm/IR/Dominators.h index c2d080b..6ceadbf 100644 --- a/llvm/include/llvm/IR/Dominators.h +++ b/llvm/include/llvm/IR/Dominators.h @@ -191,7 +191,7 @@ class DominatorTree : public DominatorTreeBase { /// * Non-instruction Defs dominate everything. /// * Def does not dominate a use in Def itself (outside of degenerate cases /// like unreachable code or trivial phi cycles). - /// * Invoke/callbr Defs only dominate uses in their default destination. + /// * Invoke Defs only dominate uses in their default destination. bool dominates(const Value *Def, const Use &U) const; /// Return true if value Def dominates all possible uses inside instruction /// User. Same comments as for the Use-based API apply. diff --git a/llvm/lib/IR/Dominators.cpp b/llvm/lib/IR/Dominators.cpp index 7c620c3..24cc9f46f 100644 --- a/llvm/lib/IR/Dominators.cpp +++ b/llvm/lib/IR/Dominators.cpp @@ -194,13 +194,6 @@ bool DominatorTree::dominates(const Instruction *Def, return dominates(E, UseBB); } - // Callbr results are similarly only usable in the default destination. - if (const auto *CBI = dyn_cast(Def)) { - BasicBlock *NormalDest = CBI->getDefaultDest(); - BasicBlockEdge E(DefBB, NormalDest); - return dominates(E, UseBB); - } - return dominates(DefBB, UseBB); } @@ -311,13 +304,6 @@ bool DominatorTree::dominates(const Value *DefV, const Use &U) const { return dominates(E, U); } - // Callbr results are similarly only usable in the default destination. - if (const auto *CBI = dyn_cast(Def)) { - BasicBlock *NormalDest = CBI->getDefaultDest(); - BasicBlockEdge E(DefBB, NormalDest); - return dominates(E, U); - } - // If the def and use are in different blocks, do a simple CFG dominator // tree query. if (DefBB != UseBB) diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp index 9c88ca1..fc4e5d4 100644 --- a/llvm/lib/IR/Instruction.cpp +++ b/llvm/lib/IR/Instruction.cpp @@ -139,8 +139,9 @@ Instruction *Instruction::getInsertionPointAfterDef() { InsertBB = II->getNormalDest(); InsertPt = InsertBB->getFirstInsertionPt(); } else if (auto *CB = dyn_cast(this)) { - InsertBB = CB->getDefaultDest(); - InsertPt = InsertBB->getFirstInsertionPt(); + // Def is available in multiple successors, there's no single dominating + // insertion point. + return nullptr; } else { assert(!isTerminator() && "Only invoke/callbr terminators return value"); InsertBB = getParent(); diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp index bc714e4..f6ae674 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp @@ -3477,13 +3477,17 @@ Instruction *InstCombinerImpl::visitCallBase(CallBase &Call) { } /// If the callee is a constexpr cast of a function, attempt to move the cast to -/// the arguments of the call/callbr/invoke. +/// the arguments of the call/invoke. +/// CallBrInst is not supported. bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) { auto *Callee = dyn_cast(Call.getCalledOperand()->stripPointerCasts()); if (!Callee) return false; + assert(!isa(Call) && + "CallBr's don't have a single point after a def to insert at"); + // If this is a call to a thunk function, don't remove the cast. Thunks are // used to transparently forward all incoming parameters and outgoing return // values, so it's important to leave the cast in place. @@ -3529,7 +3533,7 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) { return false; // Attribute not compatible with transformed value. } - // If the callbase is an invoke/callbr instruction, and the return value is + // If the callbase is an invoke instruction, and the return value is // used by a PHI node in a successor, we cannot change the return type of // the call because there is no place to put the cast instruction (without // breaking the critical edge). Bail out in this case. @@ -3537,8 +3541,6 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) { BasicBlock *PhisNotSupportedBlock = nullptr; if (auto *II = dyn_cast(Caller)) PhisNotSupportedBlock = II->getNormalDest(); - if (auto *CB = dyn_cast(Caller)) - PhisNotSupportedBlock = CB->getDefaultDest(); if (PhisNotSupportedBlock) for (User *U : Caller->users()) if (PHINode *PN = dyn_cast(U)) @@ -3722,9 +3724,6 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) { if (InvokeInst *II = dyn_cast(Caller)) { NewCall = Builder.CreateInvoke(Callee, II->getNormalDest(), II->getUnwindDest(), Args, OpBundles); - } else if (CallBrInst *CBI = dyn_cast(Caller)) { - NewCall = Builder.CreateCallBr(Callee, CBI->getDefaultDest(), - CBI->getIndirectDests(), Args, OpBundles); } else { NewCall = Builder.CreateCall(Callee, Args, OpBundles); cast(NewCall)->setTailCallKind( diff --git a/llvm/test/Transforms/Coroutines/coro-debug.ll b/llvm/test/Transforms/Coroutines/coro-debug.ll index fc3ade5..2829694 100644 --- a/llvm/test/Transforms/Coroutines/coro-debug.ll +++ b/llvm/test/Transforms/Coroutines/coro-debug.ll @@ -193,7 +193,8 @@ attributes #7 = { noduplicate } ; CHECK: %[[CALLBR_RES:.+]] = callbr i32 asm ; CHECK-NEXT: to label %[[DEFAULT_DEST:.+]] [label ; CHECK: [[DEFAULT_DEST]]: -; CHECK-NEXT: call void @llvm.dbg.declare(metadata i32 %[[CALLBR_RES]] +; CHECK-NOT: {{.*}}: +; CHECK: call void @llvm.dbg.declare(metadata i32 %[[CALLBR_RES]] ; CHECK: define internal fastcc void @f.destroy(%f.Frame* noundef nonnull align 8 dereferenceable(40) %FramePtr) #0 personality i32 0 !dbg ![[DESTROY:[0-9]+]] ; CHECK: define internal fastcc void @f.cleanup(%f.Frame* noundef nonnull align 8 dereferenceable(40) %FramePtr) #0 personality i32 0 !dbg ![[CLEANUP:[0-9]+]] diff --git a/llvm/test/Transforms/InstCombine/freeze.ll b/llvm/test/Transforms/InstCombine/freeze.ll index 7bbc32f..d3274ef 100644 --- a/llvm/test/Transforms/InstCombine/freeze.ll +++ b/llvm/test/Transforms/InstCombine/freeze.ll @@ -453,9 +453,9 @@ define i32 @freeze_callbr_use_after_phi(i1 %c) { ; CHECK-NEXT: to label [[CALLBR_CONT:%.*]] [] ; CHECK: callbr.cont: ; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ [[X]], [[ENTRY:%.*]] ], [ 0, [[CALLBR_CONT]] ] +; CHECK-NEXT: call void @use_i32(i32 [[X]]) ; CHECK-NEXT: [[FR:%.*]] = freeze i32 [[X]] ; CHECK-NEXT: call void @use_i32(i32 [[FR]]) -; CHECK-NEXT: call void @use_i32(i32 [[FR]]) ; CHECK-NEXT: call void @use_i32(i32 [[PHI]]) ; CHECK-NEXT: br label [[CALLBR_CONT]] ; diff --git a/llvm/test/Transforms/Reassociate/callbr.ll b/llvm/test/Transforms/Reassociate/callbr.ll index 8b7be30..4b2323f 100644 --- a/llvm/test/Transforms/Reassociate/callbr.ll +++ b/llvm/test/Transforms/Reassociate/callbr.ll @@ -6,8 +6,10 @@ define i32 @test(i1 %b) { ; CHECK-NEXT: [[RES:%.*]] = callbr i32 asm "", "=r,!i"() ; CHECK-NEXT: to label [[NORMAL:%.*]] [label %abnormal] ; CHECK: normal: -; CHECK-NEXT: [[FACTOR:%.*]] = mul i32 [[RES]], -2 -; CHECK-NEXT: [[SUB2:%.*]] = add i32 [[FACTOR]], 5 +; CHECK-NEXT: [[RES_NEG:%.*]] = sub i32 0, [[RES]] +; CHECK-NEXT: [[SUB1:%.*]] = add i32 [[RES_NEG]], 5 +; CHECK-NEXT: [[RES_NEG1:%.*]] = sub i32 0, [[RES]] +; CHECK-NEXT: [[SUB2:%.*]] = add i32 [[SUB1]], [[RES_NEG1]] ; CHECK-NEXT: ret i32 [[SUB2]] ; CHECK: abnormal: ; CHECK-NEXT: ret i32 0 diff --git a/llvm/test/Verifier/callbr.ll b/llvm/test/Verifier/callbr.ll index 2a347d2..a77b6fe 100644 --- a/llvm/test/Verifier/callbr.ll +++ b/llvm/test/Verifier/callbr.ll @@ -56,9 +56,8 @@ define void @callbr_without_label_constraint() { ret void } -;; Ensure you cannot use the return value of a callbr in indirect targets. -; CHECK: Instruction does not dominate all uses! -; CHECK-NEXT: #test4 +;; Ensure you can use the return value of a callbr in indirect targets. +;; No issue! define i32 @test4(i1 %var) { entry: %ret = callbr i32 asm sideeffect "#test4", "=r,!i"() to label %normal [label %abnormal] diff --git a/llvm/test/Verifier/dominates.ll b/llvm/test/Verifier/dominates.ll index d3697fd..9d40890 100644 --- a/llvm/test/Verifier/dominates.ll +++ b/llvm/test/Verifier/dominates.ll @@ -69,6 +69,7 @@ next: ; CHECK-NEXT: %x = phi i32 [ %y, %entry ] } +;; No issue! define i32 @f6(i32 %x) { bb0: %y1 = callbr i32 asm "", "=r,!i"() to label %bb1 [label %bb2] @@ -76,8 +77,4 @@ bb1: ret i32 0 bb2: ret i32 %y1 -; CHECK: Instruction does not dominate all uses! -; CHECK-NEXT: %y1 = callbr i32 asm "", "=r,!i"() -; CHECK-NEXT: to label %bb1 [label %bb2] -; CHECK-NEXT: ret i32 %y1 } diff --git a/llvm/unittests/IR/DominatorTreeTest.cpp b/llvm/unittests/IR/DominatorTreeTest.cpp index 6260ce2..44bde74 100644 --- a/llvm/unittests/IR/DominatorTreeTest.cpp +++ b/llvm/unittests/IR/DominatorTreeTest.cpp @@ -1100,3 +1100,46 @@ TEST(DominatorTree, ValueDomination) { EXPECT_TRUE(DT->dominates(C, U)); }); } +TEST(DominatorTree, CallBrDomination) { + StringRef ModuleString = R"( +define void @x() { + %y = alloca i32 + %w = callbr i32 asm "", "=r,!i"() + to label %asm.fallthrough [label %z] + +asm.fallthrough: + br label %cleanup + +z: + store i32 %w, ptr %y + br label %cleanup + +cleanup: + ret void +})"; + + LLVMContext Context; + std::unique_ptr M = makeLLVMModule(Context, ModuleString); + + runWithDomTree( + *M, "x", [&](Function &F, DominatorTree *DT, PostDominatorTree *PDT) { + Function::iterator FI = F.begin(); + + BasicBlock *Entry = &*FI++; + BasicBlock *ASMFallthrough = &*FI++; + BasicBlock *Z = &*FI++; + + EXPECT_TRUE(DT->dominates(Entry, ASMFallthrough)); + EXPECT_TRUE(DT->dominates(Entry, Z)); + + BasicBlock::iterator BBI = Entry->begin(); + ++BBI; + Instruction &I = *BBI; + EXPECT_TRUE(isa(I)); + EXPECT_TRUE(isa(I)); + for (const User *U : I.users()) { + EXPECT_TRUE(isa(U)); + EXPECT_TRUE(DT->dominates(cast(&I), cast(U))); + } + }); +} -- 2.7.4