Skip to content

Commit

Permalink
JIT: verify full profile consistency after importation (dotnet#100869)
Browse files Browse the repository at this point in the history 
Move the full profile check down past the importer. Attempt local repair
for cases where the importer alters BBJ_COND. If that is unable to guarantee
consistency, mark the PGO data as inconsistent.

If the importer alters BBJ_SWITCH don't attempt repair, just mark the profile
as inconsistent.

If in an OSR method the original method entry is a loop header, and that is
not the loop that triggered OSR, mark the profile as inconsistent.

If the importer re-imports a LEAVE, there are still orphaned blocks left from
the first importation, these can mess up profiles. In that case, mark the
profile as inconsistent.

Exempt blocks with EH preds (catches, etc) from inbound checking, as
profile data propagation along EH edges is not modelled.

Modify the post-phase checks to allow either small relative errors or small
absolute errors, so that flow out of EH regions though intermediaries (say
step blocks) does not trip the checker.

Ensure the initial pass of likelihood adjustments pays attention to
throws. And only mark throws as rare in the importer if we have not
synthesized profile data (which may in fact tell us the throw is not cold).

Contributes to dotnet#93020
  • Loading branch information
@AndyAyersMS @Ruihan-Yin
AndyAyersMS authored and Ruihan-Yin committed May 30, 2024
1 parent eedef21 commit 208bcaf
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 26 deletions.
15 changes: 6 additions & 9 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2790,10 +2790,7 @@ void Compiler::compInitOptions(JitFlags* jitFlags)
fgPgoSource = ICorJitInfo::PgoSource::Unknown;
fgPgoHaveWeights = false;
fgPgoSynthesized = false;

#ifdef DEBUG
fgPgoConsistent = false;
#endif
fgPgoConsistent = false;

if (jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT))
{
Expand Down Expand Up @@ -4623,11 +4620,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
fgFixEntryFlowForOSR();
}

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

// Enable the post-phase checks that use internal logic to decide when checking makes sense.
//
activePhaseChecks |=
Expand All @@ -4637,6 +4629,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_IMPORTATION, &Compiler::fgImport);

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

// If this is a failed inline attempt, we're done.
//
if (compIsForInlining() && compInlineResult->IsFailure())
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6152,6 +6152,7 @@ class Compiler

static bool fgProfileWeightsEqual(weight_t weight1, weight_t weight2, weight_t epsilon = 0.01);
static bool fgProfileWeightsConsistent(weight_t weight1, weight_t weight2);
static bool fgProfileWeightsConsistentOrSmall(weight_t weight1, weight_t weight2, weight_t epsilon = 1e-4);

static GenTree* fgGetFirstNode(GenTree* tree);

Expand Down
22 changes: 22 additions & 0 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2944,11 +2944,23 @@ void Compiler::fgLinkBasicBlocks()
curBBdesc->SetTrueEdge(trueEdge);
curBBdesc->SetFalseEdge(falseEdge);

// Avoid making BBJ_THROW successors look likely, if possible.
//
if (trueEdge == falseEdge)
{
assert(trueEdge->getDupCount() == 2);
trueEdge->setLikelihood(1.0);
}
else if (trueTarget->KindIs(BBJ_THROW) && !falseTarget->KindIs(BBJ_THROW))
{
trueEdge->setLikelihood(0.0);
falseEdge->setLikelihood(1.0);
}
else if (!trueTarget->KindIs(BBJ_THROW) && falseTarget->KindIs(BBJ_THROW))
{
trueEdge->setLikelihood(1.0);
falseEdge->setLikelihood(0.0);
}
else
{
trueEdge->setLikelihood(0.5);
Expand Down Expand Up @@ -4226,6 +4238,16 @@ void Compiler::fgFixEntryFlowForOSR()

JITDUMP("OSR: redirecting flow at method entry from " FMT_BB " to OSR entry " FMT_BB " for the importer\n",
fgFirstBB->bbNum, fgOSREntryBB->bbNum);

// If the original entry block still has preds, it is a loop header, and is not
// the OSR entry, when we change the flow above we've made profile inconsistent.
//
if ((fgEntryBB->bbPreds != nullptr) && (fgEntryBB != fgOSREntryBB))
{
JITDUMP("OSR: profile data could not be locally repaired. Data %s inconsisent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}
}

/*****************************************************************************
Expand Down
50 changes: 48 additions & 2 deletions src/coreclr/jit/fgprofile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5222,6 +5222,34 @@ bool Compiler::fgProfileWeightsConsistent(weight_t weight1, weight_t weight2)
return fgProfileWeightsEqual(relativeDiff, BB_ZERO_WEIGHT);
}

//------------------------------------------------------------------------
// fgProfileWeightsConsistentOrSmall: check if two profile weights are within
// some small percentage of one another, or are both less than some epsilon.
//
// Arguments:
// weight1 -- first weight
// weight2 -- second weight
// epsilon -- small weight threshold
//
bool Compiler::fgProfileWeightsConsistentOrSmall(weight_t weight1, weight_t weight2, weight_t epsilon)
{
if (weight2 == BB_ZERO_WEIGHT)
{
return fgProfileWeightsEqual(weight1, weight2, epsilon);
}

weight_t const delta = fabs(weight2 - weight1);

if (delta <= epsilon)
{
return true;
}

weight_t const relativeDelta = delta / weight2;

return fgProfileWeightsEqual(relativeDelta, BB_ZERO_WEIGHT);
}

#ifdef DEBUG

//------------------------------------------------------------------------
Expand Down Expand Up @@ -5531,6 +5559,7 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
weight_t incomingLikelyWeight = 0;
unsigned missingLikelyWeight = 0;
bool foundPreds = false;
bool foundEHPreds = false;

for (FlowEdge* const predEdge : block->PredEdges())
{
Expand All @@ -5542,6 +5571,10 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
{
incomingLikelyWeight += predEdge->getLikelyWeight();
}
else
{
foundEHPreds = true;
}
}
else
{
Expand All @@ -5553,10 +5586,23 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
foundPreds = true;
}

// We almost certainly won't get the likelihoods on a BBJ_EHFINALLYRET right,
// so special-case BBJ_CALLFINALLYRET incoming flow.
//
if (block->isBBCallFinallyPairTail())
{
incomingLikelyWeight = block->Prev()->bbWeight;
incomingWeightMin = incomingLikelyWeight;
incomingWeightMax = incomingLikelyWeight;
foundEHPreds = false;
}

bool classicWeightsValid = true;
bool likelyWeightsValid = true;

if (foundPreds)
// If we have EH preds we may not have consistent incoming flow.
//
if (foundPreds && !foundEHPreds)
{
if (verifyClassicWeights)
{
Expand Down Expand Up @@ -5584,7 +5630,7 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks

if (verifyLikelyWeights)
{
if (!fgProfileWeightsConsistent(blockWeight, incomingLikelyWeight))
if (!fgProfileWeightsConsistentOrSmall(blockWeight, incomingLikelyWeight))
{
JITDUMP(" " FMT_BB " - block weight " FMT_WT " inconsistent with incoming likely weight " FMT_WT "\n",
block->bbNum, blockWeight, incomingLikelyWeight);
Expand Down
106 changes: 91 additions & 15 deletions src/coreclr/jit/importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5128,6 +5128,18 @@ void Compiler::impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr)
// reason we don't want to remove the block at this point is that if we call
// fgInitBBLookup() again we will do it wrong as the BBJ_ALWAYS block won't be
// added and the linked list length will be different than fgBBcount.
//
// Because of this incomplete cleanup. profile data may be left inconsistent.
//
if (block->hasProfileWeight())
{
// We are unlikely to be able to repair the profile.
// For now we don't even try.
//
JITDUMP("\nimpResetLeaveBlock: Profile data could not be locally repaired. Data %s inconsisent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}
}

/*****************************************************************************/
Expand Down Expand Up @@ -6606,7 +6618,11 @@ void Compiler::impImportBlockCode(BasicBlock* block)
return;
}

block->bbSetRunRarely(); // filters are rare
if (!fgPgoSynthesized)
{
// filters are rare
block->bbSetRunRarely();
}

if (info.compXcptnsCount == 0)
{
Expand Down Expand Up @@ -7294,19 +7310,67 @@ void Compiler::impImportBlockCode(BasicBlock* block)

if (block->KindIs(BBJ_COND))
{
if (op1->AsIntCon()->gtIconVal)
{
JITDUMP("\nThe conditional jump becomes an unconditional jump to " FMT_BB "\n",
block->GetTrueTarget()->bbNum);
fgRemoveRefPred(block->GetFalseEdge());
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetTrueEdge());
}
else
bool const isCondTrue = op1->AsIntCon()->gtIconVal != 0;
FlowEdge* const removedEdge = isCondTrue ? block->GetFalseEdge() : block->GetTrueEdge();
FlowEdge* const retainedEdge = isCondTrue ? block->GetTrueEdge() : block->GetFalseEdge();

JITDUMP("\nThe conditional jump becomes an unconditional jump to " FMT_BB "\n",
retainedEdge->getDestinationBlock()->bbNum);

fgRemoveRefPred(removedEdge);
block->SetKindAndTargetEdge(BBJ_ALWAYS, retainedEdge);

// If we removed an edge carrying profile, try to do a local repair.
//
if (block->hasProfileWeight())
{
assert(block->NextIs(block->GetFalseTarget()));
JITDUMP("\nThe block jumps to the next " FMT_BB "\n", block->Next()->bbNum);
fgRemoveRefPred(block->GetTrueEdge());
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetFalseEdge());
bool repairWasComplete = true;
weight_t const weight = removedEdge->getLikelyWeight();

if (weight > 0)
{
// Target block weight will increase.
//
BasicBlock* const target = block->GetTarget();
assert(target->hasProfileWeight());
target->setBBProfileWeight(target->bbWeight + weight);

// Alternate weight will decrease
//
BasicBlock* const alternate = removedEdge->getDestinationBlock();
assert(alternate->hasProfileWeight());
weight_t const alternateNewWeight = alternate->bbWeight - weight;

// If profile weights are consistent, expect at worst a slight underflow.
//
if (fgPgoConsistent && (alternateNewWeight < 0))
{
assert(fgProfileWeightsEqual(alternateNewWeight, 0));
}
alternate->setBBProfileWeight(max(0.0, alternateNewWeight));

// This will affect profile transitively, so in general
// the profile will become inconsistent.
//
repairWasComplete = false;

// But we can check for the special case where the
// block's postdominator is target's target (simple
// if/then/else/join).
//
if (target->KindIs(BBJ_ALWAYS))
{
repairWasComplete = alternate->KindIs(BBJ_ALWAYS) &&
(alternate->GetTarget() == target->GetTarget());
}
}

if (!repairWasComplete)
{
JITDUMP("Profile data could not be locally repaired. Data %s inconsistent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}
}
}

Expand Down Expand Up @@ -7584,6 +7648,15 @@ void Compiler::impImportBlockCode(BasicBlock* block)
printf("\n");
}
#endif
if (block->hasProfileWeight())
{
// We are unlikely to be able to repair the profile.
// For now we don't even try.
//
JITDUMP("Profile data could not be locally repaired. Data %s inconsisent.\n",
fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

// Create a NOP node
op1 = gtNewNothingNode();
Expand Down Expand Up @@ -10065,8 +10138,11 @@ void Compiler::impImportBlockCode(BasicBlock* block)

case CEE_THROW:

// Any block with a throw is rarely executed.
block->bbSetRunRarely();
if (!fgPgoSynthesized)
{
// Any block with a throw is rarely executed.
block->bbSetRunRarely();
}

// Pop the exception object and create the 'throw' helper call
op1 = gtNewHelperCallNode(CORINFO_HELP_THROW, TYP_VOID, impPopStack().val);
Expand Down

0 comments on commit 208bcaf

Please sign in to comment.