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: verify full profile consistency after importation #100869

Merged
merged 7 commits into from
Apr 13, 2024
Merged

JIT: verify full profile consistency after importation #100869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b244d01
JIT: verify full profile consistency after importation
AndyAyersMS Apr 9, 2024
6e61a50
fix check
AndyAyersMS Apr 11, 2024
18ebada
Merge branch 'main' into BlockWeightConsistency1
AndyAyersMS Apr 11, 2024
823fd99
allow small count discrepancy
AndyAyersMS Apr 11, 2024
2b41492
handle folded switches
AndyAyersMS Apr 12, 2024
69c4a4a
fix OSR special case where method entry is loop header
AndyAyersMS Apr 12, 2024
748980f
handle reimported leave
AndyAyersMS Apr 12, 2024
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
15 changes: 6 additions & 9 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6148,6 +6148,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2942,11 +2942,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 @@ -4224,6 +4236,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
Open in desktop
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
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is premature, but do you think we should consider changing the block weight API surface sooner rather than later? It might be useful as we do these profile fixups to have something like incrementBBProfileWeight that asserts the increment amount is positive, and clears the BBF_RUN_RARELY flag (until we decide to decouple the meaning of this flag from BB_ZERO_WEIGHT) -- and maybe doesn't touch the BBF_PROF_WEIGHT flag?

I'm happy to add some new helpers like this, if you think they'd be useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should do that -- want to get a bit more experience with the updates first, so we can see what patterns are common.


// 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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto my above comment -- maybe something like decrementBBProfileWeight that does these underflow checks.


// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on the planned BasicBlock interface changes, I imagine this pattern will come up enough that we'll want to move the check into bbSetRunRarely (or do some similar cleanup).

}

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