diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 8385050b517bb4..5aab07895a7b85 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4779,11 +4779,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_MORPH_IMPBYREF, &Compiler::fgRetypeImplicitByRefArgs); - // Drop back to just checking profile likelihoods. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS; - #ifdef DEBUG // Now that locals have address-taken and implicit byref marked, we can safely apply stress. lvaStressLclFld(); @@ -4819,6 +4814,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_GS_COOKIE, &Compiler::gsPhase); + // Drop back to just checking profile likelihoods. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS; + if (opts.OptimizationEnabled()) { // Compute the block weights diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 652a2d434380b1..a34e2622157ba3 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -156,14 +156,29 @@ void Compiler::fgConvertBBToThrowBB(BasicBlock* block) } // Scrub this block from the pred lists of any successors - fgRemoveBlockAsPred(block); + bool profileInconsistent = false; + + for (BasicBlock* const succBlock : block->Succs(this)) + { + FlowEdge* const succEdge = fgRemoveAllRefPreds(succBlock, block); + + if (block->hasProfileWeight() && succBlock->hasProfileWeight()) + { + succBlock->decreaseBBProfileWeight(succEdge->getLikelyWeight()); + profileInconsistent |= (succBlock->NumSucc() > 0); + } + } + + if (profileInconsistent) + { + JITDUMP("Flow removal of " FMT_BB " needs to be propagated. Data %s inconsistent.\n", block->bbNum, + fgPgoConsistent ? "is now" : "was already"); + fgPgoConsistent = false; + } // Update jump kind after the scrub. block->SetKindAndTargetEdge(BBJ_THROW); block->RemoveFlags(BBF_RETLESS_CALL); // no longer a BBJ_CALLFINALLY - - // Any block with a throw is rare - block->bbSetRunRarely(); } /***************************************************************************** diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index b7ecbd51176cdc..2c8504ea0a8095 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2433,7 +2433,7 @@ PhaseStatus Compiler::fgTailMergeThrows() if (canonicalBlock->hasProfileWeight()) { - canonicalBlock->setBBProfileWeight(canonicalBlock->bbWeight + removedWeight); + canonicalBlock->increaseBBProfileWeight(removedWeight); modifiedProfile = true; // Don't bother updating flow into nonCanonicalBlock, since it is now unreachable diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index c214a83152c9e7..a55770ea1fe29b 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -1355,7 +1355,7 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc // if (bDest->hasProfileWeight()) { - bDest->setBBProfileWeight(max(0.0, bDest->bbWeight - removedWeight)); + bDest->decreaseBBProfileWeight(removedWeight); } return true; @@ -1620,7 +1620,7 @@ bool Compiler::fgOptimizeSwitchBranches(BasicBlock* block) if (bDest->hasProfileWeight()) { weight_t const branchThroughWeight = oldEdge->getLikelyWeight(); - bDest->setBBProfileWeight(max(0.0, bDest->bbWeight - branchThroughWeight)); + bDest->decreaseBBProfileWeight(branchThroughWeight); } // Update the switch jump table @@ -6633,7 +6633,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // crossJumpTarget, so the profile update can be done locally. if (crossJumpTarget->hasProfileWeight()) { - crossJumpTarget->setBBProfileWeight(crossJumpTarget->bbWeight + predBlock->bbWeight); + crossJumpTarget->increaseBBProfileWeight(predBlock->bbWeight); } } diff --git a/src/coreclr/jit/jitmetadatalist.h b/src/coreclr/jit/jitmetadatalist.h index 7ba0d732ab8645..db0f66f3f2c97e 100644 --- a/src/coreclr/jit/jitmetadatalist.h +++ b/src/coreclr/jit/jitmetadatalist.h @@ -68,6 +68,8 @@ JITMETADATAMETRIC(InlineAttempt, int, 0) JITMETADATAMETRIC(InlineCount, int, 0) JITMETADATAMETRIC(ProfileConsistentBeforeInline, int, 0) JITMETADATAMETRIC(ProfileConsistentAfterInline, int, 0) +JITMETADATAMETRIC(ProfileConsistentBeforeMorph, int, 0) +JITMETADATAMETRIC(ProfileConsistentAfterMorph, int, 0) JITMETADATAMETRIC(ProfileSynthesizedBlendedOrRepaired, int, 0) JITMETADATAMETRIC(ProfileInconsistentInitially, int, 0) JITMETADATAMETRIC(ProfileInconsistentResetLeave, int, 0) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 4555b1dc7560fa..944b09fc0490ae 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5366,55 +5366,27 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call) // if (compCurBB->KindIs(BBJ_ALWAYS)) { + BasicBlock* const curBlock = compCurBB; + BasicBlock* const targetBlock = curBlock->GetTarget(); + // Flow no longer reaches the target from here. // - fgRemoveRefPred(compCurBB->GetTargetEdge()); + fgRemoveRefPred(curBlock->GetTargetEdge()); - // Adjust profile weights of the successor blocks. + // Adjust profile weights of the successor block. // // Note if this is a tail call to loop, further updates // are needed once we install the loop edge. // - BasicBlock* curBlock = compCurBB; - if (curBlock->hasProfileWeight()) + if (curBlock->hasProfileWeight() && targetBlock->hasProfileWeight()) { - weight_t weightLoss = curBlock->bbWeight; - BasicBlock* nextBlock = curBlock->GetTarget(); + targetBlock->decreaseBBProfileWeight(curBlock->bbWeight); - while (nextBlock->hasProfileWeight()) + if (targetBlock->NumSucc() > 0) { - // Since we have linear flow we can update the next block weight. - // - weight_t const nextWeight = nextBlock->bbWeight; - weight_t const newNextWeight = nextWeight - weightLoss; - - // If the math would result in a negative weight then there's - // no local repair we can do; just leave things inconsistent. - // - if (newNextWeight >= 0) - { - // Note if we'd already morphed the IR in nextblock we might - // have done something profile sensitive that we should arguably reconsider. - // - JITDUMP("Reducing profile weight of " FMT_BB " from " FMT_WT " to " FMT_WT "\n", nextBlock->bbNum, - nextWeight, newNextWeight); - - nextBlock->setBBProfileWeight(newNextWeight); - } - else - { - JITDUMP("Not reducing profile weight of " FMT_BB " as its weight " FMT_WT - " is less than direct flow pred " FMT_BB " weight " FMT_WT "\n", - nextBlock->bbNum, nextWeight, compCurBB->bbNum, weightLoss); - } - - if (!nextBlock->KindIs(BBJ_ALWAYS)) - { - break; - } - - curBlock = nextBlock; - nextBlock = curBlock->GetTarget(); + JITDUMP("Flow removal out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n", + curBlock->bbNum, fgPgoConsistent ? "is now" : "was already"); + fgPgoConsistent = false; } } } @@ -6755,12 +6727,12 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa fgRemoveStmt(block, lastStmt); // Set the loop edge. + BasicBlock* entryBB; if (opts.IsOSR()) { // Todo: this may not look like a viable loop header. // Might need the moral equivalent of an init BB. - FlowEdge* const newEdge = fgAddRefPred(fgEntryBB, block); - block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + entryBB = fgEntryBB; } else { @@ -6769,9 +6741,19 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa // TODO-Cleanup: We should really be expanding tailcalls into loops // much earlier than this, at a place where we do not need to have // hacky workarounds to figure out what the actual IL entry block is. - BasicBlock* firstILBB = fgGetFirstILBlock(); - FlowEdge* const newEdge = fgAddRefPred(firstILBB, block); - block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + entryBB = fgGetFirstILBlock(); + } + + FlowEdge* const newEdge = fgAddRefPred(entryBB, block); + block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); + + // Update profile + if (block->hasProfileWeight() && entryBB->hasProfileWeight()) + { + entryBB->increaseBBProfileWeight(block->bbWeight); + JITDUMP("Flow into entry BB " FMT_BB " increased. Data %s inconsistent.\n", entryBB->bbNum, + fgPgoConsistent ? "is now" : "was already"); + fgPgoConsistent = false; } // Finish hooking things up. @@ -12741,10 +12723,11 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block) // modify the flow graph // Find the actual jump target - size_t switchVal = (size_t)cond->AsIntCon()->gtIconVal; - unsigned jumpCnt = block->GetSwitchTargets()->bbsCount; - FlowEdge** jumpTab = block->GetSwitchTargets()->bbsDstTab; - bool foundVal = false; + size_t switchVal = (size_t)cond->AsIntCon()->gtIconVal; + unsigned jumpCnt = block->GetSwitchTargets()->bbsCount; + FlowEdge** jumpTab = block->GetSwitchTargets()->bbsDstTab; + bool foundVal = false; + bool profileInconsistent = false; for (unsigned val = 0; val < jumpCnt; val++, jumpTab++) { @@ -12752,6 +12735,13 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block) assert(curEdge->getDestinationBlock()->countOfInEdges() > 0); + BasicBlock* const targetBlock = curEdge->getDestinationBlock(); + if (block->hasProfileWeight() && targetBlock->hasProfileWeight()) + { + targetBlock->decreaseBBProfileWeight(curEdge->getLikelyWeight()); + profileInconsistent |= (targetBlock->NumSucc() > 0); + } + // If val matches switchVal or we are at the last entry and // we never found the switch value then set the new jump dest @@ -12759,6 +12749,12 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block) { block->SetKindAndTargetEdge(BBJ_ALWAYS, curEdge); foundVal = true; + + if (block->hasProfileWeight() && targetBlock->hasProfileWeight()) + { + targetBlock->increaseBBProfileWeight(block->bbWeight); + profileInconsistent |= (targetBlock->NumSucc() > 0); + } } else { @@ -12767,6 +12763,13 @@ Compiler::FoldResult Compiler::fgFoldConditional(BasicBlock* block) } } + if (profileInconsistent) + { + JITDUMP("Flow change out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n", block->bbNum, + fgPgoConsistent ? "is now" : "was already"); + fgPgoConsistent = false; + } + assert(foundVal); #ifdef DEBUG if (verbose) @@ -13408,6 +13411,11 @@ PhaseStatus Compiler::fgMorphBlocks() // fgGlobalMorph = true; + if (fgPgoConsistent) + { + Metrics.ProfileConsistentBeforeMorph = 1; + } + if (opts.OptimizationEnabled()) { // Local assertion prop is enabled if we are optimizing. @@ -13505,6 +13513,25 @@ PhaseStatus Compiler::fgMorphBlocks() fgEntryBB->bbRefs--; fgEntryBBExtraRefs = 0; + // The original method entry will now be checked for profile consistency. + // If the entry has inconsistent incoming weight, flag the profile as inconsistent. + // + if (fgEntryBB->hasProfileWeight()) + { + weight_t incomingWeight = BB_ZERO_WEIGHT; + for (FlowEdge* const predEdge : fgEntryBB->PredEdges()) + { + incomingWeight += predEdge->getLikelyWeight(); + } + + if (!fgProfileWeightsConsistent(incomingWeight, fgEntryBB->bbWeight)) + { + JITDUMP("OSR: Original method entry " FMT_BB " has inconsistent weight. Data %s inconsistent.\n", + fgPgoConsistent ? "is now" : "was already"); + fgPgoConsistent = false; + } + } + // We don't need to remember this block anymore. fgEntryBB = nullptr; } @@ -13544,6 +13571,11 @@ PhaseStatus Compiler::fgMorphBlocks() // may no longer be canonical. fgCanonicalizeFirstBB(); + if (fgPgoConsistent) + { + Metrics.ProfileConsistentAfterMorph = 1; + } + INDEBUG(fgPostGlobalMorphChecks();) return PhaseStatus::MODIFIED_EVERYTHING; @@ -14263,6 +14295,15 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) BasicBlock* condBlock = fgNewBBafter(BBJ_ALWAYS, block, true); BasicBlock* elseBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true); + // Update flowgraph + fgRedirectTargetEdge(block, condBlock); + condBlock->SetTargetEdge(fgAddRefPred(elseBlock, condBlock)); + elseBlock->SetTargetEdge(fgAddRefPred(remainderBlock, elseBlock)); + + // Propagate flow from block into condBlock. + // Leave flow out of remainderBlock intact, as it will post-dominate block. + condBlock->inheritWeight(block); + // These blocks are only internal if 'block' is (but they've been set as internal by fgNewBBafter). // If they're not internal, mark them as imported to avoid asserts about un-imported blocks. if (!block->HasFlag(BBF_INTERNAL)) @@ -14276,27 +14317,6 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) block->RemoveFlags(BBF_NEEDS_GCPOLL); remainderBlock->SetFlags(propagateFlagsToRemainder | propagateFlagsToAll); - condBlock->inheritWeight(block); - - // Make sure remainderBlock gets exactly the same weight as block after split - assert(condBlock->bbWeight == remainderBlock->bbWeight); - - assert(block->KindIs(BBJ_ALWAYS)); - fgRedirectTargetEdge(block, condBlock); - - { - FlowEdge* const newEdge = fgAddRefPred(elseBlock, condBlock); - condBlock->SetTargetEdge(newEdge); - } - - { - FlowEdge* const newEdge = fgAddRefPred(remainderBlock, elseBlock); - elseBlock->SetTargetEdge(newEdge); - } - - assert(condBlock->JumpsToNext()); - assert(elseBlock->JumpsToNext()); - condBlock->SetFlags(propagateFlagsToAll); elseBlock->SetFlags(propagateFlagsToAll); @@ -14311,6 +14331,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) // +--->--------+ // bbj_cond(true) // + // TODO: Remove unnecessary condition reversal gtReverseCond(condExpr); thenBlock = fgNewBBafter(BBJ_ALWAYS, condBlock, true); @@ -14324,13 +14345,12 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) const unsigned thenLikelihood = qmark->ThenNodeLikelihood(); const unsigned elseLikelihood = qmark->ElseNodeLikelihood(); - FlowEdge* const newEdge = fgAddRefPred(remainderBlock, thenBlock); - thenBlock->SetTargetEdge(newEdge); + thenBlock->SetTargetEdge(fgAddRefPred(remainderBlock, thenBlock)); assert(condBlock->TargetIs(elseBlock)); - FlowEdge* const elseEdge = fgAddRefPred(thenBlock, condBlock); - FlowEdge* const thenEdge = condBlock->GetTargetEdge(); - condBlock->SetCond(thenEdge, elseEdge); + FlowEdge* const thenEdge = fgAddRefPred(thenBlock, condBlock); + FlowEdge* const elseEdge = condBlock->GetTargetEdge(); + condBlock->SetCond(elseEdge, thenEdge); thenBlock->inheritWeightPercentage(condBlock, thenLikelihood); elseBlock->inheritWeightPercentage(condBlock, elseLikelihood); thenEdge->setLikelihood(thenLikelihood / 100.0); @@ -14344,6 +14364,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) // +-->-------------+ // bbj_cond(true) // + // TODO: Remove unnecessary condition reversal gtReverseCond(condExpr); const unsigned thenLikelihood = qmark->ThenNodeLikelihood();