Skip to content

[SelectionDAGBuilder] Fix non-determanism in shouldKeepJumpConditionsTogether #83687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[SelectionDAGBuilder] Fix non-determanism in shouldKeepJumpConditionsTogether #83687

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2450,11 +2450,10 @@ SelectionDAGBuilder::EmitBranchForMergedCondition(const Value *Cond,

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

// Already added this dep.
if (!Deps->insert(I).second)
if (!Deps->try_emplace(I, false).second)
return true;

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

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

const auto &TLI = DAG.getTargetLoweringInfo();
const auto &TTI =
Expand Down Expand Up @@ -2564,9 +2565,9 @@ bool SelectionDAGBuilder::shouldKeepJumpConditionsTogether(
// instructions.
for (unsigned PruneIters = 0; PruneIters < MaxPruneIters; ++PruneIters) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you give up multipass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err... brain farted and forgot that we are progressively dropping deps.

Let me re-post, ill just do you smallmapvector idea.

const Instruction *ToDrop = nullptr;
for (const auto *Ins : RhsDeps) {
if (!ShouldCountInsn(Ins)) {
ToDrop = Ins;
for (const auto &InsPair : RhsDeps) {
if (!ShouldCountInsn(InsPair.first)) {
ToDrop = InsPair.first;
break;
}
}
Expand All @@ -2575,13 +2576,13 @@ bool SelectionDAGBuilder::shouldKeepJumpConditionsTogether(
RhsDeps.erase(ToDrop);
}

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

if (CostOfIncluding > CostThresh)
return false;
Expand Down