Skip to content
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.

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

JIT: Enable profile consistency checking up to morph #111047

Merged
merged 11 commits into from
Jan 7, 2025
10 changes: 5 additions & 5 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4711,11 +4711,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_CLONE_FINALLY, &Compiler::fgCloneFinally);

// Drop back to just checking profile likelihoods.
//
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;
activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS;

// Do some flow-related optimizations
//
if (opts.OptimizationEnabled())
Expand Down Expand Up @@ -4784,6 +4779,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_MORPH_IMPBYREF, &Compiler::fgRetypeImplicitByRefArgs);

// Drop back to just checking profile likelihoods.
//
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;
activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS;

#ifdef DEBUG
// Now that locals have address-taken and implicit byref marked, we can safely apply stress.
lvaStressLclFld();
Expand Down
26 changes: 25 additions & 1 deletion src/coreclr/jit/fgehopt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2410,6 +2410,7 @@ PhaseStatus Compiler::fgTailMergeThrows()
}

JITDUMP("\n*** found %d merge candidates, rewriting flow\n\n", numCandidates);
bool modifiedProfile = false;

// Second pass.
//
Expand All @@ -2418,14 +2419,37 @@ PhaseStatus Compiler::fgTailMergeThrows()
{
BasicBlock* const nonCanonicalBlock = iter->GetKey();
BasicBlock* const canonicalBlock = iter->GetValue();
weight_t removedWeight = BB_ZERO_WEIGHT;

// Walk pred list of the non canonical block, updating flow to target
// the canonical block instead.
for (BasicBlock* const predBlock : nonCanonicalBlock->PredBlocksEditing())
for (FlowEdge* const predEdge : nonCanonicalBlock->PredEdgesEditing())
{
removedWeight += predEdge->getLikelyWeight();
BasicBlock* const predBlock = predEdge->getSourceBlock();
JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum);
fgReplaceJumpTarget(predBlock, nonCanonicalBlock, canonicalBlock);
}

if (canonicalBlock->hasProfileWeight())
{
canonicalBlock->setBBProfileWeight(canonicalBlock->bbWeight + removedWeight);
modifiedProfile = true;

// Don't bother updating flow into nonCanonicalBlock, since it is now unreachable
}
}

// In practice, when we have true profile data, we can repair it locally above, since the no-return
// calls mean that there is no contribution from the throw blocks to any of their successors.
// However, these blocks won't be morphed into BBJ_THROW blocks until later,
// so mark profile data as inconsistent for now.
if (modifiedProfile)
{
JITDUMP(
"fgTailMergeThrows: Modified flow into no-return blocks that still have successors. Data %s inconsistent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

// Update the count of noreturn call sites
Expand Down
60 changes: 27 additions & 33 deletions src/coreclr/jit/fgopt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1317,37 +1317,15 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc
}
#endif // DEBUG

//
// When we optimize a branch to branch we need to update the profile weight
// of bDest by subtracting out the weight of the path that is being optimized.
//
if (bDest->hasProfileWeight())
{
FlowEdge* const edge = fgGetPredForBlock(bDest, block);
noway_assert(edge != nullptr);

const weight_t edgeWeight = edge->getLikelyWeight();

//
// Update the bDest->bbWeight
//
if (bDest->bbWeight > edgeWeight)
{
bDest->bbWeight -= edgeWeight;
}
else
{
bDest->bbWeight = BB_ZERO_WEIGHT;
bDest->SetFlags(BBF_RUN_RARELY); // Set the RarelyRun flag
}
}
weight_t removedWeight;

// Optimize the JUMP to empty unconditional JUMP to go to the new target
switch (block->GetKind())
{
case BBJ_ALWAYS:
case BBJ_CALLFINALLYRET:
{
removedWeight = block->bbWeight;
fgRedirectTargetEdge(block, bDest->GetTarget());
break;
}
Expand All @@ -1356,11 +1334,13 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc
if (block->TrueTargetIs(bDest))
{
assert(!block->FalseTargetIs(bDest));
removedWeight = block->GetTrueEdge()->getLikelyWeight();
fgRedirectTrueEdge(block, bDest->GetTarget());
}
else
{
assert(block->FalseTargetIs(bDest));
removedWeight = block->GetFalseEdge()->getLikelyWeight();
fgRedirectFalseEdge(block, bDest->GetTarget());
}
break;
Expand All @@ -1369,6 +1349,15 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc
unreached();
}

//
// When we optimize a branch to branch we need to update the profile weight
// of bDest by subtracting out the weight of the path that is being optimized.
//
if (bDest->hasProfileWeight())
{
bDest->setBBProfileWeight(max(0.0, bDest->bbWeight - removedWeight));
}

return true;
}
return false;
Expand Down Expand Up @@ -1628,17 +1617,10 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block)
//
FlowEdge* const oldEdge = *jmpTab;

if (fgIsUsingProfileWeights() && bDest->hasProfileWeight())
if (bDest->hasProfileWeight())
{
weight_t const branchThroughWeight = oldEdge->getLikelyWeight();
if (bDest->bbWeight > branchThroughWeight)
{
bDest->bbWeight -= branchThroughWeight;
}
else
{
bDest->bbSetRunRarely();
}
bDest->setBBProfileWeight(max(0.0, bDest->bbWeight - branchThroughWeight));
}

// Update the switch jump table
Expand Down Expand Up @@ -1676,6 +1658,11 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block)
{
// Invalidate the set of unique targets for block, since we modified the targets
fgInvalidateSwitchDescMapEntry(block);

JITDUMP(
"fgOptimizeSwitchBranches: Optimized switch flow. Profile needs to be re-propagated. Data %s consistent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

Statement* switchStmt = nullptr;
Expand Down Expand Up @@ -6614,6 +6601,13 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early)
FlowEdge* const newEdge = fgAddRefPred(crossJumpTarget, predBlock);
predBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge);
}

// For tail merge we have a common successor of predBlock and
// crossJumpTarget, so the profile update can be done locally.
if (crossJumpTarget->hasProfileWeight())
{
crossJumpTarget->setBBProfileWeight(crossJumpTarget->bbWeight + predBlock->bbWeight);
}
}

// We changed things
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/fgprofile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5134,7 +5134,8 @@ void Compiler::fgRepairProfileCondToUncond(BasicBlock* block,

// If profile weights are consistent, expect at worst a slight underflow.
//
if (fgPgoConsistent && (alternateNewWeight < 0.0))
const bool checkProfileConsistency = hasFlag(activePhaseChecks, PhaseChecks::CHECK_PROFILE);
if (checkProfileConsistency && fgPgoConsistent && (alternateNewWeight < 0.0))
{
assert(fgProfileWeightsEqual(alternateNewWeight, 0.0));
}
Expand Down
58 changes: 9 additions & 49 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12658,63 +12658,23 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block)
fgRemoveStmt(block, lastStmt);
result = FoldResult::FOLD_REMOVED_LAST_STMT;
}
// block is a BBJ_COND that we are folding the conditional for.
// bTaken is the path that will always be taken from block.
// bNotTaken is the path that will never be taken from block.
//
BasicBlock* bTaken;
BasicBlock* bNotTaken;
FlowEdge* edgeTaken;

FlowEdge *retainedEdge, *removedEdge;

if (cond->AsIntCon()->gtIconVal != 0)
{
// JTRUE 1 - transform the basic block into a BBJ_ALWAYS
bTaken = block->GetTrueTarget();
bNotTaken = block->GetFalseTarget();

// Remove 'block' from the predecessor list of 'bNotTaken' */
fgRemoveRefPred(block->GetFalseEdge());

edgeTaken = block->GetTrueEdge();
block->SetKindAndTargetEdge(BBJ_ALWAYS, edgeTaken);
retainedEdge = block->GetTrueEdge();
removedEdge = block->GetFalseEdge();
}
else
{
// JTRUE 0 - transform the basic block into a BBJ_ALWAYS
bTaken = block->GetFalseTarget();
bNotTaken = block->GetTrueTarget();

// Remove 'block' from the predecessor list of 'bNotTaken' */
fgRemoveRefPred(block->GetTrueEdge());

edgeTaken = block->GetFalseEdge();
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetFalseEdge());
retainedEdge = block->GetFalseEdge();
removedEdge = block->GetTrueEdge();
}

// We examine the taken edge (block -> bTaken)
// if block has valid profile weight and bTaken does not we try to adjust bTaken's weight
// else if bTaken has valid profile weight and block does not we try to adjust block's weight
// We can only adjust the block weights when (the edge block -> bTaken) is the only edge into bTaken
//
if (block->hasProfileWeight())
{
if (!bTaken->hasProfileWeight())
{
if ((bTaken->countOfInEdges() == 1) || (bTaken->bbWeight < block->bbWeight))
{
// Update the weight of bTaken
bTaken->inheritWeight(block);
}
}
}
else if (bTaken->hasProfileWeight())
{
if (bTaken->countOfInEdges() == 1)
{
// Update the weight of block
block->inheritWeight(bTaken);
}
}
fgRemoveRefPred(removedEdge);
block->SetKindAndTargetEdge(BBJ_ALWAYS, retainedEdge);
fgRepairProfileCondToUncond(block, retainedEdge, removedEdge);

#ifdef DEBUG
if (verbose)
Expand Down
Loading