Skip to content

Commit

Permalink
JIT: Move profile consistency checks to after morph (#111253)
Browse files Browse the repository at this point in the history 
Part of #107749. Enables profile checks for morph and post-morph phases.

For benchmarks.run_pgo, 45383 methods are consistent before inlining; after, we're down to 37215, or 82%. By the time we make it to morph, 33461 methods (~74% of the original) are consistent; after morph, we're down to 29402 (~65%). The decline isn't too dramatic for this collection, though I imagine we fare worse elsewhere.

---------

Co-authored-by: Andy Ayers <[email protected]>
  • Loading branch information
@amanasifkhalid @AndyAyersMS
amanasifkhalid and AndyAyersMS authored Jan 14, 2025
1 parent bbe2cb9 commit 845dc11
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 87 deletions.
10 changes: 5 additions & 5 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4779,11 +4779,6 @@ 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 Expand Up @@ -4819,6 +4814,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_GS_COOKIE, &Compiler::gsPhase);

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

if (opts.OptimizationEnabled())
{
// Compute the block weights
Expand Down
23 changes: 19 additions & 4 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,29 @@ void Compiler::fgConvertBBToThrowBB(BasicBlock* block)
}

// Scrub this block from the pred lists of any successors
fgRemoveBlockAsPred(block);
bool profileInconsistent = false;

for (BasicBlock* const succBlock : block->Succs(this))
{
FlowEdge* const succEdge = fgRemoveAllRefPreds(succBlock, block);

if (block->hasProfileWeight() && succBlock->hasProfileWeight())
{
succBlock->decreaseBBProfileWeight(succEdge->getLikelyWeight());
profileInconsistent |= (succBlock->NumSucc() > 0);
}
}

if (profileInconsistent)
{
JITDUMP("Flow removal of " FMT_BB " needs to be propagated. Data %s inconsistent.\n", block->bbNum,
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

// Update jump kind after the scrub.
block->SetKindAndTargetEdge(BBJ_THROW);
block->RemoveFlags(BBF_RETLESS_CALL); // no longer a BBJ_CALLFINALLY

// Any block with a throw is rare
block->bbSetRunRarely();
}

/*****************************************************************************
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/fgehopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2433,7 +2433,7 @@ PhaseStatus Compiler::fgTailMergeThrows()

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

// Don't bother updating flow into nonCanonicalBlock, since it is now unreachable
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/fgopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1355,7 +1355,7 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc
//
if (bDest->hasProfileWeight())
{
bDest->setBBProfileWeight(max(0.0, bDest->bbWeight - removedWeight));
bDest->decreaseBBProfileWeight(removedWeight);
}

return true;
Expand Down Expand Up @@ -1620,7 +1620,7 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block)
if (bDest->hasProfileWeight())
{
weight_t const branchThroughWeight = oldEdge->getLikelyWeight();
bDest->setBBProfileWeight(max(0.0, bDest->bbWeight - branchThroughWeight));
bDest->decreaseBBProfileWeight(branchThroughWeight);
}

// Update the switch jump table
Expand Down Expand Up @@ -6633,7 +6633,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early)
// crossJumpTarget, so the profile update can be done locally.
if (crossJumpTarget->hasProfileWeight())
{
crossJumpTarget->setBBProfileWeight(crossJumpTarget->bbWeight + predBlock->bbWeight);
crossJumpTarget->increaseBBProfileWeight(predBlock->bbWeight);
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/jitmetadatalist.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ JITMETADATAMETRIC(InlineAttempt, int, 0)
JITMETADATAMETRIC(InlineCount, int, 0)
JITMETADATAMETRIC(ProfileConsistentBeforeInline, int, 0)
JITMETADATAMETRIC(ProfileConsistentAfterInline, int, 0)
JITMETADATAMETRIC(ProfileConsistentBeforeMorph, int, 0)
JITMETADATAMETRIC(ProfileConsistentAfterMorph, int, 0)
JITMETADATAMETRIC(ProfileSynthesizedBlendedOrRepaired, int, 0)
JITMETADATAMETRIC(ProfileInconsistentInitially, int, 0)
JITMETADATAMETRIC(ProfileInconsistentResetLeave, int, 0)
Expand Down
169 changes: 95 additions & 74 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5366,55 +5366,27 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call)
//
if (compCurBB->KindIs(BBJ_ALWAYS))
{
BasicBlock* const curBlock = compCurBB;
BasicBlock* const targetBlock = curBlock->GetTarget();

// Flow no longer reaches the target from here.
//
fgRemoveRefPred(compCurBB->GetTargetEdge());
fgRemoveRefPred(curBlock->GetTargetEdge());

// Adjust profile weights of the successor blocks.
// Adjust profile weights of the successor block.
//
// Note if this is a tail call to loop, further updates
// are needed once we install the loop edge.
//
BasicBlock* curBlock = compCurBB;
if (curBlock->hasProfileWeight())
if (curBlock->hasProfileWeight() && targetBlock->hasProfileWeight())
{
weight_t weightLoss = curBlock->bbWeight;
BasicBlock* nextBlock = curBlock->GetTarget();
targetBlock->decreaseBBProfileWeight(curBlock->bbWeight);

while (nextBlock->hasProfileWeight())
if (targetBlock->NumSucc() > 0)
{
// Since we have linear flow we can update the next block weight.
//
weight_t const nextWeight = nextBlock->bbWeight;
weight_t const newNextWeight = nextWeight - weightLoss;

// If the math would result in a negative weight then there's
// no local repair we can do; just leave things inconsistent.
//
if (newNextWeight >= 0)
{
// Note if we'd already morphed the IR in nextblock we might
// have done something profile sensitive that we should arguably reconsider.
//
JITDUMP("Reducing profile weight of " FMT_BB " from " FMT_WT " to " FMT_WT "\n", nextBlock->bbNum,
nextWeight, newNextWeight);

nextBlock->setBBProfileWeight(newNextWeight);
}
else
{
JITDUMP("Not reducing profile weight of " FMT_BB " as its weight " FMT_WT
" is less than direct flow pred " FMT_BB " weight " FMT_WT "\n",
nextBlock->bbNum, nextWeight, compCurBB->bbNum, weightLoss);
}

if (!nextBlock->KindIs(BBJ_ALWAYS))
{
break;
}

curBlock = nextBlock;
nextBlock = curBlock->GetTarget();
JITDUMP("Flow removal out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n",
curBlock->bbNum, fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}
}
}
Expand Down Expand Up @@ -6755,12 +6727,12 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa
fgRemoveStmt(block, lastStmt);

// Set the loop edge.
BasicBlock* entryBB;
if (opts.IsOSR())
{
// Todo: this may not look like a viable loop header.
// Might need the moral equivalent of an init BB.
FlowEdge* const newEdge = fgAddRefPred(fgEntryBB, block);
block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge);
entryBB = fgEntryBB;
}
else
{
Expand All @@ -6769,9 +6741,19 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa
// TODO-Cleanup: We should really be expanding tailcalls into loops
// much earlier than this, at a place where we do not need to have
// hacky workarounds to figure out what the actual IL entry block is.
BasicBlock* firstILBB = fgGetFirstILBlock();
FlowEdge* const newEdge = fgAddRefPred(firstILBB, block);
block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge);
entryBB = fgGetFirstILBlock();
}

FlowEdge* const newEdge = fgAddRefPred(entryBB, block);
block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge);

// Update profile
if (block->hasProfileWeight() && entryBB->hasProfileWeight())
{
entryBB->increaseBBProfileWeight(block->bbWeight);
JITDUMP("Flow into entry BB " FMT_BB " increased. Data %s inconsistent.\n", entryBB->bbNum,
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

// Finish hooking things up.
Expand Down Expand Up @@ -12741,24 +12723,38 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block)
// modify the flow graph

// Find the actual jump target
size_t switchVal = (size_t)cond->AsIntCon()->gtIconVal;
unsigned jumpCnt = block->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTab = block->GetSwitchTargets()->bbsDstTab;
bool foundVal = false;
size_t switchVal = (size_t)cond->AsIntCon()->gtIconVal;
unsigned jumpCnt = block->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTab = block->GetSwitchTargets()->bbsDstTab;
bool foundVal = false;
bool profileInconsistent = false;

for (unsigned val = 0; val < jumpCnt; val++, jumpTab++)
{
FlowEdge* curEdge = *jumpTab;

assert(curEdge->getDestinationBlock()->countOfInEdges() > 0);

BasicBlock* const targetBlock = curEdge->getDestinationBlock();
if (block->hasProfileWeight() && targetBlock->hasProfileWeight())
{
targetBlock->decreaseBBProfileWeight(curEdge->getLikelyWeight());
profileInconsistent |= (targetBlock->NumSucc() > 0);
}

// If val matches switchVal or we are at the last entry and
// we never found the switch value then set the new jump dest

if ((val == switchVal) || (!foundVal && (val == jumpCnt - 1)))
{
block->SetKindAndTargetEdge(BBJ_ALWAYS, curEdge);
foundVal = true;

if (block->hasProfileWeight() && targetBlock->hasProfileWeight())
{
targetBlock->increaseBBProfileWeight(block->bbWeight);
profileInconsistent |= (targetBlock->NumSucc() > 0);
}
}
else
{
Expand All @@ -12767,6 +12763,13 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block)
}
}

if (profileInconsistent)
{
JITDUMP("Flow change out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n", block->bbNum,
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

assert(foundVal);
#ifdef DEBUG
if (verbose)
Expand Down Expand Up @@ -13408,6 +13411,11 @@ PhaseStatus Compiler::fgMorphBlocks()
//
fgGlobalMorph = true;

if (fgPgoConsistent)
{
Metrics.ProfileConsistentBeforeMorph = 1;
}

if (opts.OptimizationEnabled())
{
// Local assertion prop is enabled if we are optimizing.
Expand Down Expand Up @@ -13505,6 +13513,25 @@ PhaseStatus Compiler::fgMorphBlocks()
fgEntryBB->bbRefs--;
fgEntryBBExtraRefs = 0;

// The original method entry will now be checked for profile consistency.
// If the entry has inconsistent incoming weight, flag the profile as inconsistent.
//
if (fgEntryBB->hasProfileWeight())
{
weight_t incomingWeight = BB_ZERO_WEIGHT;
for (FlowEdge* const predEdge : fgEntryBB->PredEdges())
{
incomingWeight += predEdge->getLikelyWeight();
}

if (!fgProfileWeightsConsistent(incomingWeight, fgEntryBB->bbWeight))
{
JITDUMP("OSR: Original method entry " FMT_BB " has inconsistent weight. Data %s inconsistent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}
}

// We don't need to remember this block anymore.
fgEntryBB = nullptr;
}
Expand Down Expand Up @@ -13544,6 +13571,11 @@ PhaseStatus Compiler::fgMorphBlocks()
// may no longer be canonical.
fgCanonicalizeFirstBB();

if (fgPgoConsistent)
{
Metrics.ProfileConsistentAfterMorph = 1;
}

INDEBUG(fgPostGlobalMorphChecks();)

return PhaseStatus::MODIFIED_EVERYTHING;
Expand Down Expand Up @@ -14263,6 +14295,15 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt)
BasicBlock* condBlock = fgNewBBafter(BBJ_ALWAYS, block, true);
BasicBlock* elseBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true);

// Update flowgraph
fgRedirectTargetEdge(block, condBlock);
condBlock->SetTargetEdge(fgAddRefPred(elseBlock, condBlock));
elseBlock->SetTargetEdge(fgAddRefPred(remainderBlock, elseBlock));

// Propagate flow from block into condBlock.
// Leave flow out of remainderBlock intact, as it will post-dominate block.
condBlock->inheritWeight(block);

// These blocks are only internal if 'block' is (but they've been set as internal by fgNewBBafter).
// If they're not internal, mark them as imported to avoid asserts about un-imported blocks.
if (!block->HasFlag(BBF_INTERNAL))
Expand All @@ -14276,27 +14317,6 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt)
block->RemoveFlags(BBF_NEEDS_GCPOLL);
remainderBlock->SetFlags(propagateFlagsToRemainder | propagateFlagsToAll);

condBlock->inheritWeight(block);

// Make sure remainderBlock gets exactly the same weight as block after split
assert(condBlock->bbWeight == remainderBlock->bbWeight);

assert(block->KindIs(BBJ_ALWAYS));
fgRedirectTargetEdge(block, condBlock);

{
FlowEdge* const newEdge = fgAddRefPred(elseBlock, condBlock);
condBlock->SetTargetEdge(newEdge);
}

{
FlowEdge* const newEdge = fgAddRefPred(remainderBlock, elseBlock);
elseBlock->SetTargetEdge(newEdge);
}

assert(condBlock->JumpsToNext());
assert(elseBlock->JumpsToNext());

condBlock->SetFlags(propagateFlagsToAll);
elseBlock->SetFlags(propagateFlagsToAll);

Expand All @@ -14311,6 +14331,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt)
// +--->--------+
// bbj_cond(true)
//
// TODO: Remove unnecessary condition reversal
gtReverseCond(condExpr);

thenBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true);
Expand All @@ -14324,13 +14345,12 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt)
const unsigned thenLikelihood = qmark->ThenNodeLikelihood();
const unsigned elseLikelihood = qmark->ElseNodeLikelihood();

FlowEdge* const newEdge = fgAddRefPred(remainderBlock, thenBlock);
thenBlock->SetTargetEdge(newEdge);
thenBlock->SetTargetEdge(fgAddRefPred(remainderBlock, thenBlock));

assert(condBlock->TargetIs(elseBlock));
FlowEdge* const elseEdge = fgAddRefPred(thenBlock, condBlock);
FlowEdge* const thenEdge = condBlock->GetTargetEdge();
condBlock->SetCond(thenEdge, elseEdge);
FlowEdge* const thenEdge = fgAddRefPred(thenBlock, condBlock);
FlowEdge* const elseEdge = condBlock->GetTargetEdge();
condBlock->SetCond(elseEdge, thenEdge);
thenBlock->inheritWeightPercentage(condBlock, thenLikelihood);
elseBlock->inheritWeightPercentage(condBlock, elseLikelihood);
thenEdge->setLikelihood(thenLikelihood / 100.0);
Expand All @@ -14344,6 +14364,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt)
// +-->-------------+
// bbj_cond(true)
//
// TODO: Remove unnecessary condition reversal
gtReverseCond(condExpr);

const unsigned thenLikelihood = qmark->ThenNodeLikelihood();
Expand Down

0 comments on commit 845dc11

Please sign in to comment.