[Bitcode] Fix incomplete deduplication of PHI entries #162860
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
Since this is my first contribution I'll tag you @nikic, since you had a look at the original fix. |
|
@llvm/pr-subscribers-llvm-ir Author: Florian Stamer (FlStamerGS) ChangesFixes a verification error that occurs when a basic block contains multiple PHI nodes with duplicate entries, where some use constexprs in them. In #141560 a fix was introduced that fails in this case of multiple PHI nodes, as this will only deduplicate the entries for the PHI nodes that use constexprs, but not for those that e.g. only use a constant. Full diff: https://github.com/llvm/llvm-project/pull/162860.diff 6 Files Affected:
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index 533808e0666d5..dfd44d2243bfb 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -703,6 +703,10 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
/// block \p New instead of to it.
LLVM_ABI void replaceSuccessorsPhiUsesWith(BasicBlock *New);
+ /// Update all phi nodes in this basic block by deduplicating the references
+ /// to \p BB.
+ LLVM_ABI void deduplicatePhiUsesOf(BasicBlock *BB);
+
/// Return true if this basic block is an exception handling block.
bool isEHPad() const { return getFirstNonPHIIt()->isEHPad(); }
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index aaee1f0a7687c..88422aa0c7a8c 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -6105,18 +6105,14 @@ Error BitcodeReader::parseFunctionBody(Function *F) {
// seen value here, to avoid expanding a constant expression multiple
// times.
auto It = Args.find(BB);
- BasicBlock *EdgeBB = ConstExprEdgeBBs.lookup({BB, CurBB});
if (It != Args.end()) {
- // If this predecessor was also replaced with a constexpr basic
- // block, it must be de-duplicated.
- if (!EdgeBB) {
- PN->addIncoming(It->second, BB);
- }
+ PN->addIncoming(It->second, BB);
continue;
}
// If there already is a block for this edge (from a different phi),
// use it.
+ BasicBlock *EdgeBB = ConstExprEdgeBBs.lookup({BB, CurBB});
if (!EdgeBB) {
// Otherwise, use a temporary block (that we will discard if it
// turns out to be unnecessary).
@@ -6944,6 +6940,7 @@ Error BitcodeReader::parseFunctionBody(Function *F) {
BranchInst::Create(To, EdgeBB);
From->getTerminator()->replaceSuccessorWith(To, EdgeBB);
To->replacePhiUsesWith(From, EdgeBB);
+ To->deduplicatePhiUsesOf(EdgeBB);
EdgeBB->moveBefore(To);
}
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 3642e935397cb..49b09bd959335 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -658,6 +658,26 @@ void BasicBlock::replaceSuccessorsPhiUsesWith(BasicBlock *New) {
this->replaceSuccessorsPhiUsesWith(this, New);
}
+void BasicBlock::deduplicatePhiUsesOf(BasicBlock *BB) {
+ // N.B. This might not be a complete BasicBlock, so don't assume
+ // that it ends with a non-phi instruction.
+ for (Instruction &I : *this) {
+ PHINode *PN = dyn_cast<PHINode>(&I);
+ if (!PN)
+ break;
+ // Since the order of basic blocks in a PHINode are sorted we can
+ // use the index of \p BB + 1 as the first index to check for duplicates.
+ unsigned Idx = PN->getBasicBlockIndex(BB) + 1;
+ while (Idx < PN->getNumIncomingValues()) {
+ BasicBlock *DuplicateBB = PN->getIncomingBlock(Idx);
+ if (DuplicateBB != BB) {
+ break;
+ }
+ PN->removeIncomingValue(Idx);
+ }
+ }
+}
+
bool BasicBlock::isLandingPad() const {
return isa<LandingPadInst>(getFirstNonPHIIt());
}
diff --git a/llvm/test/Bitcode/constexpr-to-instr-dups.ll b/llvm/test/Bitcode/constexpr-to-instr-dups.ll
index dee9490b616fa..d439a6dcf0c7b 100644
--- a/llvm/test/Bitcode/constexpr-to-instr-dups.ll
+++ b/llvm/test/Bitcode/constexpr-to-instr-dups.ll
@@ -28,3 +28,30 @@ cont:
[1, %nonconst]
ret i32 %res
}
+
+define i32 @test_multiple_phis(i32 %arg) {
+entry:
+ switch i32 %arg, label %cont [
+ i32 1, label %cont
+ i32 2, label %nonconst
+ ]
+
+nonconst: ; preds = %entry
+ %cmp = icmp ne i32 %arg, 2
+ br i1 %cmp, label %cont, label %cont
+
+; CHECK-LABEL: phi.constexpr:
+; CHECK-NEXT: %constexpr = ptrtoint ptr @foo to i32
+; CHECK-NEXT: %constexpr1 = or i32 %constexpr, 5
+; CHECK-NEXT: br label %cont
+
+
+; CHECK-LABEL: cont:
+; CHECK-NEXT: %phi1 = phi i32 [ 0, %phi.constexpr ], [ 1, %nonconst ], [ 1, %nonconst ]
+; CHECK-NEXT: %phi2 = phi i32 [ %constexpr1, %phi.constexpr ], [ 1, %nonconst ], [ 1, %nonconst ]
+; CHECK-NEXT: ret i32 %phi2
+cont: ; preds = %nonconst, %nonconst, %entry, %entry
+ %phi1 = phi i32 [ 0, %entry ], [ 0, %entry ], [ 1, %nonconst ], [ 1, %nonconst ]
+ %phi2 = phi i32 [ or (i32 ptrtoint (ptr @foo to i32), i32 5), %entry ], [ or (i32 ptrtoint (ptr @foo to i32), i32 5), %entry ], [ 1, %nonconst ], [ 1, %nonconst ]
+ ret i32 %phi2
+}
diff --git a/llvm/test/Bitcode/constexpr-to-instr-dups.ll.bc b/llvm/test/Bitcode/constexpr-to-instr-dups.ll.bc
index 7897f51322fcc..15a3eeb449d87 100644
Binary files a/llvm/test/Bitcode/constexpr-to-instr-dups.ll.bc and b/llvm/test/Bitcode/constexpr-to-instr-dups.ll.bc differ
diff --git a/llvm/unittests/IR/BasicBlockTest.cpp b/llvm/unittests/IR/BasicBlockTest.cpp
index 1f726dbfe2325..4e7277a5fc84c 100644
--- a/llvm/unittests/IR/BasicBlockTest.cpp
+++ b/llvm/unittests/IR/BasicBlockTest.cpp
@@ -94,6 +94,64 @@ TEST(BasicBlockTest, PhiRange) {
}
}
+TEST(BasicBlockTest, DeduplicatePhiEntries) {
+ LLVMContext Context;
+
+ // Create the main block.
+ std::unique_ptr<BasicBlock> BB(BasicBlock::Create(Context));
+
+ // Create some predecessors of it.
+ std::unique_ptr<BasicBlock> BB1(BasicBlock::Create(Context));
+ BranchInst::Create(BB.get(), BB1.get());
+
+ // Make sure this doesn't crash if there are no phis.
+ int PhiCount = 0;
+ for (auto &PN : BB->phis()) {
+ (void)PN;
+ PhiCount++;
+ }
+ ASSERT_EQ(PhiCount, 0) << "empty block should have no phis";
+
+ // Make it a cycle.
+ auto *BI = BranchInst::Create(BB.get(), BB.get());
+
+ // Now insert some PHI nodes.
+ auto *Int32Ty = Type::getInt32Ty(Context);
+ auto *P1 = PHINode::Create(Int32Ty, /*NumReservedValues*/ 3, "phi.1", BI);
+ auto *P2 = PHINode::Create(Int32Ty, /*NumReservedValues*/ 3, "phi.2", BI);
+
+ // Some non-PHI nodes.
+ auto *Sum = BinaryOperator::CreateAdd(P1, P2, "sum", BI);
+
+ // Now wire up the incoming values that are interesting.
+ P1->addIncoming(P2, BB.get());
+ P2->addIncoming(Sum, BB.get());
+
+ // Wire up the rest of the incoming values, we want duplicate entries.
+ for (auto &PN : BB->phis()) {
+ auto *SomeValue = UndefValue::get(Int32Ty);
+ PN.addIncoming(SomeValue, BB1.get());
+ PN.addIncoming(SomeValue, BB1.get());
+ }
+
+ // Check that the phi nodes have 3 incoming values.
+ EXPECT_EQ(P1->getNumIncomingValues(), 3u);
+ EXPECT_EQ(P2->getNumIncomingValues(), 3u);
+
+ // Deduplicate the incoming values for BB1.
+ BB->deduplicatePhiUsesOf(BB1.get());
+
+ // Check that the phi nodes have 2 incoming values.
+ EXPECT_EQ(P1->getNumIncomingValues(), 2u);
+ EXPECT_EQ(P2->getNumIncomingValues(), 2u);
+
+ // Check that the incoming values are unique.
+ for (auto &PN : BB->phis()) {
+ EXPECT_EQ(BB.get(), PN.getIncomingBlock(0));
+ EXPECT_EQ(BB1.get(), PN.getIncomingBlock(1));
+ }
+}
+
#define CHECK_ITERATORS(Range1, Range2) \
EXPECT_EQ(std::distance(Range1.begin(), Range1.end()), \
std::distance(Range2.begin(), Range2.end())); \
|
There was a problem hiding this comment.
I think the general approach is fine, but please keep the implementation private to BitcodeReader. This does not seem like something that should be exposed as a generic helper.
(I think you could also handle this by calling removePredecessor the appropriate number of times, though maybe that's not significantly simpler.)
You can test this locally with the following command:git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 'HEAD~1' HEAD llvm/include/llvm/IR/BasicBlock.h llvm/lib/Bitcode/Reader/BitcodeReader.cpp llvm/lib/IR/BasicBlock.cpp llvm/test/Bitcode/constexpr-to-instr-dups.ll llvm/unittests/IR/BasicBlockTest.cppThe following files introduce new uses of undef:
Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields In tests, avoid using For example, this is considered a bad practice: define void @fn() {
...
br i1 undef, ...
}Please use the following instead: define void @fn(i1 %cond) {
...
br i1 %cond, ...
}Please refer to the Undefined Behavior Manual for more information. |
FlStamerGS
force-pushed
the
fix-phi-entry-deduplication
branch
from
3966763 to
a1d65a5
Compare
3 participants
Fixes a verification error that occurs when a basic block contains multiple PHI nodes with duplicate entries, where some use constexprs in them. In #141560 a fix was introduced that fails in this case of multiple PHI nodes, as this will only deduplicate the entries for the PHI nodes that use constexprs, but not for those that e.g. only use a constant.
Instead of deduplicating the entries during the creation they will now be deduplicated during the finalisation.