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.

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

JIT: Move profile consistency checks to after morph #111253

Merged
merged 8 commits into from
Jan 14, 2025
Merged

JIT: Move profile consistency checks to after morph #111253

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
32e7735
Use block weight helpers in more places
amanasifkhalid Jan 8, 2025
57b7705
Move profile checks to after morph
amanasifkhalid Jan 9, 2025
3f5f514
Move profile checks to after post-morph
amanasifkhalid Jan 9, 2025
3e89cef
Add metrics
amanasifkhalid Jan 9, 2025
c9272ba
Remove whitespace change
amanasifkhalid Jan 9, 2025
40fdcae
Merge branch 'main' into profile-checks-morph
AndyAyersMS Jan 11, 2025
87b9d64
Merge branch 'main' into profile-checks-morph
amanasifkhalid Jan 13, 2025
bfa0672
Note if original method entry is inconsistent after morphing OSR method
amanasifkhalid Jan 14, 2025
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
10 changes: 5 additions & 5 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
150 changes: 76 additions & 74 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
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 @@ -13544,6 +13552,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 +14276,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 +14298,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 +14312,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 +14326,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 +14345,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
Loading