[llvm] [SimplifyCFG] Simplify identical predecessors (PR #173022)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 5 07:02:24 PST 2026
================
@@ -8053,113 +8088,158 @@ template <> struct llvm::DenseMapInfo<const SwitchSuccWrapper *> {
if (ABI->getSuccessor(0) != BBI->getSuccessor(0))
return false;
- // Need to check that PHIs in successor have matching values
+ // Need to check that PHIs in successor have matching values.
BasicBlock *Succ = ABI->getSuccessor(0);
- for (PHINode &Phi : Succ->phis()) {
- auto &PredIVs = (*LHS->PhiPredIVs)[&Phi];
- if (PredIVs[A] != PredIVs[B])
- return false;
- }
-
- return true;
+ auto IfPhiIVMatch = [A, B, &PhiPredIVs = *LHS->PhiPredIVs](PHINode &Phi) {
+ // Replace O(|Pred|) Phi.getIncomingValueForBlock with this O(1) hashmap
+ // query.
+ auto &PredIVs = PhiPredIVs[&Phi];
+ return PredIVs[A] == PredIVs[B];
+ };
+ return all_of(Succ->phis(), IfPhiIVMatch);
}
};
-bool SimplifyCFGOpt::simplifyDuplicateSwitchArms(SwitchInst *SI,
- DomTreeUpdater *DTU) {
+// Merge identical BBs into one of them.
+static bool mergeIdenticalBBs(ArrayRef<BasicBlock *> Candidates,
+ DomTreeUpdater *DTU) {
+ if (Candidates.size() < 2)
+ return false;
+
// Build Cases. Skip BBs that are not candidates for simplification. Mark
// PHINodes which need to be processed into PhiPredIVs. We decide to process
// an entire PHI at once after the loop, opposed to calling
// getIncomingValueForBlock inside this loop, since each call to
// getIncomingValueForBlock is O(|Preds|).
- SmallPtrSet<PHINode *, 8> Phis;
- SmallPtrSet<BasicBlock *, 8> Seen;
- DenseMap<PHINode *, SmallDenseMap<BasicBlock *, Value *, 8>> PhiPredIVs;
- DenseMap<BasicBlock *, SmallVector<unsigned, 32>> BBToSuccessorIndexes;
- SmallVector<SwitchSuccWrapper> Cases;
- Cases.reserve(SI->getNumSuccessors());
-
- for (unsigned I = 0; I < SI->getNumSuccessors(); ++I) {
- BasicBlock *BB = SI->getSuccessor(I);
-
- // FIXME: Support more than just a single BranchInst. One way we could do
- // this is by taking a hashing approach of all insts in BB.
- if (BB->size() != 1)
- continue;
-
- // FIXME: Relax that the terminator is a BranchInst by checking for equality
- // on other kinds of terminators. We decide to only support unconditional
- // branches for now for compile time reasons.
- auto *BI = dyn_cast<BranchInst>(BB->getTerminator());
- if (!BI || BI->isConditional())
- continue;
-
- if (!Seen.insert(BB).second) {
- auto It = BBToSuccessorIndexes.find(BB);
- if (It != BBToSuccessorIndexes.end())
- It->second.emplace_back(I);
- continue;
- }
-
- // FIXME: This case needs some extra care because the terminators other than
- // SI need to be updated. For now, consider only backedges to the SI.
- if (BB->getUniquePredecessor() != SI->getParent())
- continue;
-
- // Keep track of which PHIs we need as keys in PhiPredIVs below.
- for (BasicBlock *Succ : BI->successors())
- Phis.insert_range(llvm::make_pointer_range(Succ->phis()));
+ EqualBBWrapper::Phi2IVsMap PhiPredIVs;
+ SmallVector<EqualBBWrapper> BBs2Merge;
+ BBs2Merge.reserve(Candidates.size());
+ SmallSetVector<PHINode *, 8> Phis;
- // Add the successor only if not previously visited.
- Cases.emplace_back(SwitchSuccWrapper{BB, &PhiPredIVs});
- BBToSuccessorIndexes[BB].emplace_back(I);
+ for (BasicBlock *BB : Candidates) {
+ BasicBlock *Succ = BB->getSingleSuccessor();
+ assert(Succ && "Expected unconditional BB");
+ BBs2Merge.emplace_back(EqualBBWrapper{BB, &PhiPredIVs});
+ Phis.insert_range(make_pointer_range(Succ->phis()));
}
// Precompute a data structure to improve performance of isEqual for
- // SwitchSuccWrapper.
+ // EqualBBWrapper.
PhiPredIVs.reserve(Phis.size());
for (PHINode *Phi : Phis) {
auto &IVs =
PhiPredIVs.try_emplace(Phi, Phi->getNumIncomingValues()).first->second;
+ // Pre-fill all incoming for O(1) lookup as Phi.getIncomingValueForBlock is
+ // O(|Pred|).
for (auto &IV : Phi->incoming_values())
IVs.insert({Phi->getIncomingBlock(IV), IV.get()});
}
- // Build a set such that if the SwitchSuccWrapper exists in the set and
- // another SwitchSuccWrapper isEqual, then the equivalent SwitchSuccWrapper
- // which is not in the set should be replaced with the one in the set. If the
- // SwitchSuccWrapper is not in the set, then it should be added to the set so
- // other SwitchSuccWrappers can check against it in the same manner. We use
- // SwitchSuccWrapper instead of just BasicBlock because we'd like to pass
- // around information to isEquality, getHashValue, and when doing the
- // replacement with better performance.
- DenseSet<const SwitchSuccWrapper *> ReplaceWith;
- ReplaceWith.reserve(Cases.size());
+ // Group duplicates using DenseSet with custom equality/hashing.
+ // Build a set such that if the EqualBBWrapper exists in the set and another
+ // EqualBBWrapper isEqual, then the equivalent EqualBBWrapper which is not in
+ // the set should be replaced with the one in the set. If the EqualBBWrapper
+ // is not in the set, then it should be added to the set so other
+ // EqualBBWrapper can check against it in the same manner. We use
+ // EqualBBWrapper instead of just BasicBlock because we'd like to pass around
+ // information to isEquality, getHashValue, and when doing the replacement
+ // with better performance.
+ DenseSet<const EqualBBWrapper *> Keep;
+ Keep.reserve(BBs2Merge.size());
SmallVector<DominatorTree::UpdateType> Updates;
- Updates.reserve(ReplaceWith.size());
+ Updates.reserve(BBs2Merge.size() * 2);
+
bool MadeChange = false;
- for (auto &SSW : Cases) {
- // SSW is a candidate for simplification. If we find a duplicate BB,
- // replace it.
- const auto [It, Inserted] = ReplaceWith.insert(&SSW);
- if (!Inserted) {
- // We know that SI's parent BB no longer dominates the old case successor
- // since we are making it dead.
- Updates.push_back({DominatorTree::Delete, SI->getParent(), SSW.Dest});
- const auto &Successors = BBToSuccessorIndexes.at(SSW.Dest);
- for (unsigned Idx : Successors)
- SI->setSuccessor(Idx, (*It)->Dest);
- MadeChange = true;
+
+ // Helper: redirect all edges X -> DeadPred to X -> LivePred.
+ auto RedirectIncomingEdges = [&](BasicBlock *Dead, BasicBlock *Live) {
+ SmallSetVector<BasicBlock *, 8> DeadPreds(llvm::from_range,
+ predecessors(Dead));
+ if (DTU) {
+ // All predecessors of DeadPred (except the common predecessor) will be
+ // moved to LivePred.
+ Updates.reserve(Updates.size() + DeadPreds.size() * 2);
+ SmallPtrSet<BasicBlock *, 16> LivePreds(llvm::from_range,
+ predecessors(Live));
+ for (BasicBlock *PredOfDead : DeadPreds) {
+ // Do not modify those common predecessors of DeadPred and LivePred.
+ if (!LivePreds.contains(PredOfDead))
+ Updates.push_back({DominatorTree::Insert, PredOfDead, Live});
+ Updates.push_back({DominatorTree::Delete, PredOfDead, Dead});
+ }
+ }
+ LLVM_DEBUG(dbgs() << "Replacing duplicate pred BB ";
+ Dead->printAsOperand(dbgs()); dbgs() << " with pred ";
+ Live->printAsOperand(dbgs()); dbgs() << " for ";
+ Live->getSingleSuccessor()->printAsOperand(dbgs());
+ dbgs() << "\n");
+ // Replace successors in all predecessors of DeadPred.
+ for (BasicBlock *PredOfDead : DeadPreds) {
+ Instruction *T = PredOfDead->getTerminator();
+ T->replaceSuccessorWith(Dead, Live);
}
+ };
+
+ // Try to eliminate duplicate predecessors.
+ for (const auto &EBW : BBs2Merge) {
+ // Pred is a candidate for simplification. If we find a duplicate BB,
+ // replace it.
+ const auto &[It, Inserted] = Keep.insert(&EBW);
+ if (Inserted)
+ continue;
+
+ // Found duplicate: merge P into canonical predecessor It->Pred.
+ BasicBlock *KeepBB = (*It)->BB;
+ BasicBlock *DeadBB = EBW.BB;
+
+ // Avoid merging if either is the other's predecessor in weird ways.
----------------
nikic wrote:
Is this really what this condition is guarding against? I'd have thought this would occur if the same block occurs multiple times in the candidate list, rather than some kind of relationship between the blocks.
https://github.com/llvm/llvm-project/pull/173022
More information about the llvm-commits
mailing list