From 208bcafbeebab83dbb0be29f217ac4250080857d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 Apr 2024 18:59:55 -0700 Subject: [PATCH] JIT: verify full profile consistency after importation (#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 #93020 --- src/coreclr/jit/compiler.cpp | 15 ++--- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/fgbasic.cpp | 22 +++++++ src/coreclr/jit/fgprofile.cpp | 50 +++++++++++++++- src/coreclr/jit/importer.cpp | 106 +++++++++++++++++++++++++++++----- 5 files changed, 168 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index cd2dfd6b1846a3..d534caf2d3adb2 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -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)) { @@ -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 |= @@ -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()) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index c280cdb025398f..8f15dbe77446ec 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -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); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 8947afbf93ff62..8cbecbee7ca2c1 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -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); @@ -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; + } } /***************************************************************************** diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 2d492c5f00ed4a..7a46e14155b116 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -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 //------------------------------------------------------------------------ @@ -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()) { @@ -5542,6 +5571,10 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks { incomingLikelyWeight += predEdge->getLikelyWeight(); } + else + { + foundEHPreds = true; + } } else { @@ -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) { @@ -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); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 242162e77e50cb..7f2fccf0f97dca 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -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; + } } /*****************************************************************************/ @@ -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) { @@ -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; + } } } @@ -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(); @@ -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);