[llvm] [GVNSink] Fix non-determinisms by using Depth-First ordering (PR #90995)
via llvm-commits
llvm-commits at lists.llvm.org
Wed May 8 15:28:33 PDT 2024
https://github.com/hiraditya updated https://github.com/llvm/llvm-project/pull/90995
>From 057b4e0a864aa45d732b083e16b9c2e52e42a3ae Mon Sep 17 00:00:00 2001
From: AdityaK <hiraditya at msn.com>
Date: Thu, 2 May 2024 16:08:51 -0700
Subject: [PATCH 1/3] 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<Value *, 4> Values;
SmallVector<BasicBlock *, 4> 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<std::pair<BasicBlock *, Value *>, 4> Ops;
+ using OpsType = std::pair<BasicBlock *, Value *>;
+ SmallVector<OpsType, 4> 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<Instruction>(V);
+ assert(I->getParent() == this->Blocks[C++]);
+ });
+ }
/// Create a PHI from an array of incoming values and incoming blocks.
- template <typename VArray, typename BArray>
- ModelledPHI(const VArray &V, const BArray &B) {
+ ModelledPHI(SmallVectorImpl<Instruction *> &V,
+ SmallSetVector<BasicBlock *, 4> &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 <typename BArray>
- ModelledPHI(ArrayRef<Instruction *> Insts, unsigned OpNum, const BArray &B) {
+ ModelledPHI(ArrayRef<Instruction *> Insts, unsigned OpNum,
+ SmallSetVector<BasicBlock *, 4> &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<Value *> &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<BasicBlock *> Blocks,
} // end anonymous namespace
PreservedAnalyses GVNSinkPass::run(Function &F, FunctionAnalysisManager &AM) {
- GVNSink G;
+ auto &DT = AM.getResult<DominatorTreeAnalysis>(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 e313e6126ba2930f6426fc5645af604a6c7b98aa Mon Sep 17 00:00:00 2001
From: AdityaK <hiraditya at msn.com>
Date: Fri, 3 May 2024 15:25:37 -0700
Subject: [PATCH 2/3] 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<BasicBlock *, Value *>;
SmallVector<OpsType, 4> 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<Instruction>(V);
- assert(I->getParent() == this->Blocks[C++]);
+ if (!isa<UndefValue>(V)) {
+ const Instruction *I = cast<Instruction>(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<Instruction *> Insts, unsigned OpNum,
SmallSetVector<BasicBlock *, 4> &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<BasicBlock *, 4> 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<BranchInst>(T) || isa<SwitchInst>(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<BasicBlock *> Blocks,
PreservedAnalyses GVNSinkPass::run(Function &F, FunctionAnalysisManager &AM) {
auto &DT = AM.getResult<DominatorTreeAnalysis>(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<DominatorTreeAnalysis>();
+ return PA;
}
>From c01f607f7d02eb0242d8cd7f6636d1c9ece6957e Mon Sep 17 00:00:00 2001
From: AdityaK <hiraditya at msn.com>
Date: Wed, 8 May 2024 14:55:33 -0700
Subject: [PATCH 3/3] 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..df8e70fdca59f 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<Value *, 4> Values;
SmallVector<BasicBlock *, 4> Blocks;
- DominatorTree *DT;
public:
ModelledPHI() = default;
- ModelledPHI(const PHINode *PN, DominatorTree *DT) : DT(DT) {
+ ModelledPHI(const PHINode *PN,
+ const DenseMap<const BasicBlock*, int64_t> &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<const BasicBlock *, int64_t> &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<Instruction *> &V,
- SmallSetVector<BasicBlock *, 4> &B, DominatorTree *DT)
- : DT(DT) {
+ SmallSetVector<BasicBlock *, 4> &B,
+ const DenseMap<const BasicBlock *, int64_t> &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<Instruction *> Insts, unsigned OpNum,
- SmallSetVector<BasicBlock *, 4> &B, DominatorTree *DT)
- : DT(DT) {
+ SmallSetVector<BasicBlock *, 4> &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<Function*> 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<const BasicBlock *, int64_t> 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<Value *> &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<BasicBlock *, 4> 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<BranchInst>(T) || isa<SwitchInst>(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);
More information about the llvm-commits
mailing list