From 4fa45fc3d3ee774cdf42139ec8a95cdff9553b78 Mon Sep 17 00:00:00 2001 From: AdityaK Date: Thu, 2 May 2024 16:08:51 -0700 Subject: [PATCH 1/4] Fix non-determinisms GVNSink used to order instructions based on their pointer values and was prone to non-determinism because of that. Using DFSNumber for ordering all the values fixes the non-determinism --- llvm/lib/Transforms/Scalar/GVNSink.cpp | 65 +++++++++++++++---- .../test/Transforms/GVNSink/int_sideeffect.ll | 26 ++++++++ 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp index d4907326eb0a5..c6613f427d8ec 100644 --- a/llvm/lib/Transforms/Scalar/GVNSink.cpp +++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp @@ -222,20 +222,29 @@ raw_ostream &operator<<(raw_ostream &OS, const SinkingInstructionCandidate &C) { class ModelledPHI { SmallVector Values; SmallVector Blocks; + DominatorTree *DT; public: ModelledPHI() = default; - ModelledPHI(const PHINode *PN) { + ModelledPHI(const PHINode *PN, DominatorTree *DT) : DT(DT) { // BasicBlock comes first so we sort by basic block pointer order, then by value pointer order. - SmallVector, 4> Ops; + using OpsType = std::pair; + SmallVector Ops; for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I) Ops.push_back({PN->getIncomingBlock(I), PN->getIncomingValue(I)}); - llvm::sort(Ops); + + auto DFSOrder = [DT](OpsType O1, OpsType O2) { + return DT->getNode(O1.first) < DT->getNode(O2.first); + }; + // Sort by DFSNumber to have a deterministic order. + llvm::sort(Ops, DFSOrder); + for (auto &P : Ops) { Blocks.push_back(P.first); Values.push_back(P.second); } + verifyModelledPHI(); } /// Create a dummy ModelledPHI that will compare unequal to any other ModelledPHI @@ -247,19 +256,37 @@ class ModelledPHI { return M; } + void verifyModelledPHI() { + assert(Values.size() > 1 && Blocks.size() > 1 && + "Modelling PHI with less than 2 values"); + auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { + return this->DT->getNode(BB1) < this->DT->getNode(BB2); + }; + assert(llvm::is_sorted(Blocks, DFSOrder)); + int C = 0; + llvm::for_each(Values, [&C, this](const Value *V) { + const Instruction *I = cast(V); + assert(I->getParent() == this->Blocks[C++]); + }); + } /// Create a PHI from an array of incoming values and incoming blocks. - template - ModelledPHI(const VArray &V, const BArray &B) { + ModelledPHI(SmallVectorImpl &V, + SmallSetVector &B, DominatorTree *DT) + : DT(DT) { + // The order of Values and Blocks are already as per their DFSNumbers. llvm::copy(V, std::back_inserter(Values)); llvm::copy(B, std::back_inserter(Blocks)); + verifyModelledPHI(); } /// Create a PHI from [I[OpNum] for I in Insts]. - template - ModelledPHI(ArrayRef Insts, unsigned OpNum, const BArray &B) { + ModelledPHI(ArrayRef Insts, unsigned OpNum, + SmallSetVector &B, DominatorTree *DT) + : DT(DT) { llvm::copy(B, std::back_inserter(Blocks)); for (auto *I : Insts) Values.push_back(I->getOperand(OpNum)); + verifyModelledPHI(); } /// Restrict the PHI's contents down to only \c NewBlocks. @@ -297,7 +324,8 @@ class ModelledPHI { // Hash functor unsigned hash() const { - return (unsigned)hash_combine_range(Values.begin(), Values.end()); + // Is deterministic because Values are saved in DFSOrder. + return (unsigned)hash_combine_range(Values.begin(), Values.end()); } bool operator==(const ModelledPHI &Other) const { @@ -566,7 +594,7 @@ class ValueTable { class GVNSink { public: - GVNSink() = default; + GVNSink(DominatorTree *DT) : DT(DT) {} bool run(Function &F) { LLVM_DEBUG(dbgs() << "GVNSink: running on function @" << F.getName() @@ -583,6 +611,7 @@ class GVNSink { private: ValueTable VN; + DominatorTree *DT; bool shouldAvoidSinkingInstruction(Instruction *I) { // These instructions may change or break semantics if moved. @@ -603,7 +632,7 @@ class GVNSink { void analyzeInitialPHIs(BasicBlock *BB, ModelledPHISet &PHIs, SmallPtrSetImpl &PHIContents) { for (PHINode &PN : BB->phis()) { - auto MPHI = ModelledPHI(&PN); + auto MPHI = ModelledPHI(&PN, DT); PHIs.insert(MPHI); for (auto *V : MPHI.getValues()) PHIContents.insert(V); @@ -691,7 +720,7 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI, } // The sunk instruction's results. - ModelledPHI NewPHI(NewInsts, ActivePreds); + ModelledPHI NewPHI(NewInsts, ActivePreds, DT); // Does sinking this instruction render previous PHIs redundant? if (NeededPHIs.erase(NewPHI)) @@ -728,7 +757,7 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI, return std::nullopt; for (unsigned OpNum = 0, E = I0->getNumOperands(); OpNum != E; ++OpNum) { - ModelledPHI PHI(NewInsts, OpNum, ActivePreds); + ModelledPHI PHI(NewInsts, OpNum, ActivePreds, DT); if (PHI.areAllIncomingValuesSame()) continue; if (!canReplaceOperandWithVariable(I0, OpNum)) @@ -774,7 +803,11 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { } if (Preds.size() < 2) return 0; - llvm::sort(Preds); + auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { + return this->DT->getNode(BB1) < this->DT->getNode(BB2); + }; + // Sort by DFSNumber to have a deterministic order. + llvm::sort(Preds, DFSOrder); unsigned NumOrigPreds = Preds.size(); // We can only sink instructions through unconditional branches. @@ -886,8 +919,12 @@ void GVNSink::sinkLastInstruction(ArrayRef Blocks, } // end anonymous namespace PreservedAnalyses GVNSinkPass::run(Function &F, FunctionAnalysisManager &AM) { - GVNSink G; + auto &DT = AM.getResult(F); + GVNSink G(&DT); if (!G.run(F)) return PreservedAnalyses::all(); + + // PHI nodes get inserted which haven't been added to the Dominator Tree. + // FIXME: Update DominatorTree to account for sunk instructions. return PreservedAnalyses::none(); } diff --git a/llvm/test/Transforms/GVNSink/int_sideeffect.ll b/llvm/test/Transforms/GVNSink/int_sideeffect.ll index 3cc54e84f17c1..9a3bc062dd946 100644 --- a/llvm/test/Transforms/GVNSink/int_sideeffect.ll +++ b/llvm/test/Transforms/GVNSink/int_sideeffect.ll @@ -28,3 +28,29 @@ if.end: ret float %phi } +; CHECK-LABEL: scalarsSinkingReverse +; CHECK-NOT: fmul +; CHECK: = phi +; CHECK: = fmul +define float @scalarsSinkingReverse(float %d, float %m, float %a, i1 %cmp) { +; This test is just a reverse(graph mirror) of the test +; above to ensure GVNSink doesn't depend on the order of branches. +entry: + br i1 %cmp, label %if.then, label %if.else + +if.then: + %add = fadd float %m, %a + %mul1 = fmul float %add, %d + br label %if.end + +if.else: + call void @llvm.sideeffect() + %sub = fsub float %m, %a + %mul0 = fmul float %sub, %d + br label %if.end + +if.end: + %phi = phi float [ %mul1, %if.then ], [ %mul0, %if.else ] + ret float %phi +} + From 879999b8f2f9c368e72b7bedb7048ed85fcb2f93 Mon Sep 17 00:00:00 2001 From: AdityaK Date: Fri, 3 May 2024 15:25:37 -0700 Subject: [PATCH 2/4] Fix issues with DT update, use DFSNumbers --- llvm/lib/Transforms/Scalar/GVNSink.cpp | 35 +++++++++++++++++--------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp index c6613f427d8ec..1f8f2d511a686 100644 --- a/llvm/lib/Transforms/Scalar/GVNSink.cpp +++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp @@ -42,6 +42,7 @@ #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/Statistic.h" +#include "llvm/Analysis/DomTreeUpdater.h" #include "llvm/Analysis/GlobalsModRef.h" #include "llvm/IR/BasicBlock.h" #include "llvm/IR/CFG.h" @@ -228,14 +229,17 @@ class ModelledPHI { ModelledPHI() = default; ModelledPHI(const PHINode *PN, DominatorTree *DT) : DT(DT) { - // BasicBlock comes first so we sort by basic block pointer order, then by value pointer order. + // BasicBlock comes first so we sort by basic block pointer order, + // then by value pointer order. No need to call `verifyModelledPHI` + // As the Values and Blocks are populated in DFSOrder already. using OpsType = std::pair; SmallVector Ops; for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I) Ops.push_back({PN->getIncomingBlock(I), PN->getIncomingValue(I)}); auto DFSOrder = [DT](OpsType O1, OpsType O2) { - return DT->getNode(O1.first) < DT->getNode(O2.first); + return DT->getNode(O1.first)->getDFSNumIn() < + DT->getNode(O2.first)->getDFSNumIn(); }; // Sort by DFSNumber to have a deterministic order. llvm::sort(Ops, DFSOrder); @@ -244,7 +248,6 @@ class ModelledPHI { Blocks.push_back(P.first); Values.push_back(P.second); } - verifyModelledPHI(); } /// Create a dummy ModelledPHI that will compare unequal to any other ModelledPHI @@ -260,13 +263,16 @@ class ModelledPHI { assert(Values.size() > 1 && Blocks.size() > 1 && "Modelling PHI with less than 2 values"); auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { - return this->DT->getNode(BB1) < this->DT->getNode(BB2); + return DT->getNode(BB1)->getDFSNumIn() < DT->getNode(BB2)->getDFSNumIn(); }; assert(llvm::is_sorted(Blocks, DFSOrder)); int C = 0; llvm::for_each(Values, [&C, this](const Value *V) { - const Instruction *I = cast(V); - assert(I->getParent() == this->Blocks[C++]); + if (!isa(V)) { + const Instruction *I = cast(V); + assert(I->getParent() == this->Blocks[C]); + } + C++; }); } /// Create a PHI from an array of incoming values and incoming blocks. @@ -280,13 +286,13 @@ class ModelledPHI { } /// Create a PHI from [I[OpNum] for I in Insts]. + /// TODO: Figure out a way to verifyModelledPHI in this constructor. ModelledPHI(ArrayRef Insts, unsigned OpNum, SmallSetVector &B, DominatorTree *DT) : DT(DT) { llvm::copy(B, std::back_inserter(Blocks)); for (auto *I : Insts) Values.push_back(I->getOperand(OpNum)); - verifyModelledPHI(); } /// Restrict the PHI's contents down to only \c NewBlocks. @@ -795,6 +801,9 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { BBEnd->printAsOperand(dbgs()); dbgs() << "\n"); SmallVector Preds; for (auto *B : predecessors(BBEnd)) { + // Bailout on malformed CFG where BasicBlock has no predecessor(PR42346). + if (!DT->getNode(B)) + return 0; auto *T = B->getTerminator(); if (isa(T) || isa(T)) Preds.push_back(B); @@ -804,7 +813,7 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { if (Preds.size() < 2) return 0; auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { - return this->DT->getNode(BB1) < this->DT->getNode(BB2); + return DT->getNode(BB1)->getDFSNumIn() < DT->getNode(BB2)->getDFSNumIn(); }; // Sort by DFSNumber to have a deterministic order. llvm::sort(Preds, DFSOrder); @@ -844,11 +853,12 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { auto C = Candidates.front(); LLVM_DEBUG(dbgs() << " -- Sinking: " << C << "\n"); + DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager); BasicBlock *InsertBB = BBEnd; if (C.Blocks.size() < NumOrigPreds) { LLVM_DEBUG(dbgs() << " -- Splitting edge to "; BBEnd->printAsOperand(dbgs()); dbgs() << "\n"); - InsertBB = SplitBlockPredecessors(BBEnd, C.Blocks, ".gvnsink.split"); + InsertBB = SplitBlockPredecessors(BBEnd, C.Blocks, ".gvnsink.split", &DTU); if (!InsertBB) { LLVM_DEBUG(dbgs() << " -- FAILED to split edge!\n"); // Edge couldn't be split. @@ -921,10 +931,11 @@ void GVNSink::sinkLastInstruction(ArrayRef Blocks, PreservedAnalyses GVNSinkPass::run(Function &F, FunctionAnalysisManager &AM) { auto &DT = AM.getResult(F); GVNSink G(&DT); + DT.updateDFSNumbers(); if (!G.run(F)) return PreservedAnalyses::all(); - // PHI nodes get inserted which haven't been added to the Dominator Tree. - // FIXME: Update DominatorTree to account for sunk instructions. - return PreservedAnalyses::none(); + PreservedAnalyses PA; + PA.preserve(); + return PA; } From 37992c488a48675581776cabafb6972cd0c0e047 Mon Sep 17 00:00:00 2001 From: AdityaK Date: Wed, 8 May 2024 14:55:33 -0700 Subject: [PATCH 3/4] Populate DFS numbers ahead of time This prevents quadratic behavior of calling DominatorTree::updateDFSNumbers everytime BasicBlocks are split to sink instructions --- llvm/lib/Transforms/Scalar/GVNSink.cpp | 42 +++++++++++++++----------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp index 1f8f2d511a686..57422759437c3 100644 --- a/llvm/lib/Transforms/Scalar/GVNSink.cpp +++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp @@ -223,12 +223,12 @@ raw_ostream &operator<<(raw_ostream &OS, const SinkingInstructionCandidate &C) { class ModelledPHI { SmallVector Values; SmallVector Blocks; - DominatorTree *DT; public: ModelledPHI() = default; - ModelledPHI(const PHINode *PN, DominatorTree *DT) : DT(DT) { + ModelledPHI(const PHINode *PN, + const DenseMap &DFS) { // BasicBlock comes first so we sort by basic block pointer order, // then by value pointer order. No need to call `verifyModelledPHI` // As the Values and Blocks are populated in DFSOrder already. @@ -237,9 +237,8 @@ class ModelledPHI { for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I) Ops.push_back({PN->getIncomingBlock(I), PN->getIncomingValue(I)}); - auto DFSOrder = [DT](OpsType O1, OpsType O2) { - return DT->getNode(O1.first)->getDFSNumIn() < - DT->getNode(O2.first)->getDFSNumIn(); + auto DFSOrder = [DFS](OpsType O1, OpsType O2) { + return DFS.lookup(O1.first) < DFS.lookup(O2.first); }; // Sort by DFSNumber to have a deterministic order. llvm::sort(Ops, DFSOrder); @@ -259,11 +258,11 @@ class ModelledPHI { return M; } - void verifyModelledPHI() { + void verifyModelledPHI(const DenseMap &DFS) { assert(Values.size() > 1 && Blocks.size() > 1 && "Modelling PHI with less than 2 values"); - auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { - return DT->getNode(BB1)->getDFSNumIn() < DT->getNode(BB2)->getDFSNumIn(); + auto DFSOrder = [DFS](const BasicBlock *BB1, const BasicBlock *BB2) { + return DFS.lookup(BB1) < DFS.lookup(BB2); }; assert(llvm::is_sorted(Blocks, DFSOrder)); int C = 0; @@ -277,19 +276,18 @@ class ModelledPHI { } /// Create a PHI from an array of incoming values and incoming blocks. ModelledPHI(SmallVectorImpl &V, - SmallSetVector &B, DominatorTree *DT) - : DT(DT) { + SmallSetVector &B, + const DenseMap &DFS) { // The order of Values and Blocks are already as per their DFSNumbers. llvm::copy(V, std::back_inserter(Values)); llvm::copy(B, std::back_inserter(Blocks)); - verifyModelledPHI(); + verifyModelledPHI(DFS); } /// Create a PHI from [I[OpNum] for I in Insts]. /// TODO: Figure out a way to verifyModelledPHI in this constructor. ModelledPHI(ArrayRef Insts, unsigned OpNum, - SmallSetVector &B, DominatorTree *DT) - : DT(DT) { + SmallSetVector &B) { llvm::copy(B, std::back_inserter(Blocks)); for (auto *I : Insts) Values.push_back(I->getOperand(OpNum)); @@ -609,6 +607,13 @@ class GVNSink { unsigned NumSunk = 0; ReversePostOrderTraversal RPOT(&F); VN.setReachableBBs(BasicBlocksSet(RPOT.begin(), RPOT.end())); + // Populated DFSNumbers ahead of time to avoid updating dominator tree + // when CFG is modified. The DFSNumbers of newly created basic blocks + // are irrelevant because RPOT is also obtained ahead of time and only + // DFSNumbers of original CFG are relevant for sinkable candidates. + for (auto *BB : RPOT) + if (DT->getNode(BB)) + DFSNumbers[BB] = DT->getNode(BB)->getDFSNumIn(); for (auto *N : RPOT) NumSunk += sinkBB(N); @@ -618,6 +623,7 @@ class GVNSink { private: ValueTable VN; DominatorTree *DT; + DenseMap DFSNumbers; bool shouldAvoidSinkingInstruction(Instruction *I) { // These instructions may change or break semantics if moved. @@ -638,7 +644,7 @@ class GVNSink { void analyzeInitialPHIs(BasicBlock *BB, ModelledPHISet &PHIs, SmallPtrSetImpl &PHIContents) { for (PHINode &PN : BB->phis()) { - auto MPHI = ModelledPHI(&PN, DT); + auto MPHI = ModelledPHI(&PN, DFSNumbers); PHIs.insert(MPHI); for (auto *V : MPHI.getValues()) PHIContents.insert(V); @@ -726,7 +732,7 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI, } // The sunk instruction's results. - ModelledPHI NewPHI(NewInsts, ActivePreds, DT); + ModelledPHI NewPHI(NewInsts, ActivePreds, DFSNumbers); // Does sinking this instruction render previous PHIs redundant? if (NeededPHIs.erase(NewPHI)) @@ -763,7 +769,7 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI, return std::nullopt; for (unsigned OpNum = 0, E = I0->getNumOperands(); OpNum != E; ++OpNum) { - ModelledPHI PHI(NewInsts, OpNum, ActivePreds, DT); + ModelledPHI PHI(NewInsts, OpNum, ActivePreds); if (PHI.areAllIncomingValuesSame()) continue; if (!canReplaceOperandWithVariable(I0, OpNum)) @@ -802,7 +808,7 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { SmallVector Preds; for (auto *B : predecessors(BBEnd)) { // Bailout on malformed CFG where BasicBlock has no predecessor(PR42346). - if (!DT->getNode(B)) + if (!DFSNumbers.count(B)) return 0; auto *T = B->getTerminator(); if (isa(T) || isa(T)) @@ -813,7 +819,7 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { if (Preds.size() < 2) return 0; auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { - return DT->getNode(BB1)->getDFSNumIn() < DT->getNode(BB2)->getDFSNumIn(); + return DFSNumbers.lookup(BB1) < DFSNumbers.lookup(BB2); }; // Sort by DFSNumber to have a deterministic order. llvm::sort(Preds, DFSOrder); From 4e21809f9134490d8aa43dd5e6c0eb6d31b705af Mon Sep 17 00:00:00 2001 From: AdityaK Date: Thu, 9 May 2024 13:54:55 -0700 Subject: [PATCH 4/4] Use reverse-post order instead of DFS order As noted by Eli and Nikita, there is no need to use depth first ordering because all we need is a deterministic order. This approach avoids calling expensive DFSNumbering operation and reuse an existing visitation order --- llvm/lib/Transforms/Scalar/GVNSink.cpp | 78 +++++++++++++------------- 1 file changed, 38 insertions(+), 40 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp index 57422759437c3..ddf01dc612bb5 100644 --- a/llvm/lib/Transforms/Scalar/GVNSink.cpp +++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp @@ -42,7 +42,6 @@ #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/Statistic.h" -#include "llvm/Analysis/DomTreeUpdater.h" #include "llvm/Analysis/GlobalsModRef.h" #include "llvm/IR/BasicBlock.h" #include "llvm/IR/CFG.h" @@ -228,20 +227,20 @@ class ModelledPHI { ModelledPHI() = default; ModelledPHI(const PHINode *PN, - const DenseMap &DFS) { + const DenseMap &BlockOrder) { // BasicBlock comes first so we sort by basic block pointer order, // then by value pointer order. No need to call `verifyModelledPHI` - // As the Values and Blocks are populated in DFSOrder already. + // As the Values and Blocks are populated in a deterministic order. using OpsType = std::pair; SmallVector Ops; for (unsigned I = 0, E = PN->getNumIncomingValues(); I != E; ++I) Ops.push_back({PN->getIncomingBlock(I), PN->getIncomingValue(I)}); - auto DFSOrder = [DFS](OpsType O1, OpsType O2) { - return DFS.lookup(O1.first) < DFS.lookup(O2.first); + auto ComesBefore = [BlockOrder](OpsType O1, OpsType O2) { + return BlockOrder.lookup(O1.first) < BlockOrder.lookup(O2.first); }; - // Sort by DFSNumber to have a deterministic order. - llvm::sort(Ops, DFSOrder); + // Sort in a deterministic order. + llvm::sort(Ops, ComesBefore); for (auto &P : Ops) { Blocks.push_back(P.first); @@ -258,13 +257,15 @@ class ModelledPHI { return M; } - void verifyModelledPHI(const DenseMap &DFS) { + void + verifyModelledPHI(const DenseMap &BlockOrder) { assert(Values.size() > 1 && Blocks.size() > 1 && "Modelling PHI with less than 2 values"); - auto DFSOrder = [DFS](const BasicBlock *BB1, const BasicBlock *BB2) { - return DFS.lookup(BB1) < DFS.lookup(BB2); + auto ComesBefore = [BlockOrder](const BasicBlock *BB1, + const BasicBlock *BB2) { + return BlockOrder.lookup(BB1) < BlockOrder.lookup(BB2); }; - assert(llvm::is_sorted(Blocks, DFSOrder)); + assert(llvm::is_sorted(Blocks, ComesBefore)); int C = 0; llvm::for_each(Values, [&C, this](const Value *V) { if (!isa(V)) { @@ -277,11 +278,11 @@ class ModelledPHI { /// Create a PHI from an array of incoming values and incoming blocks. ModelledPHI(SmallVectorImpl &V, SmallSetVector &B, - const DenseMap &DFS) { - // The order of Values and Blocks are already as per their DFSNumbers. + const DenseMap &BlockOrder) { + // The order of Values and Blocks are already ordered by the caller. llvm::copy(V, std::back_inserter(Values)); llvm::copy(B, std::back_inserter(Blocks)); - verifyModelledPHI(DFS); + verifyModelledPHI(BlockOrder); } /// Create a PHI from [I[OpNum] for I in Insts]. @@ -328,7 +329,7 @@ class ModelledPHI { // Hash functor unsigned hash() const { - // Is deterministic because Values are saved in DFSOrder. + // Is deterministic because Values are saved in a specific order. return (unsigned)hash_combine_range(Values.begin(), Values.end()); } @@ -598,7 +599,7 @@ class ValueTable { class GVNSink { public: - GVNSink(DominatorTree *DT) : DT(DT) {} + GVNSink() {} bool run(Function &F) { LLVM_DEBUG(dbgs() << "GVNSink: running on function @" << F.getName() @@ -607,13 +608,16 @@ class GVNSink { unsigned NumSunk = 0; ReversePostOrderTraversal RPOT(&F); VN.setReachableBBs(BasicBlocksSet(RPOT.begin(), RPOT.end())); - // Populated DFSNumbers ahead of time to avoid updating dominator tree - // when CFG is modified. The DFSNumbers of newly created basic blocks - // are irrelevant because RPOT is also obtained ahead of time and only - // DFSNumbers of original CFG are relevant for sinkable candidates. + // Populate reverse post-order to order basic blocks in deterministic + // order. Any arbitrary ordering will work in this case as long as they are + // deterministic. The node ordering of newly created basic blocks + // are irrelevant because RPOT(for computing sinkable candidates) is also + // obtained ahead of time and only their order are relevant for this pass. + unsigned NodeOrdering = 0; + RPOTOrder[*RPOT.begin()] = ++NodeOrdering; for (auto *BB : RPOT) - if (DT->getNode(BB)) - DFSNumbers[BB] = DT->getNode(BB)->getDFSNumIn(); + if (!pred_empty(BB)) + RPOTOrder[BB] = ++NodeOrdering; for (auto *N : RPOT) NumSunk += sinkBB(N); @@ -622,8 +626,7 @@ class GVNSink { private: ValueTable VN; - DominatorTree *DT; - DenseMap DFSNumbers; + DenseMap RPOTOrder; bool shouldAvoidSinkingInstruction(Instruction *I) { // These instructions may change or break semantics if moved. @@ -644,7 +647,7 @@ class GVNSink { void analyzeInitialPHIs(BasicBlock *BB, ModelledPHISet &PHIs, SmallPtrSetImpl &PHIContents) { for (PHINode &PN : BB->phis()) { - auto MPHI = ModelledPHI(&PN, DFSNumbers); + auto MPHI = ModelledPHI(&PN, RPOTOrder); PHIs.insert(MPHI); for (auto *V : MPHI.getValues()) PHIContents.insert(V); @@ -732,7 +735,7 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI, } // The sunk instruction's results. - ModelledPHI NewPHI(NewInsts, ActivePreds, DFSNumbers); + ModelledPHI NewPHI(NewInsts, ActivePreds, RPOTOrder); // Does sinking this instruction render previous PHIs redundant? if (NeededPHIs.erase(NewPHI)) @@ -807,8 +810,8 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { BBEnd->printAsOperand(dbgs()); dbgs() << "\n"); SmallVector Preds; for (auto *B : predecessors(BBEnd)) { - // Bailout on malformed CFG where BasicBlock has no predecessor(PR42346). - if (!DFSNumbers.count(B)) + // Bailout on basic blocks without predecessor(PR42346). + if (!RPOTOrder.count(B)) return 0; auto *T = B->getTerminator(); if (isa(T) || isa(T)) @@ -818,11 +821,11 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { } if (Preds.size() < 2) return 0; - auto DFSOrder = [this](const BasicBlock *BB1, const BasicBlock *BB2) { - return DFSNumbers.lookup(BB1) < DFSNumbers.lookup(BB2); + auto ComesBefore = [this](const BasicBlock *BB1, const BasicBlock *BB2) { + return RPOTOrder.lookup(BB1) < RPOTOrder.lookup(BB2); }; - // Sort by DFSNumber to have a deterministic order. - llvm::sort(Preds, DFSOrder); + // Sort in a deterministic order. + llvm::sort(Preds, ComesBefore); unsigned NumOrigPreds = Preds.size(); // We can only sink instructions through unconditional branches. @@ -859,12 +862,11 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) { auto C = Candidates.front(); LLVM_DEBUG(dbgs() << " -- Sinking: " << C << "\n"); - DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager); BasicBlock *InsertBB = BBEnd; if (C.Blocks.size() < NumOrigPreds) { LLVM_DEBUG(dbgs() << " -- Splitting edge to "; BBEnd->printAsOperand(dbgs()); dbgs() << "\n"); - InsertBB = SplitBlockPredecessors(BBEnd, C.Blocks, ".gvnsink.split", &DTU); + InsertBB = SplitBlockPredecessors(BBEnd, C.Blocks, ".gvnsink.split"); if (!InsertBB) { LLVM_DEBUG(dbgs() << " -- FAILED to split edge!\n"); // Edge couldn't be split. @@ -935,13 +937,9 @@ void GVNSink::sinkLastInstruction(ArrayRef Blocks, } // end anonymous namespace PreservedAnalyses GVNSinkPass::run(Function &F, FunctionAnalysisManager &AM) { - auto &DT = AM.getResult(F); - GVNSink G(&DT); - DT.updateDFSNumbers(); + GVNSink G; if (!G.run(F)) return PreservedAnalyses::all(); - PreservedAnalyses PA; - PA.preserve(); - return PA; + return PreservedAnalyses::none(); }