Skip to content

Commit caaf7c2

Browse files
AndyAyersMSmatouskozak
AndyAyersMS
authored and
matouskozak
committed
JIT: verify full profile consistency after importation (dotnet#100869)
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
1 parent 2f0caa9 commit caaf7c2

File tree

5 files changed

+168
-26
lines changed

5 files changed

+168
-26
lines changed

‎src/coreclr/jit/compiler.cpp

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2790,10 +2790,7 @@ void Compiler::compInitOptions(JitFlags* jitFlags)
27902790
fgPgoSource = ICorJitInfo::PgoSource::Unknown;
27912791
fgPgoHaveWeights = false;
27922792
fgPgoSynthesized = false;
2793-
2794-
#ifdef DEBUG
2795-
fgPgoConsistent = false;
2796-
#endif
2793+
fgPgoConsistent = false;
27972794

27982795
if (jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT))
27992796
{
@@ -4623,11 +4620,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
46234620
fgFixEntryFlowForOSR();
46244621
}
46254622

4626-
// Drop back to just checking profile likelihoods.
4627-
//
4628-
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;
4629-
activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS;
4630-
46314623
// Enable the post-phase checks that use internal logic to decide when checking makes sense.
46324624
//
46334625
activePhaseChecks |=
@@ -4637,6 +4629,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
46374629
//
46384630
DoPhase(this, PHASE_IMPORTATION, &Compiler::fgImport);
46394631

4632+
// Drop back to just checking profile likelihoods.
4633+
//
4634+
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;
4635+
activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS;
4636+
46404637
// If this is a failed inline attempt, we're done.
46414638
//
46424639
if (compIsForInlining() && compInlineResult->IsFailure())

‎src/coreclr/jit/compiler.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6152,6 +6152,7 @@ class Compiler
61526152

61536153
static bool fgProfileWeightsEqual(weight_t weight1, weight_t weight2, weight_t epsilon = 0.01);
61546154
static bool fgProfileWeightsConsistent(weight_t weight1, weight_t weight2);
6155+
static bool fgProfileWeightsConsistentOrSmall(weight_t weight1, weight_t weight2, weight_t epsilon = 1e-4);
61556156

61566157
static GenTree* fgGetFirstNode(GenTree* tree);
61576158

‎src/coreclr/jit/fgbasic.cpp

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2944,11 +2944,23 @@ void Compiler::fgLinkBasicBlocks()
29442944
curBBdesc->SetTrueEdge(trueEdge);
29452945
curBBdesc->SetFalseEdge(falseEdge);
29462946

2947+
// Avoid making BBJ_THROW successors look likely, if possible.
2948+
//
29472949
if (trueEdge == falseEdge)
29482950
{
29492951
assert(trueEdge->getDupCount() == 2);
29502952
trueEdge->setLikelihood(1.0);
29512953
}
2954+
else if (trueTarget->KindIs(BBJ_THROW) && !falseTarget->KindIs(BBJ_THROW))
2955+
{
2956+
trueEdge->setLikelihood(0.0);
2957+
falseEdge->setLikelihood(1.0);
2958+
}
2959+
else if (!trueTarget->KindIs(BBJ_THROW) && falseTarget->KindIs(BBJ_THROW))
2960+
{
2961+
trueEdge->setLikelihood(1.0);
2962+
falseEdge->setLikelihood(0.0);
2963+
}
29522964
else
29532965
{
29542966
trueEdge->setLikelihood(0.5);
@@ -4226,6 +4238,16 @@ void Compiler::fgFixEntryFlowForOSR()
42264238

42274239
JITDUMP("OSR: redirecting flow at method entry from " FMT_BB " to OSR entry " FMT_BB " for the importer\n",
42284240
fgFirstBB->bbNum, fgOSREntryBB->bbNum);
4241+
4242+
// If the original entry block still has preds, it is a loop header, and is not
4243+
// the OSR entry, when we change the flow above we've made profile inconsistent.
4244+
//
4245+
if ((fgEntryBB->bbPreds != nullptr) && (fgEntryBB != fgOSREntryBB))
4246+
{
4247+
JITDUMP("OSR: profile data could not be locally repaired. Data %s inconsisent.\n",
4248+
fgPgoConsistent ? "is now" : "was already");
4249+
fgPgoConsistent = false;
4250+
}
42294251
}
42304252

42314253
/*****************************************************************************

‎src/coreclr/jit/fgprofile.cpp

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5222,6 +5222,34 @@ bool Compiler::fgProfileWeightsConsistent(weight_t weight1, weight_t weight2)
52225222
return fgProfileWeightsEqual(relativeDiff, BB_ZERO_WEIGHT);
52235223
}
52245224

5225+
//------------------------------------------------------------------------
5226+
// fgProfileWeightsConsistentOrSmall: check if two profile weights are within
5227+
// some small percentage of one another, or are both less than some epsilon.
5228+
//
5229+
// Arguments:
5230+
// weight1 -- first weight
5231+
// weight2 -- second weight
5232+
// epsilon -- small weight threshold
5233+
//
5234+
bool Compiler::fgProfileWeightsConsistentOrSmall(weight_t weight1, weight_t weight2, weight_t epsilon)
5235+
{
5236+
if (weight2 == BB_ZERO_WEIGHT)
5237+
{
5238+
return fgProfileWeightsEqual(weight1, weight2, epsilon);
5239+
}
5240+
5241+
weight_t const delta = fabs(weight2 - weight1);
5242+
5243+
if (delta <= epsilon)
5244+
{
5245+
return true;
5246+
}
5247+
5248+
weight_t const relativeDelta = delta / weight2;
5249+
5250+
return fgProfileWeightsEqual(relativeDelta, BB_ZERO_WEIGHT);
5251+
}
5252+
52255253
#ifdef DEBUG
52265254

52275255
//------------------------------------------------------------------------
@@ -5531,6 +5559,7 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
55315559
weight_t incomingLikelyWeight = 0;
55325560
unsigned missingLikelyWeight = 0;
55335561
bool foundPreds = false;
5562+
bool foundEHPreds = false;
55345563

55355564
for (FlowEdge* const predEdge : block->PredEdges())
55365565
{
@@ -5542,6 +5571,10 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
55425571
{
55435572
incomingLikelyWeight += predEdge->getLikelyWeight();
55445573
}
5574+
else
5575+
{
5576+
foundEHPreds = true;
5577+
}
55455578
}
55465579
else
55475580
{
@@ -5553,10 +5586,23 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
55535586
foundPreds = true;
55545587
}
55555588

5589+
// We almost certainly won't get the likelihoods on a BBJ_EHFINALLYRET right,
5590+
// so special-case BBJ_CALLFINALLYRET incoming flow.
5591+
//
5592+
if (block->isBBCallFinallyPairTail())
5593+
{
5594+
incomingLikelyWeight = block->Prev()->bbWeight;
5595+
incomingWeightMin = incomingLikelyWeight;
5596+
incomingWeightMax = incomingLikelyWeight;
5597+
foundEHPreds = false;
5598+
}
5599+
55565600
bool classicWeightsValid = true;
55575601
bool likelyWeightsValid = true;
55585602

5559-
if (foundPreds)
5603+
// If we have EH preds we may not have consistent incoming flow.
5604+
//
5605+
if (foundPreds && !foundEHPreds)
55605606
{
55615607
if (verifyClassicWeights)
55625608
{
@@ -5584,7 +5630,7 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
55845630

55855631
if (verifyLikelyWeights)
55865632
{
5587-
if (!fgProfileWeightsConsistent(blockWeight, incomingLikelyWeight))
5633+
if (!fgProfileWeightsConsistentOrSmall(blockWeight, incomingLikelyWeight))
55885634
{
55895635
JITDUMP(" " FMT_BB " - block weight " FMT_WT " inconsistent with incoming likely weight " FMT_WT "\n",
55905636
block->bbNum, blockWeight, incomingLikelyWeight);

‎src/coreclr/jit/importer.cpp

Lines changed: 91 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5128,6 +5128,18 @@ void Compiler::impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr)
51285128
// reason we don't want to remove the block at this point is that if we call
51295129
// fgInitBBLookup() again we will do it wrong as the BBJ_ALWAYS block won't be
51305130
// added and the linked list length will be different than fgBBcount.
5131+
//
5132+
// Because of this incomplete cleanup. profile data may be left inconsistent.
5133+
//
5134+
if (block->hasProfileWeight())
5135+
{
5136+
// We are unlikely to be able to repair the profile.
5137+
// For now we don't even try.
5138+
//
5139+
JITDUMP("\nimpResetLeaveBlock: Profile data could not be locally repaired. Data %s inconsisent.\n",
5140+
fgPgoConsistent ? "is now" : "was already");
5141+
fgPgoConsistent = false;
5142+
}
51315143
}
51325144

51335145
/*****************************************************************************/
@@ -6606,7 +6618,11 @@ void Compiler::impImportBlockCode(BasicBlock* block)
66066618
return;
66076619
}
66086620

6609-
block->bbSetRunRarely(); // filters are rare
6621+
if (!fgPgoSynthesized)
6622+
{
6623+
// filters are rare
6624+
block->bbSetRunRarely();
6625+
}
66106626

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

72957311
if (block->KindIs(BBJ_COND))
72967312
{
7297-
if (op1->AsIntCon()->gtIconVal)
7298-
{
7299-
JITDUMP("\nThe conditional jump becomes an unconditional jump to " FMT_BB "\n",
7300-
block->GetTrueTarget()->bbNum);
7301-
fgRemoveRefPred(block->GetFalseEdge());
7302-
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetTrueEdge());
7303-
}
7304-
else
7313+
bool const isCondTrue = op1->AsIntCon()->gtIconVal != 0;
7314+
FlowEdge* const removedEdge = isCondTrue ? block->GetFalseEdge() : block->GetTrueEdge();
7315+
FlowEdge* const retainedEdge = isCondTrue ? block->GetTrueEdge() : block->GetFalseEdge();
7316+
7317+
JITDUMP("\nThe conditional jump becomes an unconditional jump to " FMT_BB "\n",
7318+
retainedEdge->getDestinationBlock()->bbNum);
7319+
7320+
fgRemoveRefPred(removedEdge);
7321+
block->SetKindAndTargetEdge(BBJ_ALWAYS, retainedEdge);
7322+
7323+
// If we removed an edge carrying profile, try to do a local repair.
7324+
//
7325+
if (block->hasProfileWeight())
73057326
{
7306-
assert(block->NextIs(block->GetFalseTarget()));
7307-
JITDUMP("\nThe block jumps to the next " FMT_BB "\n", block->Next()->bbNum);
7308-
fgRemoveRefPred(block->GetTrueEdge());
7309-
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetFalseEdge());
7327+
bool repairWasComplete = true;
7328+
weight_t const weight = removedEdge->getLikelyWeight();
7329+
7330+
if (weight > 0)
7331+
{
7332+
// Target block weight will increase.
7333+
//
7334+
BasicBlock* const target = block->GetTarget();
7335+
assert(target->hasProfileWeight());
7336+
target->setBBProfileWeight(target->bbWeight + weight);
7337+
7338+
// Alternate weight will decrease
7339+
//
7340+
BasicBlock* const alternate = removedEdge->getDestinationBlock();
7341+
assert(alternate->hasProfileWeight());
7342+
weight_t const alternateNewWeight = alternate->bbWeight - weight;
7343+
7344+
// If profile weights are consistent, expect at worst a slight underflow.
7345+
//
7346+
if (fgPgoConsistent && (alternateNewWeight < 0))
7347+
{
7348+
assert(fgProfileWeightsEqual(alternateNewWeight, 0));
7349+
}
7350+
alternate->setBBProfileWeight(max(0.0, alternateNewWeight));
7351+
7352+
// This will affect profile transitively, so in general
7353+
// the profile will become inconsistent.
7354+
//
7355+
repairWasComplete = false;
7356+
7357+
// But we can check for the special case where the
7358+
// block's postdominator is target's target (simple
7359+
// if/then/else/join).
7360+
//
7361+
if (target->KindIs(BBJ_ALWAYS))
7362+
{
7363+
repairWasComplete = alternate->KindIs(BBJ_ALWAYS) &&
7364+
(alternate->GetTarget() == target->GetTarget());
7365+
}
7366+
}
7367+
7368+
if (!repairWasComplete)
7369+
{
7370+
JITDUMP("Profile data could not be locally repaired. Data %s inconsistent.\n",
7371+
fgPgoConsistent ? "is now" : "was already");
7372+
fgPgoConsistent = false;
7373+
}
73107374
}
73117375
}
73127376

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

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

1006610139
case CEE_THROW:
1006710140

10068-
// Any block with a throw is rarely executed.
10069-
block->bbSetRunRarely();
10141+
if (!fgPgoSynthesized)
10142+
{
10143+
// Any block with a throw is rarely executed.
10144+
block->bbSetRunRarely();
10145+
}
1007010146

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

0 commit comments

Comments
 (0)