Skip to content

Commit

Permalink
JIT: Enable profile consistency checking up to morph (#111047)
Browse files Browse the repository at this point in the history 
Part of #107749.
  • Loading branch information
@amanasifkhalid
amanasifkhalid authored Jan 7, 2025
1 parent 2d7f45b commit aecae2c
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 89 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 @@ -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
View file
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
View file
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 @@ -6632,6 +6619,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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5119,7 +5119,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
View file
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

0 comments on commit aecae2c

Please sign in to comment.