[llvm] add_EliminateNewDuplicatePHINodes (PR #135179)
Valery Pykhtin via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 10 06:46:38 PDT 2025
https://github.com/vpykhtin created https://github.com/llvm/llvm-project/pull/135179
None
>From 2f4e71abe9535b8f3d8ec6506e2242f436adf310 Mon Sep 17 00:00:00 2001
From: Valery Pykhtin <valery.pykhtin at amd.com>
Date: Thu, 10 Apr 2025 11:53:30 +0000
Subject: [PATCH] add_EliminateNewDuplicatePHINodes
---
llvm/include/llvm/Transforms/Utils/Local.h | 16 ++
llvm/lib/Transforms/Utils/Local.cpp | 203 +++++++++++++-----
llvm/unittests/Transforms/Utils/LocalTest.cpp | 200 +++++++++++++++++
3 files changed, 370 insertions(+), 49 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
index db064e1f41f02..06b0bc3611cff 100644
--- a/llvm/include/llvm/Transforms/Utils/Local.h
+++ b/llvm/include/llvm/Transforms/Utils/Local.h
@@ -175,6 +175,22 @@ bool EliminateDuplicatePHINodes(BasicBlock *BB);
bool EliminateDuplicatePHINodes(BasicBlock *BB,
SmallPtrSetImpl<PHINode *> &ToRemove);
+/// Check for and eliminate duplicate PHI nodes in the block. This function is
+/// specifically designed for scenarios where new PHI nodes are inserted into
+/// the beginning of the block, such when SSAUpdaterBulk::RewriteAllUses. It
+/// compares the newly inserted PHI nodes against the existing ones and if a
+/// new PHI node is found to be a duplicate of an existing one, the new node is
+/// removed. Existing PHI nodes are left unmodified, even if they are
+/// duplicates. New nodes are also deleted if they are duplicates of each other.
+/// Similar to EliminateDuplicatePHINodes, this function assumes a consistent
+/// order for all incoming values across PHI nodes in the block. FirstExistedPN
+/// Points to the first existing PHI node in the block. Newly inserted PHI nodes
+/// should not reference one another. However, they may reference themselves or
+/// existing PHI nodes, and existing PHI nodes may reference the newly inserted
+/// PHI nodes.
+bool EliminateNewDuplicatePHINodes(BasicBlock *BB,
+ BasicBlock::phi_iterator FirstExistedPN);
+
/// This function is used to do simplification of a CFG. For example, it
/// adjusts branches to branches to eliminate the extra hop, it eliminates
/// unreachable basic blocks, and does other peephole optimization of the CFG.
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 2f3ea2266e07f..271d6a4c3bdf3 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1419,65 +1419,65 @@ EliminateDuplicatePHINodesNaiveImpl(BasicBlock *BB,
return Changed;
}
-static bool
-EliminateDuplicatePHINodesSetBasedImpl(BasicBlock *BB,
- SmallPtrSetImpl<PHINode *> &ToRemove) {
- // This implementation doesn't currently consider undef operands
- // specially. Theoretically, two phis which are identical except for
- // one having an undef where the other doesn't could be collapsed.
+// This implementation doesn't currently consider undef operands
+// specially. Theoretically, two phis which are identical except for
+// one having an undef where the other doesn't could be collapsed.
- struct PHIDenseMapInfo {
- static PHINode *getEmptyKey() {
- return DenseMapInfo<PHINode *>::getEmptyKey();
- }
+struct PHIDenseMapInfo {
+ static PHINode *getEmptyKey() {
+ return DenseMapInfo<PHINode *>::getEmptyKey();
+ }
- static PHINode *getTombstoneKey() {
- return DenseMapInfo<PHINode *>::getTombstoneKey();
- }
+ static PHINode *getTombstoneKey() {
+ return DenseMapInfo<PHINode *>::getTombstoneKey();
+ }
- static bool isSentinel(PHINode *PN) {
- return PN == getEmptyKey() || PN == getTombstoneKey();
- }
+ static bool isSentinel(const PHINode *PN) {
+ return PN == getEmptyKey() || PN == getTombstoneKey();
+ }
- // WARNING: this logic must be kept in sync with
- // Instruction::isIdenticalToWhenDefined()!
- static unsigned getHashValueImpl(PHINode *PN) {
- // Compute a hash value on the operands. Instcombine will likely have
- // sorted them, which helps expose duplicates, but we have to check all
- // the operands to be safe in case instcombine hasn't run.
- return static_cast<unsigned>(hash_combine(
- hash_combine_range(PN->value_op_begin(), PN->value_op_end()),
- hash_combine_range(PN->block_begin(), PN->block_end())));
- }
+ // WARNING: this logic must be kept in sync with
+ // Instruction::isIdenticalToWhenDefined()!
+ static unsigned getHashValueImpl(const PHINode *PN) {
+ // Compute a hash value on the operands. Instcombine will likely have
+ // sorted them, which helps expose duplicates, but we have to check all
+ // the operands to be safe in case instcombine hasn't run.
+ return static_cast<unsigned>(hash_combine(
+ hash_combine_range(PN->value_op_begin(), PN->value_op_end()),
+ hash_combine_range(PN->block_begin(), PN->block_end())));
+ }
- static unsigned getHashValue(PHINode *PN) {
+ static unsigned getHashValue(const PHINode *PN) {
#ifndef NDEBUG
- // If -phicse-debug-hash was specified, return a constant -- this
- // will force all hashing to collide, so we'll exhaustively search
- // the table for a match, and the assertion in isEqual will fire if
- // there's a bug causing equal keys to hash differently.
- if (PHICSEDebugHash)
- return 0;
+ // If -phicse-debug-hash was specified, return a constant -- this
+ // will force all hashing to collide, so we'll exhaustively search
+ // the table for a match, and the assertion in isEqual will fire if
+ // there's a bug causing equal keys to hash differently.
+ if (PHICSEDebugHash)
+ return 0;
#endif
- return getHashValueImpl(PN);
- }
+ return getHashValueImpl(PN);
+ }
- static bool isEqualImpl(PHINode *LHS, PHINode *RHS) {
- if (isSentinel(LHS) || isSentinel(RHS))
- return LHS == RHS;
- return LHS->isIdenticalTo(RHS);
- }
+ static bool isEqualImpl(const PHINode *LHS, const PHINode *RHS) {
+ if (isSentinel(LHS) || isSentinel(RHS))
+ return LHS == RHS;
+ return LHS->isIdenticalTo(RHS);
+ }
- static bool isEqual(PHINode *LHS, PHINode *RHS) {
- // These comparisons are nontrivial, so assert that equality implies
- // hash equality (DenseMap demands this as an invariant).
- bool Result = isEqualImpl(LHS, RHS);
- assert(!Result || (isSentinel(LHS) && LHS == RHS) ||
- getHashValueImpl(LHS) == getHashValueImpl(RHS));
- return Result;
- }
- };
+ static bool isEqual(const PHINode *LHS, const PHINode *RHS) {
+ // These comparisons are nontrivial, so assert that equality implies
+ // hash equality (DenseMap demands this as an invariant).
+ bool Result = isEqualImpl(LHS, RHS);
+ assert(!Result || (isSentinel(LHS) && LHS == RHS) ||
+ getHashValueImpl(LHS) == getHashValueImpl(RHS));
+ return Result;
+ }
+};
+static bool
+EliminateDuplicatePHINodesSetBasedImpl(BasicBlock *BB,
+ SmallPtrSetImpl<PHINode *> &ToRemove) {
// Set of unique PHINodes.
DenseSet<PHINode *, PHIDenseMapInfo> PHISet;
PHISet.reserve(4 * PHICSENumPHISmallSize);
@@ -1524,6 +1524,111 @@ bool llvm::EliminateDuplicatePHINodes(BasicBlock *BB) {
return Changed;
}
+#ifndef NDEBUG // Should this be under EXPENSIVE_CHECKS?
+// New PHI nodes should not reference one another but they may reference
+// themselves or existing PHI nodes, and existing PHI nodes may reference new
+// PHI nodes.
+static bool
+PHIAreRefEachOther(const iterator_range<BasicBlock::phi_iterator> &NewPHIs) {
+ SmallPtrSet<PHINode *, 8> NewPHISet;
+ for (PHINode &PN : NewPHIs)
+ NewPHISet.insert(&PN);
+ for (PHINode &PHI : NewPHIs) {
+ for (Value *V : PHI.incoming_values()) {
+ PHINode *IncPHI = dyn_cast<PHINode>(V);
+ if (IncPHI && IncPHI != &PHI && NewPHISet.contains(IncPHI))
+ return true;
+ }
+ }
+ return false;
+}
+#endif
+
+bool EliminateNewDuplicatePHINodesN2(BasicBlock *BB,
+ BasicBlock::phi_iterator FirstExistedPN) {
+ auto ReplaceIfIdentical = [](PHINode &PHI, PHINode &ReplPHI) {
+ if (!PHI.isIdenticalToWhenDefined(&ReplPHI))
+ return false;
+ PHI.replaceAllUsesWith(&ReplPHI);
+ PHI.eraseFromParent();
+ return true;
+ };
+
+ // Deduplicate new PHIs first to reduce the number of comparisons on the
+ // following new -> existing pass.
+ bool Changed = false;
+ for (auto I = BB->phis().begin(); I != FirstExistedPN; ++I) {
+ for (auto J = std::next(I); J != FirstExistedPN;) {
+ Changed |= ReplaceIfIdentical(*J++, *I);
+ }
+ }
+
+ // Iterate over existing PHIs and replace identical new PHIs.
+ for (PHINode &ExistingPHI : make_range(FirstExistedPN, BB->phis().end())) {
+ auto I = BB->phis().begin();
+ assert(I != FirstExistedPN); // Should be at least one new PHI.
+ do {
+ Changed |= ReplaceIfIdentical(*I++, ExistingPHI);
+ } while (I != FirstExistedPN);
+ if (BB->phis().begin() == FirstExistedPN)
+ return Changed;
+ }
+ return Changed;
+}
+
+bool EliminateNewDuplicatePHINodesSet(BasicBlock *BB,
+ BasicBlock::phi_iterator FirstExistedPN) {
+ auto Replace = [](PHINode &PHI, PHINode &ReplPHI) {
+ PHI.replaceAllUsesWith(&ReplPHI);
+ PHI.eraseFromParent();
+ return true;
+ };
+
+ DenseSet<PHINode *, PHIDenseMapInfo> NewPHISet;
+ NewPHISet.reserve(4 * PHICSENumPHISmallSize);
+
+ // Deduplicate new PHIs, note that NewPHISet remains consistent because new
+ // PHIs are not reference each other.
+ bool Changed = false;
+ for (PHINode &NewPHI :
+ make_early_inc_range(make_range(BB->phis().begin(), FirstExistedPN))) {
+ auto [I, Inserted] = NewPHISet.insert(&NewPHI);
+ if (!Inserted)
+ Changed |= Replace(NewPHI, **I);
+ }
+
+ // Iterate over existing PHIs and replace matching new PHIs.
+ for (PHINode &ExistingPHI : make_range(FirstExistedPN, BB->phis().end())) {
+ assert(!NewPHISet.empty()); // Should be at least one new PHI.
+ auto I = NewPHISet.find(&ExistingPHI);
+ if (I == NewPHISet.end())
+ continue;
+ Changed |= Replace(**I, ExistingPHI);
+ NewPHISet.erase(I);
+ if (NewPHISet.empty())
+ return Changed;
+ }
+ return Changed;
+}
+
+bool llvm::EliminateNewDuplicatePHINodes(
+ BasicBlock *BB, BasicBlock::phi_iterator FirstExistedPN) {
+
+ if (hasNItemsOrLess(BB->phis(), 1))
+ return false;
+
+ auto NewPHIs = make_range(BB->phis().begin(), FirstExistedPN);
+ assert(!PHIAreRefEachOther(NewPHIs));
+
+ // Both functions perform identical pass over existing PHIs and differ in time
+ // spent on new PHI duplicate check which depends on the number of new PHIs.
+ // Therefore make a choice based on the number of new PHIs and not the total
+ // number of PHIs in the block.
+ return hasNItemsOrLess(NewPHIs, PHICSENumPHISmallSize)
+ ? EliminateNewDuplicatePHINodesN2(BB, FirstExistedPN)
+ : EliminateNewDuplicatePHINodesSet(BB, FirstExistedPN);
+}
+
Align llvm::tryEnforceAlignment(Value *V, Align PrefAlign,
const DataLayout &DL) {
V = V->stripPointerCasts();
diff --git a/llvm/unittests/Transforms/Utils/LocalTest.cpp b/llvm/unittests/Transforms/Utils/LocalTest.cpp
index 3c7a892c9d65a..f7b658b17b6ef 100644
--- a/llvm/unittests/Transforms/Utils/LocalTest.cpp
+++ b/llvm/unittests/Transforms/Utils/LocalTest.cpp
@@ -1362,3 +1362,203 @@ TEST(Local, ReplaceDbgVariableRecord) {
// Teardown.
RetInst->DebugMarker->eraseFromParent();
}
+
+bool EliminateNewDuplicatePHINodesN2(BasicBlock *BB,
+ BasicBlock::phi_iterator FirstExistedPN);
+bool EliminateNewDuplicatePHINodesSet(BasicBlock *BB,
+ BasicBlock::phi_iterator FirstExistedPN);
+
+TEST(Local, RaceEliminateNewDuplicatePHINodes) {
+ GTEST_SKIP(); // Comment out to run this test manually.
+ using namespace std::chrono;
+
+ LLVMContext C;
+ IRBuilder<> B(C);
+ std::unique_ptr<Function> F(
+ Function::Create(FunctionType::get(B.getVoidTy(), false),
+ GlobalValue::ExternalLinkage, "F"));
+ BasicBlock *Entry(BasicBlock::Create(C, "", F.get()));
+ BasicBlock *BB(BasicBlock::Create(C, "", F.get()));
+ BranchInst::Create(BB, Entry);
+ B.SetInsertPoint(BB);
+ auto *Ret = B.CreateRetVoid();
+ B.SetInsertPoint(Ret);
+
+ const unsigned NumPreds = 5;
+ for (unsigned Pass = 1; Pass < 64; ++Pass) {
+ auto *PHI = B.CreatePHI(Type::getInt32Ty(C), NumPreds);
+ for (unsigned I = 0; I < NumPreds; ++I)
+ PHI->addIncoming(B.getInt32(Pass), Entry);
+
+ if (Pass < 2)
+ continue;
+
+ auto FirstExistedPN = std::next(BB->phis().begin(), Pass / 2);
+ const unsigned NumRuns = 1000000;
+
+ outs() << "Num phis: " << Pass;
+ auto Start = high_resolution_clock::now();
+ for (unsigned Run = NumRuns; Run > 0; --Run)
+ EliminateNewDuplicatePHINodesSet(BB, FirstExistedPN);
+ auto End = high_resolution_clock::now();
+ auto TH = duration_cast<milliseconds>(End - Start).count();
+ outs() << " H: " << TH;
+
+ Start = high_resolution_clock::now();
+ for (unsigned Run = NumRuns; Run > 0; --Run)
+ EliminateNewDuplicatePHINodesN2(BB, FirstExistedPN);
+ End = high_resolution_clock::now();
+ auto TN2 = duration_cast<milliseconds>(End - Start).count();
+
+ outs() << " N2: " << TN2 << " Diff: " << (TH - TN2)
+ << " Ratio: " << format("%.3f", ((double)TH / TN2)) << "\n";
+ }
+}
+
+// Helper to run both versions on the same input.
+static void RunEliminateNewDuplicatePHINode(
+ const char *AsmText,
+ std::function<void(BasicBlock &,
+ bool(BasicBlock *BB, BasicBlock::phi_iterator))>
+ Check) {
+ LLVMContext C;
+ for (int Pass = 0; Pass < 2; ++Pass) {
+ std::unique_ptr<Module> M = parseIR(C, AsmText);
+ Function *F = M->getFunction("main");
+ auto BBIt = std::find_if(F->begin(), F->end(), [](const BasicBlock &Block) {
+ return Block.getName() == "testbb";
+ });
+ ASSERT_NE(BBIt, F->end());
+ Check(*BBIt, Pass == 0 ? EliminateNewDuplicatePHINodesSet
+ : EliminateNewDuplicatePHINodesN2);
+ }
+}
+
+static BasicBlock::phi_iterator getPhiIt(BasicBlock &BB, unsigned Idx) {
+ return std::next(BB.phis().begin(), Idx);
+}
+
+static PHINode *getPhi(BasicBlock &BB, unsigned Idx) {
+ return &*getPhiIt(BB, Idx);
+}
+
+static int getNumPHIs(BasicBlock &BB) {
+ return std::distance(BB.phis().begin(), BB.phis().end());
+}
+
+TEST(Local, EliminateNewDuplicatePHINodes_OrderExisting) {
+ RunEliminateNewDuplicatePHINode(R"(
+ define void @main() {
+ entry:
+ br label %testbb
+ testbb:
+ %np0 = phi i32 [ 1, %entry ]
+ %np1 = phi i32 [ 1, %entry ]
+ %ep0 = phi i32 [ 1, %entry ]
+ %ep1 = phi i32 [ 1, %entry ]
+ %u = add i32 %np0, %np1
+ ret void
+ }
+ )", [](BasicBlock &BB, auto *ENDPN) {
+ AssertingVH<PHINode> EP0 = getPhi(BB, 2);
+ AssertingVH<PHINode> EP1 = getPhi(BB, 3);
+ EXPECT_TRUE(ENDPN(&BB, getPhiIt(BB, 2)));
+ // Expected:
+ // %ep0 = phi i32 [ 1, %entry ]
+ // %ep1 = phi i32 [ 1, %entry ]
+ // %u = add i32 %ep0, %ep0
+ EXPECT_EQ(getNumPHIs(BB), 2);
+ Instruction &Add = *BB.getFirstNonPHIIt();
+ EXPECT_EQ(Add.getOperand(0), EP0);
+ EXPECT_EQ(Add.getOperand(1), EP0);
+ (void)EP1; // Avoid "unused" warning.
+ });
+}
+
+TEST(Local, EliminateNewDuplicatePHINodes_OrderNew) {
+ RunEliminateNewDuplicatePHINode(R"(
+ define void @main() {
+ entry:
+ br label %testbb
+ testbb:
+ %np0 = phi i32 [ 1, %entry ]
+ %np1 = phi i32 [ 1, %entry ]
+ %ep0 = phi i32 [ 2, %entry ]
+ %ep1 = phi i32 [ 2, %entry ]
+ %u = add i32 %np0, %np1
+ ret void
+ }
+ )", [](BasicBlock &BB, auto *ENDPN) {
+ AssertingVH<PHINode> NP0 = getPhi(BB, 0);
+ AssertingVH<PHINode> EP0 = getPhi(BB, 2);
+ AssertingVH<PHINode> EP1 = getPhi(BB, 3);
+ EXPECT_TRUE(ENDPN(&BB, getPhiIt(BB, 2)));
+ // Expected:
+ // %np0 = phi i32 [ 1, %entry ]
+ // %ep0 = phi i32 [ 2, %entry ]
+ // %ep1 = phi i32 [ 2, %entry ]
+ // %u = add i32 %np0, %np0
+ EXPECT_EQ(getNumPHIs(BB), 3);
+ Instruction &Add = *BB.getFirstNonPHIIt();
+ EXPECT_EQ(Add.getOperand(0), NP0);
+ EXPECT_EQ(Add.getOperand(1), NP0);
+ (void)EP0;
+ (void)EP1; // Avoid "unused" warning.
+ });
+}
+
+TEST(Local, EliminateNewDuplicatePHINodes_NewRefExisting) {
+ RunEliminateNewDuplicatePHINode(R"(
+ define void @main() {
+ entry:
+ br label %testbb
+ testbb:
+ %np0 = phi i32 [ 1, %entry ], [ %ep0, %testbb ]
+ %np1 = phi i32 [ 1, %entry ], [ %ep1, %testbb ]
+ %ep0 = phi i32 [ 1, %entry ], [ %ep0, %testbb ]
+ %ep1 = phi i32 [ 1, %entry ], [ %ep1, %testbb ]
+ %u = add i32 %np0, %np1
+ br label %testbb
+ }
+ )", [](BasicBlock &BB, auto *ENDPN) {
+ AssertingVH<PHINode> EP0 = getPhi(BB, 2);
+ AssertingVH<PHINode> EP1 = getPhi(BB, 3);
+ EXPECT_TRUE(ENDPN(&BB, getPhiIt(BB, 2)));
+ // Expected:
+ // %ep0 = phi i32 [ 1, %entry ], [ %ep0, %testbb ]
+ // %ep1 = phi i32 [ 1, %entry ], [ %ep1, %testbb ]
+ // %u = add i32 %ep0, %ep1
+ EXPECT_EQ(getNumPHIs(BB), 2);
+ Instruction &Add = *BB.getFirstNonPHIIt();
+ EXPECT_EQ(Add.getOperand(0), EP0);
+ EXPECT_EQ(Add.getOperand(1), EP1);
+ });
+}
+
+TEST(Local, EliminateNewDuplicatePHINodes_ExistingRefNew) {
+ RunEliminateNewDuplicatePHINode(R"(
+ define void @main() {
+ entry:
+ br label %testbb
+ testbb:
+ %np0 = phi i32 [ 1, %entry ], [ %np0, %testbb ]
+ %np1 = phi i32 [ 1, %entry ], [ %np1, %testbb ]
+ %ep0 = phi i32 [ 1, %entry ], [ %np0, %testbb ]
+ %ep1 = phi i32 [ 1, %entry ], [ %np1, %testbb ]
+ %u = add i32 %np0, %np1
+ br label %testbb
+ }
+ )", [](BasicBlock &BB, auto *ENDPN) {
+ AssertingVH<PHINode> EP0 = getPhi(BB, 2);
+ AssertingVH<PHINode> EP1 = getPhi(BB, 3);
+ EXPECT_TRUE(ENDPN(&BB, getPhiIt(BB, 2)));
+ // Expected:
+ // %ep0 = phi i32 [ 1, %entry ], [ %ep0, %testbb ]
+ // %ep1 = phi i32 [ 1, %entry ], [ %ep1, %testbb ]
+ // %u = add i32 %ep0, %ep1
+ EXPECT_EQ(getNumPHIs(BB), 2);
+ Instruction &Add = *BB.getFirstNonPHIIt();
+ EXPECT_EQ(Add.getOperand(0), EP0);
+ EXPECT_EQ(Add.getOperand(1), EP1);
+ });
+}
\ No newline at end of file
More information about the llvm-commits
mailing list