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);