diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp --- a/llvm/lib/IR/BasicBlock.cpp +++ b/llvm/lib/IR/BasicBlock.cpp @@ -315,6 +315,25 @@ PHINode *APN = dyn_cast(&front()); if (!APN) return; // Quick exit. + // In the case below we can't replace use of %ptr.1 with %inc.ptr1 + // because it will break SSA form. + // phi.bb: + // %ptr.1 = phi i8* [ %inc.ptr1, %use.bb ] + // br label %use.bb + // use.bb: + // %inc.ptr1 = getelementptr inbounds i8, i8* %ptr.1, i64 1 + // br i1 %cmp, label %ret.bb, label %phi.bb + auto canReplaceAllPhiUses = [](PHINode *PN) { + assert((PN->getNumIncomingValues() == 1 || PN->hasConstantValue()) && + "PHI Node must have 1 or more than 1, but constant input value"); + Value *ValToPropagate = PN->getIncomingValue(0); + assert(ValToPropagate && "Invalid value"); + for (User *User : PN->users()) + if (ValToPropagate == User) + return false; + return true; + }; + // If there are exactly two predecessors, then we want to nuke the PHI nodes // altogether. However, we cannot do this, if this in this case: // @@ -347,7 +366,7 @@ PN->removeIncomingValue(Pred, !KeepOneInputPHIs); // If the PHI _HAD_ two inputs, replace PHI node with its now *single* value - if (AllowPhiElim && max_idx == 2) { + if (AllowPhiElim && max_idx == 2 && canReplaceAllPhiUses(PN)) { if (PN->getIncomingValue(0) != PN) PN->replaceAllUsesWith(PN->getIncomingValue(0)); else @@ -369,7 +388,8 @@ // If all incoming values to the Phi are the same, we can replace the Phi // with that value. Value* PNV = nullptr; - if (!KeepOneInputPHIs && (PNV = PN->hasConstantValue())) + if (!KeepOneInputPHIs && (PNV = PN->hasConstantValue()) && + canReplaceAllPhiUses(PN)) if (PNV != PN) { PN->replaceAllUsesWith(PNV); PN->eraseFromParent(); diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp --- a/llvm/lib/Transforms/Utils/CloneFunction.cpp +++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp @@ -685,12 +685,13 @@ ++I; continue; } - // We shouldn't be able to get single-entry PHI nodes here, as instsimplify - // above should have zapped all of them.. - assert(!isa(Dest->begin())); + // There are cases when we can't replace single-entry PHI, as it will break + // SSA form (see PR27065). We skip such blocks. + if (isa(Dest->begin())) { + ++I; continue; + } - // We know all single-entry PHI nodes in the inlined function have been - // removed, so we just need to splice the blocks. + // There are no PHI nodes in Dest, so we just need to splice the blocks. BI->eraseFromParent(); // Make all PHI nodes that referred to Dest now refer to I as their source. diff --git a/llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll b/llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll --- a/llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll +++ b/llvm/test/Transforms/Inline/clone-function-no-assert-on-phis.ll @@ -2,12 +2,10 @@ ; RUN: opt -S -inline < %s | FileCheck %s ; RUN: opt -S -passes='cgscc(inline)' < %s | FileCheck %s -; This test exposes bugs in BasicBlock::removePredecessor and -; will be fixed in further patches. -; Take a look at this line which has invalid SSA form: -; [[TMP81_I:%.*]] = getelementptr inbounds i8, i8* [[TMP81_I]], i64 1 -; It happens to work, because the code is dead and -; verifier doesn't complain. +; This test checks that no assertion happens in CloneAndPruneIntoFromInst +; from CloneFunction.cpp. +; As a result of the changes in BasicBlock::removePredecessor, +; now there might be phi nodes with single entry. target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" @@ -26,20 +24,24 @@ ; CHECK-NEXT: [[TMP_I:%.*]] = ptrtoint i8* [[TMP4]] to i64 ; CHECK-NEXT: [[TMP3_I:%.*]] = ptrtoint i8* [[TMP4]] to i64 ; CHECK-NEXT: br label [[_ZST6REMOVEIPCCET_S1_S1_RKT0__EXIT:%.*]] +; CHECK: bb71.i: +; CHECK-NEXT: [[TMP73_I:%.*]] = phi i8* [ [[TMP81_I:%.*]], [[BB83_I:%.*]] ] +; CHECK-NEXT: [[TMP74_I:%.*]] = phi i8* [ [[TMP80_I:%.*]], [[BB83_I]] ] +; CHECK-NEXT: [[TMP75_I:%.*]] = load i8, i8* [[TMP73_I]], align 1 +; CHECK-NEXT: [[TMP76_I:%.*]] = icmp eq i8 [[TMP75_I]], [[TMP84_I:%.*]] +; CHECK-NEXT: br i1 [[TMP76_I]], label [[BB79_I:%.*]], label [[BB77_I:%.*]] ; CHECK: bb77.i: -; CHECK-NEXT: store i8 [[TMP75_I:%.*]], i8* [[TMP80_I:%.*]], align 1 -; CHECK-NEXT: [[TMP78_I:%.*]] = getelementptr inbounds i8, i8* [[TMP80_I]], i64 1 -; CHECK-NEXT: br label [[BB79_I:%.*]] +; CHECK-NEXT: store i8 [[TMP75_I]], i8* [[TMP74_I]], align 1 +; CHECK-NEXT: [[TMP78_I:%.*]] = getelementptr inbounds i8, i8* [[TMP74_I]], i64 1 +; CHECK-NEXT: br label [[BB79_I]] ; CHECK: bb79.i: -; CHECK-NEXT: [[TMP80_I]] = phi i8* [ [[TMP80_I]], [[BB83_I:%.*]] ], [ [[TMP78_I]], [[BB77_I:%.*]] ] -; CHECK-NEXT: [[TMP81_I:%.*]] = getelementptr inbounds i8, i8* [[TMP81_I]], i64 1 +; CHECK-NEXT: [[TMP80_I]] = phi i8* [ [[TMP74_I]], [[BB71_I:%.*]] ], [ [[TMP78_I]], [[BB77_I]] ] +; CHECK-NEXT: [[TMP81_I]] = getelementptr inbounds i8, i8* [[TMP73_I]], i64 1 ; CHECK-NEXT: [[TMP82_I:%.*]] = icmp eq i8* [[TMP81_I]], [[TMP4]] ; CHECK-NEXT: br i1 [[TMP82_I]], label [[_ZST6REMOVEIPCCET_S1_S1_RKT0__EXIT]], label [[BB83_I]] ; CHECK: bb83.i: -; CHECK-NEXT: [[TMP84_I:%.*]] = load i8, i8* [[TMP2]], align 1 -; CHECK-NEXT: [[TMP75_I]] = load i8, i8* [[TMP81_I]], align 1 -; CHECK-NEXT: [[TMP76_I:%.*]] = icmp eq i8 [[TMP75_I]], [[TMP84_I]] -; CHECK-NEXT: br i1 [[TMP76_I]], label [[BB79_I]], label [[BB77_I]] +; CHECK-NEXT: [[TMP84_I]] = load i8, i8* [[TMP2]], align 1 +; CHECK-NEXT: br label [[BB71_I]] ; CHECK: _ZSt6removeIPccET_S1_S1_RKT0_.exit: ; CHECK-NEXT: [[TMP86_I:%.*]] = phi i8* [ [[TMP4]], [[BB:%.*]] ], [ [[TMP80_I]], [[BB79_I]] ] ; CHECK-NEXT: ret i32 0 diff --git a/llvm/unittests/IR/BasicBlockTest.cpp b/llvm/unittests/IR/BasicBlockTest.cpp --- a/llvm/unittests/IR/BasicBlockTest.cpp +++ b/llvm/unittests/IR/BasicBlockTest.cpp @@ -147,10 +147,6 @@ EXPECT_STREQ(ErrorOS.str().c_str(), ""); } -static void checkFunctionIsNotValid(const Function &F) { - EXPECT_TRUE(verifyFunction(F)); -} - TEST(BasicBlockTest, RemovePredecessorPhiOneInput) { StringRef ModuleString = R"( define i32 @f(i1 %cmp, i8 *%call) { @@ -399,7 +395,6 @@ BasicBlock *UseBB = &*FI++; { - checkFunctionIsValid(*F); EXPECT_EQ(PhiBB->size(), 2u); PHINode *PN = dyn_cast(PhiBB->begin()); ASSERT_NE(PN, nullptr) << "Couldn't get PHINode."; @@ -409,14 +404,16 @@ PhiBB->removePredecessor(EntryBB); { - checkFunctionIsNotValid(*F); - // Input value %inc.ptr1 was propagated into all it's uses. - // Now SSA form is broken. - EXPECT_EQ(PhiBB->size(), 1u); + // Input value %inc.ptr1 was NOT propagated into all it's uses. + // Otherwise, SSA form would be broken. + EXPECT_EQ(PhiBB->size(), 2u); + PHINode *PN = dyn_cast(PhiBB->begin()); + ASSERT_NE(PN, nullptr) << "Couldn't get PHINode."; + EXPECT_EQ(PN->getNumIncomingValues(), 1u); EXPECT_EQ(UseBB->size(), 2u); GetElementPtrInst *GEP = dyn_cast(UseBB->begin()); ASSERT_NE(GEP, nullptr) << "Expecting GEP as a first instruction"; - EXPECT_EQ(GEP, GEP->getPointerOperand()); + EXPECT_NE(GEP, GEP->getPointerOperand()); } } @@ -446,7 +443,6 @@ BasicBlock *UseBB = &*FI++; { - checkFunctionIsValid(*F); EXPECT_EQ(PhiBB->size(), 2u); PHINode *PN = dyn_cast(PhiBB->begin()); ASSERT_NE(PN, nullptr) << "Couldn't get PHINode."; @@ -456,14 +452,16 @@ PhiBB->removePredecessor(EntryBB); { - // Input value %inc.ptr1 was propagated into all it's uses. - // Now SSA form is broken. - checkFunctionIsNotValid(*F); - EXPECT_EQ(PhiBB->size(), 1u); + // Input value %inc.ptr1 was NOT propagated into all it's uses. + // Otherwise, SSA form would be broken. + EXPECT_EQ(PhiBB->size(), 2u); + PHINode *PN = dyn_cast(PhiBB->begin()); + ASSERT_NE(PN, nullptr) << "Couldn't get PHINode."; + EXPECT_EQ(PN->getNumIncomingValues(), 2u); EXPECT_EQ(UseBB->size(), 2u); GetElementPtrInst *GEP = dyn_cast(UseBB->begin()); ASSERT_NE(GEP, nullptr) << "Expecting GEP as a first instruction"; - EXPECT_EQ(GEP, GEP->getPointerOperand()); + EXPECT_NE(GEP, GEP->getPointerOperand()); } }