Skip to content

Commit afc5fbe

Browse files
committed
[SelectionDAGBuilder] Fix non-determanism in shouldKeepJumpConditionsTogether
The issue was we where iteration on `SmallPtrSet` order which is based on runtime address and thus can vary with the same input. Fix by just using `SmallMapVector` with dummy value.
1 parent 1f613bc commit afc5fbe

File tree

1 file changed

+14
-13
lines changed

1 file changed

+14
-13
lines changed

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2450,11 +2450,10 @@ SelectionDAGBuilder::EmitBranchForMergedCondition(const Value *Cond,
24502450

24512451
// Collect dependencies on V recursively. This is used for the cost analysis in
24522452
// `shouldKeepJumpConditionsTogether`.
2453-
static bool
2454-
collectInstructionDeps(SmallPtrSet<const Instruction *, 8> *Deps,
2455-
const Value *V,
2456-
SmallPtrSet<const Instruction *, 8> *Necessary = nullptr,
2457-
unsigned Depth = 0) {
2453+
static bool collectInstructionDeps(
2454+
SmallMapVector<const Instruction *, bool, 8> *Deps, const Value *V,
2455+
SmallMapVector<const Instruction *, bool, 8> *Necessary = nullptr,
2456+
unsigned Depth = 0) {
24582457
// Return false if we have an incomplete count.
24592458
if (Depth >= SelectionDAG::MaxRecursionDepth)
24602459
return false;
@@ -2471,7 +2470,7 @@ collectInstructionDeps(SmallPtrSet<const Instruction *, 8> *Deps,
24712470
}
24722471

24732472
// Already added this dep.
2474-
if (!Deps->insert(I).second)
2473+
if (!Deps->try_emplace(I, false).second)
24752474
return true;
24762475

24772476
for (unsigned OpIdx = 0, E = I->getNumOperands(); OpIdx < E; ++OpIdx)
@@ -2526,7 +2525,9 @@ bool SelectionDAGBuilder::shouldKeepJumpConditionsTogether(
25262525
return false;
25272526

25282527
// Collect "all" instructions that lhs condition is dependent on.
2529-
SmallPtrSet<const Instruction *, 8> LhsDeps, RhsDeps;
2528+
// Use map for stable iteration (to avoid non-determanism of iteration of
2529+
// SmallPtrSet). The `bool` value is just a dummy.
2530+
SmallMapVector<const Instruction *, bool, 8> LhsDeps, RhsDeps;
25302531
collectInstructionDeps(&LhsDeps, Lhs);
25312532
// Collect "all" instructions that rhs condition is dependent on AND are
25322533
// dependencies of lhs. This gives us an estimate on which instructions we
@@ -2536,7 +2537,7 @@ bool SelectionDAGBuilder::shouldKeepJumpConditionsTogether(
25362537
// Add the compare instruction itself unless its a dependency on the LHS.
25372538
if (const auto *RhsI = dyn_cast<Instruction>(Rhs))
25382539
if (!LhsDeps.contains(RhsI))
2539-
RhsDeps.insert(RhsI);
2540+
RhsDeps.try_emplace(RhsI, false);
25402541

25412542
const auto &TLI = DAG.getTargetLoweringInfo();
25422543
const auto &TTI =
@@ -2564,9 +2565,9 @@ bool SelectionDAGBuilder::shouldKeepJumpConditionsTogether(
25642565
// instructions.
25652566
for (unsigned PruneIters = 0; PruneIters < MaxPruneIters; ++PruneIters) {
25662567
const Instruction *ToDrop = nullptr;
2567-
for (const auto *Ins : RhsDeps) {
2568-
if (!ShouldCountInsn(Ins)) {
2569-
ToDrop = Ins;
2568+
for (const auto &InsPair : RhsDeps) {
2569+
if (!ShouldCountInsn(InsPair.first)) {
2570+
ToDrop = InsPair.first;
25702571
break;
25712572
}
25722573
}
@@ -2575,13 +2576,13 @@ bool SelectionDAGBuilder::shouldKeepJumpConditionsTogether(
25752576
RhsDeps.erase(ToDrop);
25762577
}
25772578

2578-
for (const auto *Ins : RhsDeps) {
2579+
for (const auto &InsPair : RhsDeps) {
25792580
// Finally accumulate latency that we can only attribute to computing the
25802581
// RHS condition. Use latency because we are essentially trying to calculate
25812582
// the cost of the dependency chain.
25822583
// Possible TODO: We could try to estimate ILP and make this more precise.
25832584
CostOfIncluding +=
2584-
TTI.getInstructionCost(Ins, TargetTransformInfo::TCK_Latency);
2585+
TTI.getInstructionCost(InsPair.first, TargetTransformInfo::TCK_Latency);
25852586

25862587
if (CostOfIncluding > CostThresh)
25872588
return false;

0 commit comments

Comments
 (0)