From 01ef993869024681c792fdb8f6ff1d03ca570d48 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 30 Jan 2024 18:50:04 -0800 Subject: [PATCH 01/11] enable has likelhood checks, repair likelihoods after random profile incorporation, stop checking very early --- src/coreclr/jit/compiler.h | 7 +-- src/coreclr/jit/fgprofile.cpp | 74 ++++++++++++++++---------- src/coreclr/jit/fgprofilesynthesis.cpp | 7 --- src/coreclr/jit/jitconfigvalues.h | 5 +- 4 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 14f4b872a6d169..5c2c715c3e9162 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1580,9 +1580,10 @@ enum class ProfileChecks : unsigned int { CHECK_NONE = 0, CHECK_CLASSIC = 1 << 0, // check "classic" jit weights - CHECK_LIKELY = 1 << 1, // check likelihood based weights - RAISE_ASSERT = 1 << 2, // assert on check failure - CHECK_ALL_BLOCKS = 1 << 3, // check blocks even if bbHasProfileWeight is false + CHECK_HASLIKELIHOOD = 1 << 1, // check all FlowEdges for hasLikelihood + CHECK_LIKELY = 1 << 2, // fully check likelihood based weights + RAISE_ASSERT = 1 << 3, // assert on check failure + CHECK_ALL_BLOCKS = 1 << 4, // check blocks even if bbHasProfileWeight is false }; inline constexpr ProfileChecks operator ~(ProfileChecks a) diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 504449a7c178a1..05b28ec700e34b 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -2798,6 +2798,7 @@ PhaseStatus Compiler::fgIncorporateProfileData() JITDUMP("JitStress -- incorporating random profile data\n"); fgIncorporateBlockCounts(); fgApplyProfileScale(); + ProfileSynthesis::Run(this, ProfileSynthesisOption::RepairLikelihoods); return PhaseStatus::MODIFIED_EVERYTHING; } @@ -5274,20 +5275,22 @@ bool Compiler::fgProfileWeightsConsistent(weight_t weight1, weight_t weight2) // (or nearly so) // // Notes: -// Does nothing, if profile checks are disabled, or there are -// no profile weights or pred lists. +// By default, just checks for each flow edge having likelihood. +// Can be altered via external config. // void Compiler::fgDebugCheckProfileWeights() { - // Optionally check profile data, if we have any. - // - const bool enabled = (JitConfig.JitProfileChecks() > 0) && fgHaveProfileWeights() && fgPredsComputed; - if (!enabled) + const bool configEnabled = (JitConfig.JitProfileChecks() >= 0) && fgHaveProfileWeights() && fgPredsComputed; + + if (configEnabled) { - return; + fgDebugCheckProfileWeights((ProfileChecks)JitConfig.JitProfileChecks()); + } + else + { + ProfileChecks checks = ProfileChecks::CHECK_HASLIKELIHOOD | ProfileChecks::RAISE_ASSERT; + fgDebugCheckProfileWeights(checks); } - - fgDebugCheckProfileWeights((ProfileChecks)JitConfig.JitProfileChecks()); } //------------------------------------------------------------------------ @@ -5312,19 +5315,21 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) { // We can check classic (min/max, late computed) weights // and/or - // new likelyhood based weights. + // new likelihood based weights. // const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC); const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY); + const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD); const bool assertOnFailure = hasFlag(checks, ProfileChecks::RAISE_ASSERT); const bool checkAllBlocks = hasFlag(checks, ProfileChecks::CHECK_ALL_BLOCKS); - if (!(verifyClassicWeights || verifyLikelyWeights)) + if (!(verifyClassicWeights || verifyLikelyWeights || verifyHasLikelihood)) { + JITDUMP("[profile weight checks disabled]\n"); return; } - JITDUMP("Checking Profile Data\n"); + JITDUMP("Checking Profile Weights (flags:0x%x)\n", checks); unsigned problemBlocks = 0; unsigned unprofiledBlocks = 0; unsigned profiledBlocks = 0; @@ -5434,22 +5439,25 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) // Verify overall input-output balance. // - if (entryProfiled && exitProfiled) + if (verifyClassicWeights || verifyLikelyWeights) { - // Note these may not agree, if fgEntryBB is a loop header. - // - if (fgFirstBB->bbRefs > 1) - { - JITDUMP(" Method entry " FMT_BB " is loop head, can't check entry/exit balance\n"); - } - else if (!fgProfileWeightsConsistent(entryWeight, exitWeight)) + if (entryProfiled && exitProfiled) { - problemBlocks++; - JITDUMP(" Method entry " FMT_WT " method exit " FMT_WT " weight mismatch\n", entryWeight, exitWeight); + // Note these may not agree, if fgEntryBB is a loop header. + // + if (fgFirstBB->bbRefs > 1) + { + JITDUMP(" Method entry " FMT_BB " is loop head, can't check entry/exit balance\n"); + } + else if (!fgProfileWeightsConsistent(entryWeight, exitWeight)) + { + problemBlocks++; + JITDUMP(" Method entry " FMT_WT " method exit " FMT_WT " weight mismatch\n", entryWeight, exitWeight); + } } } - // Sum up what we discovered. + // Summarize what we discovered. // if (problemBlocks == 0) { @@ -5457,11 +5465,15 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) { JITDUMP("No blocks were profiled, so nothing to check\n"); } - else + else if (verifyClassicWeights || verifyLikelyWeights) { JITDUMP("Profile is self-consistent (%d profiled blocks, %d unprofiled)\n", profiledBlocks, unprofiledBlocks); } + else if (verifyHasLikelihood) + { + JITDUMP("All flow edges have likelihoods\n"); + } } else { @@ -5493,8 +5505,9 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks { const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC); const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY); + const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD); - if (!(verifyClassicWeights || verifyLikelyWeights)) + if (!(verifyClassicWeights || verifyLikelyWeights || verifyHasLikelihood)) { return true; } @@ -5562,7 +5575,10 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks block->bbNum, blockWeight, incomingLikelyWeight); likelyWeightsValid = false; } + } + if (verifyHasLikelihood) + { if (missingLikelyWeight > 0) { JITDUMP(" " FMT_BB " -- %u incoming edges are missing likely weights\n", block->bbNum, @@ -5593,8 +5609,9 @@ bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks { const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC); const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY); + const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD); - if (!(verifyClassicWeights || verifyLikelyWeights)) + if (!(verifyClassicWeights || verifyLikelyWeights || verifyHasLikelihood)) { return true; } @@ -5674,14 +5691,17 @@ bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks } } - if (verifyLikelyWeights) + if (verifyHasLikelihood) { if (missingLikelihood > 0) { JITDUMP(" " FMT_BB " - missing likelihood on %d successor edges\n", block->bbNum, missingLikelihood); likelyWeightsValid = false; } + } + if (verifyLikelyWeights) + { if (!fgProfileWeightsConsistent(outgoingLikelihood, 1.0)) { JITDUMP(" " FMT_BB " - outgoing likelihood " FMT_WT " should be 1.0\n", block->bbNum, diff --git a/src/coreclr/jit/fgprofilesynthesis.cpp b/src/coreclr/jit/fgprofilesynthesis.cpp index b7af7de958555f..e315e33015e138 100644 --- a/src/coreclr/jit/fgprofilesynthesis.cpp +++ b/src/coreclr/jit/fgprofilesynthesis.cpp @@ -16,7 +16,6 @@ // * IR based heuristics (perhaps) // * During Cp, avoid repeatedly propagating through nested loops // * Fake BB0 or always force scratch BB -// * Reconcile with our other loop finding stuff // * Stop the upweight/downweight of loops in rest of jit // * Durable edge properties (exit, back) // * Tweak RunRarely to be at or near zero @@ -649,12 +648,6 @@ void ProfileSynthesis::RandomizeLikelihoods() for (BasicBlock* const block : m_comp->Blocks()) { unsigned const N = block->NumSucc(m_comp); - - if (N < 2) - { - continue; - } - likelihoods.clear(); likelihoods.reserve(N); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index bdcace7841f475..abc510d967a80d 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -144,8 +144,9 @@ CONFIG_INTEGER(JitPrintInlinedMethodsVerbose, W("JitPrintInlinedMethodsVerboseLe CONFIG_METHODSET(JitPrintInlinedMethods, W("JitPrintInlinedMethods")) CONFIG_METHODSET(JitPrintDevirtualizedMethods, W("JitPrintDevirtualizedMethods")) -CONFIG_INTEGER(JitProfileChecks, W("JitProfileChecks"), 0) // Bitflag: 0x1 check classic, 0x2 check likely, 0x4 enable - // asserts +// -1: just do internal checks +// Else bitflag: 0x1 check classic, 0x2 check likely, 0x4 enable asserts +CONFIG_INTEGER(JitProfileChecks, W("JitProfileChecks"), -1) CONFIG_INTEGER(JitRequired, W("JITRequired"), -1) CONFIG_INTEGER(JitRoundFloat, W("JITRoundFloat"), DEFAULT_ROUND_LEVEL) CONFIG_INTEGER(JitStackAllocToLocalSize, W("JitStackAllocToLocalSize"), DEFAULT_MAX_LOCALLOC_TO_LOCAL_SIZE) From c3a9e6ad17fd263e03497575303a16781261b8e8 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 30 Jan 2024 19:19:11 -0800 Subject: [PATCH 02/11] fixed importer; moved checking back a bit --- src/coreclr/jit/compiler.cpp | 6 +++++- src/coreclr/jit/importer.cpp | 42 ++++++++++++++++++++++++------------ 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index f0d8e0e46fd72d..679d7434801d39 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4503,7 +4503,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // activePhaseChecks |= PhaseChecks::CHECK_PROFILE; DoPhase(this, PHASE_INCPROFILE, &Compiler::fgIncorporateProfileData); - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; // If we are doing OSR, update flow to initially reach the appropriate IL offset. // @@ -4521,6 +4520,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_IMPORTATION, &Compiler::fgImport); + // Disable profile checks now. + // Over time we will move this further and further back in the phase list, as we fix issues. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + // If this is a failed inline attempt, we're done. // if (compIsForInlining() && compInlineResult->IsFailure()) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index a5901e40d721bc..e5b86440edb396 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4437,7 +4437,8 @@ void Compiler::impImportLeave(BasicBlock* block) // the previous call to a finally returns to this call (to the next finally in the chain) step->SetTarget(callBlock); - fgAddRefPred(callBlock, step); + FlowEdge* const newEdge = fgAddRefPred(callBlock, step); + newEdge->setLikelihood(1.0); // The new block will inherit this block's weight. callBlock->inheritWeight(block); @@ -4487,7 +4488,8 @@ void Compiler::impImportLeave(BasicBlock* block) assert(callBlock->KindIs(BBJ_CALLFINALLY)); assert(callBlock->TargetIs(HBtab->ebdHndBeg)); - fgAddRefPred(callBlock->GetTarget(), callBlock); + FlowEdge* const newEdge = fgAddRefPred(callBlock->GetTarget(), callBlock); + newEdge->setLikelihood(1.0); GenTree* endLFin = new (this, GT_END_LFIN) GenTreeVal(GT_END_LFIN, TYP_VOID, finallyNesting); endLFinStmt = gtNewStmt(endLFin); @@ -4537,7 +4539,8 @@ void Compiler::impImportLeave(BasicBlock* block) assert(!step->HasInitializedTarget()); step->SetTarget(finalStep); - fgAddRefPred(finalStep, step); + FlowEdge* const newEdge = fgAddRefPred(finalStep, step); + newEdge->setLikelihood(1.0); // The new block will inherit this block's weight. finalStep->inheritWeight(block); @@ -4566,7 +4569,8 @@ void Compiler::impImportLeave(BasicBlock* block) impEndTreeList(finalStep, endLFinStmt, lastStmt); // this is the ultimate destination of the LEAVE - fgAddRefPred(leaveTarget, finalStep); + FlowEdge* const newEdge = fgAddRefPred(leaveTarget, finalStep); + newEdge->setLikelihood(1.0); // Queue up the jump target for importing @@ -4684,7 +4688,8 @@ void Compiler::impImportLeave(BasicBlock* block) } step->SetTarget(exitBlock); // the previous step (maybe a call to a nested finally, or a nested catch // exit) returns to this block - fgAddRefPred(exitBlock, step); + FlowEdge* const newEdge = fgAddRefPred(exitBlock, step); + newEdge->setLikelihood(1.0); // The new block will inherit this block's weight. exitBlock->inheritWeight(block); @@ -4728,7 +4733,8 @@ void Compiler::impImportLeave(BasicBlock* block) // next block, and flow optimizations will remove it. fgRemoveRefPred(block->GetTarget(), block); block->SetKindAndTarget(BBJ_ALWAYS, callBlock); - fgAddRefPred(callBlock, block); + FlowEdge* const newEdge = fgAddRefPred(callBlock, block); + newEdge->setLikelihood(1.0); // The new block will inherit this block's weight. callBlock->inheritWeight(block); @@ -4797,7 +4803,8 @@ void Compiler::impImportLeave(BasicBlock* block) fgRemoveRefPred(step->GetTarget(), step); } step->SetTarget(step2); - fgAddRefPred(step2, step); + FlowEdge* const newEdge = fgAddRefPred(step2, step); + newEdge->setLikelihood(1.0); step2->inheritWeight(block); step2->CopyFlags(block, BBF_RUN_RARELY); step2->SetFlags(BBF_IMPORTED); @@ -4838,7 +4845,8 @@ void Compiler::impImportLeave(BasicBlock* block) } step->SetTarget(callBlock); // the previous call to a finally returns to this call (to the next // finally in the chain) - fgAddRefPred(callBlock, step); + FlowEdge* const newEdge = fgAddRefPred(callBlock, step); + newEdge->setLikelihood(1.0); // The new block will inherit this block's weight. callBlock->inheritWeight(block); @@ -4874,7 +4882,8 @@ void Compiler::impImportLeave(BasicBlock* block) assert(callBlock->KindIs(BBJ_CALLFINALLY)); assert(callBlock->TargetIs(HBtab->ebdHndBeg)); - fgAddRefPred(callBlock->GetTarget(), callBlock); + FlowEdge* const newEdge = fgAddRefPred(callBlock->GetTarget(), callBlock); + newEdge->setLikelihood(1.0); } else if (HBtab->HasCatchHandler() && jitIsBetween(blkAddr, tryBeg, tryEnd) && !jitIsBetween(jmpAddr, tryBeg, tryEnd)) @@ -4941,7 +4950,8 @@ void Compiler::impImportLeave(BasicBlock* block) fgRemoveRefPred(step->GetTarget(), step); } step->SetTarget(catchStep); - fgAddRefPred(catchStep, step); + FlowEdge* const newEdge = fgAddRefPred(catchStep, step); + newEdge->setLikelihood(1.0); // The new block will inherit this block's weight. catchStep->inheritWeight(block); @@ -4995,7 +5005,8 @@ void Compiler::impImportLeave(BasicBlock* block) fgRemoveRefPred(step->GetTarget(), step); } step->SetTarget(leaveTarget); // this is the ultimate destination of the LEAVE - fgAddRefPred(leaveTarget, step); + FlowEdge* const newEdge = fgAddRefPred(leaveTarget, step); + newEdge->setLikelihood(1.0); #ifdef DEBUG if (verbose) @@ -5056,7 +5067,8 @@ void Compiler::impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr) { BasicBlock* dupBlock = BasicBlock::New(this, BBJ_CALLFINALLY, block->GetTarget()); dupBlock->CopyFlags(block); - fgAddRefPred(dupBlock->GetTarget(), dupBlock); + FlowEdge* const newEdge = fgAddRefPred(dupBlock->GetTarget(), dupBlock); + newEdge->setLikelihood(1.0); dupBlock->copyEHRegion(block); dupBlock->bbCatchTyp = block->bbCatchTyp; @@ -5087,7 +5099,8 @@ void Compiler::impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr) fgRemoveRefPred(block->GetTarget(), block); block->SetKindAndTarget(BBJ_LEAVE, fgLookupBB(jmpAddr)); - fgAddRefPred(block->GetTarget(), block); + FlowEdge* const newEdge = fgAddRefPred(block->GetTarget(), block); + newEdge->setLikelihood(1.0); // We will leave the BBJ_ALWAYS block we introduced. When it's reimported // the BBJ_ALWAYS block will be unreachable, and will be removed after. The @@ -12244,7 +12257,8 @@ void Compiler::impFixPredLists() } BasicBlock* const continuation = predBlock->Next(); - fgAddRefPred(continuation, finallyBlock); + FlowEdge* const newEdge = fgAddRefPred(continuation, finallyBlock); + newEdge->setLikelihood(1.0); jumpEhf->bbeSuccs[predNum] = continuation; ++predNum; From 40d27dbd8cad8d54f783eda07b34bddf138e2c57 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 30 Jan 2024 20:00:31 -0800 Subject: [PATCH 03/11] fixed profile instrumentation --- src/coreclr/jit/compiler.cpp | 10 ++++----- src/coreclr/jit/fgbasic.cpp | 42 ++++++++++++++++++++++------------- src/coreclr/jit/fgflow.cpp | 7 ++++++ src/coreclr/jit/fgprofile.cpp | 6 +++-- 4 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 679d7434801d39..9a9ddf0dc34da6 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4520,11 +4520,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_IMPORTATION, &Compiler::fgImport); - // Disable profile checks now. - // Over time we will move this further and further back in the phase list, as we fix issues. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // If this is a failed inline attempt, we're done. // if (compIsForInlining() && compInlineResult->IsFailure()) @@ -4549,6 +4544,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl DoPhase(this, PHASE_IBCINSTR, &Compiler::fgInstrumentMethod); } + // Disable profile checks now. + // Over time we will move this further and further back in the phase list, as we fix issues. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + // Expand any patchpoints // DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgTransformPatchpoints); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 3852eec65d4bfe..8c7685224a6f42 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -456,7 +456,7 @@ void Compiler::fgReplaceSwitchJumpTarget(BasicBlock* blockSwitch, BasicBlock* ne { // Remove the old edge [oldTarget from blockSwitch] // - fgRemoveAllRefPreds(oldTarget, blockSwitch); + FlowEdge* const oldEdge = fgRemoveAllRefPreds(oldTarget, blockSwitch); // // Change the jumpTab entry to branch to the new location @@ -466,7 +466,7 @@ void Compiler::fgReplaceSwitchJumpTarget(BasicBlock* blockSwitch, BasicBlock* ne // // Create the new edge [newTarget from blockSwitch] // - FlowEdge* const newEdge = fgAddRefPred(newTarget, blockSwitch); + FlowEdge* const newEdge = fgAddRefPred(newTarget, blockSwitch, oldEdge); // Now set the correct value of newEdge's DupCount // and replace any other jumps in jumpTab[] that go to oldTarget. @@ -687,14 +687,15 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas if (block->TargetIs(oldTarget)) { block->SetTarget(newTarget); - fgRemoveRefPred(oldTarget, block); - fgAddRefPred(newTarget, block); + FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block); + fgAddRefPred(newTarget, block, oldEdge); } break; case BBJ_COND: { - FlowEdge* oldEdge = fgRemoveRefPred(oldTarget, block); + FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block); + if (block->TrueTargetIs(oldTarget)) { if (block->FalseTargetIs(oldTarget)) @@ -712,9 +713,15 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas block->SetTrueTarget(newTarget); } - // TODO-NoFallThrough: Proliferate weight from oldEdge - // (we avoid doing so when updating the true target to reduce diffs for now, as a quirk) - fgAddRefPred(newTarget, block); + FlowEdge* const newEdge = fgAddRefPred(newTarget, block); + if (block->KindIs(BBJ_ALWAYS)) + { + newEdge->setLikelihood(1.0); + } + else if (oldEdge->hasLikelihood()) + { + newEdge->setLikelihood(oldEdge->getLikelihood()); + } } else { @@ -737,10 +744,10 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas { if (jumpTab[i] == oldTarget) { - jumpTab[i] = newTarget; - changed = true; - fgRemoveRefPred(oldTarget, block); - fgAddRefPred(newTarget, block); + jumpTab[i] = newTarget; + changed = true; + FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block); + fgAddRefPred(newTarget, block, oldEdge); } } @@ -5067,7 +5074,8 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ) } fgReplacePred(succ, curr, newBlock); - fgAddRefPred(newBlock, curr); + FlowEdge* const newEdge = fgAddRefPred(newBlock, curr); + newEdge->setLikelihood(1.0); } else if (curr->KindIs(BBJ_SWITCH)) { @@ -5075,19 +5083,23 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ) fgReplaceSwitchJumpTarget(curr, newBlock, succ); // And 'succ' has 'newBlock' as a new predecessor. - fgAddRefPred(succ, newBlock); + FlowEdge* const newEdge = fgAddRefPred(succ, newBlock); + newEdge->setLikelihood(1.0); } else { assert(curr->KindIs(BBJ_ALWAYS)); fgReplacePred(succ, curr, newBlock); curr->SetTarget(newBlock); - fgAddRefPred(newBlock, curr); + FlowEdge* const newEdge = fgAddRefPred(newBlock, curr); + newEdge->setLikelihood(1.0); } // This isn't accurate, but it is complex to compute a reasonable number so just assume that we take the // branch 50% of the time. // + // TODO: leverage edge likelihood. + // if (!curr->KindIs(BBJ_ALWAYS)) { newBlock->inheritWeightPercentage(curr, 50); diff --git a/src/coreclr/jit/fgflow.cpp b/src/coreclr/jit/fgflow.cpp index 5f6a754a99416c..1321977e1fd077 100644 --- a/src/coreclr/jit/fgflow.cpp +++ b/src/coreclr/jit/fgflow.cpp @@ -187,6 +187,13 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE block->bbLastPred = flow; } + // Copy likelihood from old edge, if any. + // + if ((oldEdge != nullptr) && oldEdge->hasLikelihood()) + { + flow->setLikelihood(oldEdge->getLikelihood()); + } + if (fgHaveValidEdgeWeights) { // We are creating an edge from blockPred to block diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 05b28ec700e34b..55c851922ca132 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -511,7 +511,8 @@ void BlockCountInstrumentor::RelocateProbes() m_comp->fgNewBBbefore(BBJ_ALWAYS, block, /* extendRegion */ true, /* jumpDest */ block); intermediary->SetFlags(BBF_IMPORTED | BBF_MARKED | BBF_NONE_QUIRK); intermediary->inheritWeight(block); - m_comp->fgAddRefPred(block, intermediary); + FlowEdge* const newEdge = m_comp->fgAddRefPred(block, intermediary); + newEdge->setLikelihood(1.0); SetModifiedFlow(); while (criticalPreds.Height() > 0) @@ -1682,7 +1683,8 @@ void EfficientEdgeCountInstrumentor::RelocateProbes() m_comp->fgNewBBbefore(BBJ_ALWAYS, block, /* extendRegion */ true, /* jumpDest */ block); intermediary->SetFlags(BBF_IMPORTED | BBF_NONE_QUIRK); intermediary->inheritWeight(block); - m_comp->fgAddRefPred(block, intermediary); + FlowEdge* const newEdge = m_comp->fgAddRefPred(block, intermediary); + newEdge->setLikelihood(1.0); NewRelocatedProbe(intermediary, probe->source, probe->target, &leader); SetModifiedFlow(); From 199acbb768b72423c2712a823dceac3204d72c3d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 07:49:21 -0800 Subject: [PATCH 04/11] fix duplicate def --- src/coreclr/jit/importer.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e5b86440edb396..25937adca38bba 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4539,8 +4539,10 @@ void Compiler::impImportLeave(BasicBlock* block) assert(!step->HasInitializedTarget()); step->SetTarget(finalStep); - FlowEdge* const newEdge = fgAddRefPred(finalStep, step); - newEdge->setLikelihood(1.0); + { + FlowEdge* const newEdge = fgAddRefPred(finalStep, step); + newEdge->setLikelihood(1.0); + } // The new block will inherit this block's weight. finalStep->inheritWeight(block); @@ -4569,8 +4571,10 @@ void Compiler::impImportLeave(BasicBlock* block) impEndTreeList(finalStep, endLFinStmt, lastStmt); // this is the ultimate destination of the LEAVE - FlowEdge* const newEdge = fgAddRefPred(leaveTarget, finalStep); - newEdge->setLikelihood(1.0); + { + FlowEdge* const newEdge = fgAddRefPred(leaveTarget, finalStep); + newEdge->setLikelihood(1.0); + } // Queue up the jump target for importing From fab47e38ffda7a651f2d95dc4eb32a837d5548b1 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 08:12:34 -0800 Subject: [PATCH 05/11] check patchpoints phase --- src/coreclr/jit/compiler.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 9a9ddf0dc34da6..aef531c11a6f3c 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4544,15 +4544,15 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl DoPhase(this, PHASE_IBCINSTR, &Compiler::fgInstrumentMethod); } + // Expand any patchpoints + // + DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgTransformPatchpoints); + // Disable profile checks now. // Over time we will move this further and further back in the phase list, as we fix issues. // activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // Expand any patchpoints - // - DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgTransformPatchpoints); - // Transform indirect calls that require control flow expansion. // DoPhase(this, PHASE_INDXCALL, &Compiler::fgTransformIndirectCalls); From 8775912cf5bdfe7a15a0f4570b10c4b18018cefd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 08:19:23 -0800 Subject: [PATCH 06/11] make hasLikelihood be available in release builds --- src/coreclr/jit/block.h | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 9fa0bcf0f8d730..3556087253edb2 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -2070,9 +2070,8 @@ struct FlowEdge // The count of duplicate "edges" (used for switch stmts or degenerate branches) unsigned m_dupCount; -#ifdef DEBUG + // True if likelihood has been set bool m_likelihoodSet; -#endif public: FlowEdge(BasicBlock* block, FlowEdge* rest) @@ -2081,7 +2080,8 @@ struct FlowEdge , m_edgeWeightMin(0) , m_edgeWeightMax(0) , m_likelihood(0) - , m_dupCount(0) DEBUGARG(m_likelihoodSet(false)) + , m_dupCount(0) + , m_likelihoodSet(false) { } @@ -2137,22 +2137,20 @@ struct FlowEdge { assert(likelihood >= 0.0); assert(likelihood <= 1.0); - INDEBUG(m_likelihoodSet = true); - m_likelihood = likelihood; + m_likelihoodSet = true; + m_likelihood = likelihood; } void clearLikelihood() { - m_likelihood = 0.0; - INDEBUG(m_likelihoodSet = false); + m_likelihood = 0.0; + m_likelihoodSet = false; } -#ifdef DEBUG bool hasLikelihood() const { return m_likelihoodSet; } -#endif weight_t getLikelyWeight() const { From a4b2eae82f21df1561f0492a5988a0fed7361b38 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 09:00:59 -0800 Subject: [PATCH 07/11] check indirect calls phase --- src/coreclr/jit/compiler.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index aef531c11a6f3c..6403ef582fa404 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4548,15 +4548,15 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgTransformPatchpoints); + // Transform indirect calls that require control flow expansion. + // + DoPhase(this, PHASE_INDXCALL, &Compiler::fgTransformIndirectCalls); + // Disable profile checks now. // Over time we will move this further and further back in the phase list, as we fix issues. // activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // Transform indirect calls that require control flow expansion. - // - DoPhase(this, PHASE_INDXCALL, &Compiler::fgTransformIndirectCalls); - // Cleanup un-imported BBs, cleanup un-imported or // partially imported try regions, add OSR step blocks. // From 2972f1cbc779fe28404865867a7a68482b9dc6e5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 09:28:23 -0800 Subject: [PATCH 08/11] check post import and morph init --- src/coreclr/jit/compiler.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 6403ef582fa404..b10b6174596349 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4552,11 +4552,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_INDXCALL, &Compiler::fgTransformIndirectCalls); - // Disable profile checks now. - // Over time we will move this further and further back in the phase list, as we fix issues. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // Cleanup un-imported BBs, cleanup un-imported or // partially imported try regions, add OSR step blocks. // @@ -4588,6 +4583,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_MORPH_INIT, &Compiler::fgMorphInit); + // Disable profile checks now. + // Over time we will move this further and further back in the phase list, as we fix issues. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + // Inline callee methods into this root method // DoPhase(this, PHASE_MORPH_INLINE, &Compiler::fgInline); From 7ff6a9a9a475d1f3b0ffbf28bc341c9f097ec304 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 10:12:48 -0800 Subject: [PATCH 09/11] check inlining --- src/coreclr/jit/compiler.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index b10b6174596349..46268949b7ba8c 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4583,15 +4583,15 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_MORPH_INIT, &Compiler::fgMorphInit); + // Inline callee methods into this root method + // + DoPhase(this, PHASE_MORPH_INLINE, &Compiler::fgInline); + // Disable profile checks now. // Over time we will move this further and further back in the phase list, as we fix issues. // activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // Inline callee methods into this root method - // - DoPhase(this, PHASE_MORPH_INLINE, &Compiler::fgInline); - // Record "start" values for post-inlining cycles and elapsed time. RecordStateAtEndOfInlining(); From 9bf870e1122ab4fcb19cc2a1e8197e87705d1651 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 18:08:49 -0800 Subject: [PATCH 10/11] check fgAddInternal; x86 fixes, and many other fixes --- src/coreclr/jit/compiler.cpp | 10 +-- src/coreclr/jit/fgbasic.cpp | 21 ++++-- src/coreclr/jit/fginline.cpp | 7 +- src/coreclr/jit/fgopt.cpp | 18 +++-- src/coreclr/jit/fgprofile.cpp | 4 +- src/coreclr/jit/flowgraph.cpp | 6 +- src/coreclr/jit/importer.cpp | 3 +- src/coreclr/jit/indirectcalltransformer.cpp | 75 ++++++++++++++------- src/coreclr/jit/jiteh.cpp | 15 +++-- 9 files changed, 108 insertions(+), 51 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 46268949b7ba8c..5fac5c8e0704c6 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4587,11 +4587,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_MORPH_INLINE, &Compiler::fgInline); - // Disable profile checks now. - // Over time we will move this further and further back in the phase list, as we fix issues. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - // Record "start" values for post-inlining cycles and elapsed time. RecordStateAtEndOfInlining(); @@ -4610,6 +4605,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_MORPH_ADD_INTERNAL, &Compiler::fgAddInternal); + // Disable profile checks now. + // Over time we will move this further and further back in the phase list, as we fix issues. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + // Remove empty try regions // DoPhase(this, PHASE_EMPTY_TRY, &Compiler::fgRemoveEmptyTry); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 8c7685224a6f42..43913e91f47e2c 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -390,17 +390,29 @@ void Compiler::fgChangeSwitchBlock(BasicBlock* oldSwitchBlock, BasicBlock* newSw // fgRemoveRefPred()/fgAddRefPred() will do the right thing: the second and // subsequent duplicates will simply subtract from and add to the duplicate // count (respectively). - + // + // However this does the "wrong" thing with respect to edge profile + // data; the old edge is not returned by fgRemoveRefPred until it has + // a dup count of 0, and the fgAddRefPred only uses the optional + // old edge arg when the new edge is first created. // // Remove the old edge [oldSwitchBlock => bJump] // assert(bJump->countOfInEdges() > 0); - fgRemoveRefPred(bJump, oldSwitchBlock); + FlowEdge* const oldEdge = fgRemoveRefPred(bJump, oldSwitchBlock); // // Create the new edge [newSwitchBlock => bJump] // - fgAddRefPred(bJump, newSwitchBlock); + FlowEdge* const newEdge = fgAddRefPred(bJump, newSwitchBlock); + + // Handle the profile update, once we get our hands on the old edge. + // + if (oldEdge != nullptr) + { + assert(!newEdge->hasLikelihood()); + newEdge->setLikelihood(oldEdge->getLikelihood()); + } } if (m_switchDescMap != nullptr) @@ -4822,7 +4834,8 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr) curr->SetFlags(BBF_NONE_QUIRK); assert(curr->JumpsToNext()); - fgAddRefPred(newBlock, curr); + FlowEdge* const newEdge = fgAddRefPred(newBlock, curr); + newEdge->setLikelihood(1.0); return newBlock; } diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 4a4093492179dd..54003637e7e36e 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -1534,7 +1534,8 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) bottomBlock->bbNum); block->SetKindAndTarget(BBJ_ALWAYS, bottomBlock); - fgAddRefPred(bottomBlock, block); + FlowEdge* const newEdge = fgAddRefPred(bottomBlock, block); + newEdge->setLikelihood(1.0); if (block == InlineeCompiler->fgLastBB) { @@ -1553,8 +1554,8 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) topBlock->SetNext(InlineeCompiler->fgFirstBB); topBlock->SetTarget(topBlock->Next()); topBlock->SetFlags(BBF_NONE_QUIRK); - fgRemoveRefPred(bottomBlock, topBlock); - fgAddRefPred(InlineeCompiler->fgFirstBB, topBlock); + FlowEdge* const oldEdge = fgRemoveRefPred(bottomBlock, topBlock); + fgAddRefPred(InlineeCompiler->fgFirstBB, topBlock, oldEdge); InlineeCompiler->fgLastBB->SetNext(bottomBlock); // diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 52f969611a0a76..4c5aa2eec27c2e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -647,7 +647,8 @@ PhaseStatus Compiler::fgPostImportationCleanup() } else { - fgAddRefPred(newTryEntry->Next(), newTryEntry); + FlowEdge* const newEdge = fgAddRefPred(newTryEntry->Next(), newTryEntry); + newEdge->setLikelihood(1.0); } JITDUMP("OSR: changing start of try region #%u from " FMT_BB " to new " FMT_BB "\n", @@ -773,6 +774,7 @@ PhaseStatus Compiler::fgPostImportationCleanup() fromBlock->SetFlags(BBF_INTERNAL); newBlock->RemoveFlags(BBF_DONT_REMOVE); addedBlocks++; + FlowEdge* const normalTryEntryEdge = fgGetPredForBlock(newBlock, fromBlock); GenTree* const entryStateLcl = gtNewLclvNode(entryStateVar, TYP_INT); GenTree* const compareEntryStateToZero = @@ -781,9 +783,17 @@ PhaseStatus Compiler::fgPostImportationCleanup() fgNewStmtAtBeg(fromBlock, jumpIfEntryStateZero); fromBlock->SetCond(toBlock, newBlock); - fgAddRefPred(toBlock, fromBlock); + FlowEdge* const osrTryEntryEdge = fgAddRefPred(toBlock, fromBlock); newBlock->inheritWeight(fromBlock); + // Not sure what the correct edge likelihoods are just yet; + // for now we'll say the OSR path is the likely one. + // + // Todo: can we leverage profile data here to get a better answer? + // + osrTryEntryEdge->setLikelihood(0.9); + normalTryEntryEdge->setLikelihood(0.1); + entryJumpTarget = fromBlock; }; @@ -824,8 +834,8 @@ PhaseStatus Compiler::fgPostImportationCleanup() if (entryJumpTarget != osrEntry) { fgFirstBB->SetTarget(entryJumpTarget); - fgRemoveRefPred(osrEntry, fgFirstBB); - fgAddRefPred(entryJumpTarget, fgFirstBB); + FlowEdge* const oldEdge = fgRemoveRefPred(osrEntry, fgFirstBB); + fgAddRefPred(entryJumpTarget, fgFirstBB, oldEdge); JITDUMP("OSR: redirecting flow from method entry " FMT_BB " to OSR entry " FMT_BB " via step blocks.\n", diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 55c851922ca132..cfa6ae0ff72859 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -4068,7 +4068,6 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf assert(block == edge->m_sourceBlock); FlowEdge* const flowEdge = m_comp->fgGetPredForBlock(edge->m_targetBlock, block); assert(flowEdge != nullptr); - assert(!flowEdge->hasLikelihood()); weight_t likelihood = 0; if (nEdges == 1) @@ -5534,6 +5533,8 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks } else { + JITDUMP("Missing likelihood on %p " FMT_BB "->" FMT_BB "\n", predEdge, predEdge->getSourceBlock()->bbNum, + block->bbNum); missingLikelyWeight++; } @@ -5658,6 +5659,7 @@ bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks } else { + JITDUMP("Missing likelihood on %p " FMT_BB "->" FMT_BB "\n", succEdge, block->bbNum, succBlock->bbNum); missingLikelihood++; } } diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index f5ce79df20d4f5..eff5c2f001bad3 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -1626,7 +1626,8 @@ void Compiler::fgConvertSyncReturnToLeave(BasicBlock* block) // Convert the BBJ_RETURN to BBJ_ALWAYS, jumping to genReturnBB. block->SetKindAndTarget(BBJ_ALWAYS, genReturnBB); - fgAddRefPred(genReturnBB, block); + FlowEdge* const newEdge = fgAddRefPred(genReturnBB, block); + newEdge->setLikelihood(1.0); #ifdef DEBUG if (verbose) @@ -2097,7 +2098,8 @@ class MergedReturns // Change BBJ_RETURN to BBJ_ALWAYS targeting const return block. assert((comp->info.compFlags & CORINFO_FLG_SYNCH) == 0); returnBlock->SetKindAndTarget(BBJ_ALWAYS, constReturnBlock); - comp->fgAddRefPred(constReturnBlock, returnBlock); + FlowEdge* const newEdge = comp->fgAddRefPred(constReturnBlock, returnBlock); + newEdge->setLikelihood(1.0); // Remove GT_RETURN since constReturnBlock returns the constant. assert(returnBlock->lastStmt()->GetRootNode()->OperIs(GT_RETURN)); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 25937adca38bba..69d623787fa35f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -2026,7 +2026,8 @@ BasicBlock* Compiler::impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_H newBlk->inheritWeight(hndBlk); newBlk->bbCodeOffs = hndBlk->bbCodeOffs; - fgAddRefPred(hndBlk, newBlk); + FlowEdge* const newEdge = fgAddRefPred(hndBlk, newBlk); + newEdge->setLikelihood(1.0); // Spill into a temp. unsigned tempNum = lvaGrabTemp(false DEBUGARG("SpillCatchArg")); diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 479b1385f773c0..fbbe08d66fc3c6 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -273,21 +273,32 @@ class IndirectCallTransformer { assert(currBlock->KindIs(BBJ_ALWAYS)); currBlock->SetTarget(checkBlock); - compiler->fgAddRefPred(checkBlock, currBlock); + FlowEdge* const newEdge = compiler->fgAddRefPred(checkBlock, currBlock); + newEdge->setLikelihood(1.0); } // checkBlock + // Todo: get likelihoods right + // assert(checkBlock->KindIs(BBJ_ALWAYS)); checkBlock->SetCond(elseBlock, thenBlock); - compiler->fgAddRefPred(elseBlock, checkBlock); - compiler->fgAddRefPred(thenBlock, checkBlock); + FlowEdge* const thenEdge = compiler->fgAddRefPred(thenBlock, checkBlock); + thenEdge->setLikelihood(0.5); + FlowEdge* const elseEdge = compiler->fgAddRefPred(elseBlock, checkBlock); + elseEdge->setLikelihood(0.5); // thenBlock assert(thenBlock->TargetIs(remainderBlock)); - compiler->fgAddRefPred(remainderBlock, thenBlock); + { + FlowEdge* const newEdge = compiler->fgAddRefPred(remainderBlock, thenBlock); + newEdge->setLikelihood(1.0); + } // elseBlock - compiler->fgAddRefPred(remainderBlock, elseBlock); + { + FlowEdge* const newEdge = compiler->fgAddRefPred(remainderBlock, elseBlock); + newEdge->setLikelihood(1.0); + } } Compiler* compiler; @@ -590,10 +601,6 @@ class IndirectCallTransformer checkBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, thenBlock); checkFallsThrough = false; - // prevCheckBlock is expected to jump to this new check (if its type check doesn't succeed) - prevCheckBlock->SetCond(checkBlock, prevCheckBlock->Next()); - compiler->fgAddRefPred(checkBlock, prevCheckBlock); - // Calculate the total likelihood for this check as a sum of likelihoods // of all previous candidates (thenBlocks) unsigned checkLikelihood = 100; @@ -604,7 +611,12 @@ class IndirectCallTransformer // Make sure we didn't overflow assert(checkLikelihood <= 100); + weight_t checkLikelihoodWt = ((weight_t)checkLikelihood) / 100.0; + // prevCheckBlock is expected to jump to this new check (if its type check doesn't succeed) + prevCheckBlock->SetCond(checkBlock, prevCheckBlock->Next()); + FlowEdge* const checkEdge = compiler->fgAddRefPred(checkBlock, prevCheckBlock); + checkEdge->setLikelihood(checkLikelihoodWt); checkBlock->inheritWeightPercentage(currBlock, checkLikelihood); } @@ -1002,18 +1014,26 @@ class IndirectCallTransformer // virtual void CreateThen(uint8_t checkIdx) { + // Compute likelihoods + // + unsigned const thenLikelihood = origCall->GetGDVCandidateInfo(checkIdx)->likelihood; + weight_t thenLikelihoodWt = min(((weight_t)thenLikelihood) / 100.0, 100.0); + weight_t elseLikelihoodWt = max(1.0 - thenLikelihoodWt, 0.0); + // thenBlock always jumps to remainderBlock thenBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, checkBlock, remainderBlock); thenBlock->CopyFlags(currBlock, BBF_SPLIT_GAINED); - thenBlock->inheritWeightPercentage(currBlock, origCall->GetGDVCandidateInfo(checkIdx)->likelihood); + thenBlock->inheritWeightPercentage(currBlock, thenLikelihood); // Also, thenBlock has a single pred - last checkBlock assert(checkBlock->KindIs(BBJ_ALWAYS)); checkBlock->SetTarget(thenBlock); checkBlock->SetFlags(BBF_NONE_QUIRK); assert(checkBlock->JumpsToNext()); - compiler->fgAddRefPred(thenBlock, checkBlock); - compiler->fgAddRefPred(remainderBlock, thenBlock); + FlowEdge* const thenEdge = compiler->fgAddRefPred(thenBlock, checkBlock); + thenEdge->setLikelihood(thenLikelihoodWt); + FlowEdge* const elseEdge = compiler->fgAddRefPred(remainderBlock, thenBlock); + elseEdge->setLikelihood(elseLikelihoodWt); DevirtualizeCall(thenBlock, checkIdx); } @@ -1023,6 +1043,17 @@ class IndirectCallTransformer // virtual void CreateElse() { + // Calculate the likelihood of the else block as a remainder of the sum + // of all the other likelihoods. + unsigned elseLikelihood = 100; + for (uint8_t i = 0; i < origCall->GetInlineCandidatesCount(); i++) + { + elseLikelihood -= origCall->GetGDVCandidateInfo(i)->likelihood; + } + // Make sure it didn't overflow + assert(elseLikelihood <= 100); + weight_t elseLikelihoodDbl = ((weight_t)elseLikelihood) / 100.0; + elseBlock = CreateAndInsertBasicBlock(BBJ_ALWAYS, thenBlock, thenBlock->Next()); elseBlock->CopyFlags(currBlock, BBF_SPLIT_GAINED); elseBlock->SetFlags(BBF_NONE_QUIRK); @@ -1032,7 +1063,8 @@ class IndirectCallTransformer if (!checkFallsThrough) { checkBlock->SetCond(elseBlock, checkBlock->Next()); - compiler->fgAddRefPred(elseBlock, checkBlock); + FlowEdge* const checkEdge = compiler->fgAddRefPred(elseBlock, checkBlock); + checkEdge->setLikelihood(elseLikelihoodDbl); } else { @@ -1043,17 +1075,8 @@ class IndirectCallTransformer } // elseBlock always flows into remainderBlock - compiler->fgAddRefPred(remainderBlock, elseBlock); - - // Calculate the likelihood of the else block as a remainder of the sum - // of all the other likelihoods. - unsigned elseLikelihood = 100; - for (uint8_t i = 0; i < origCall->GetInlineCandidatesCount(); i++) - { - elseLikelihood -= origCall->GetGDVCandidateInfo(i)->likelihood; - } - // Make sure it didn't overflow - assert(elseLikelihood <= 100); + FlowEdge* const elseEdge = compiler->fgAddRefPred(remainderBlock, elseBlock); + elseEdge->setLikelihood(1.0); // Remove everything related to inlining from the original call origCall->ClearInlineInfo(); @@ -1153,9 +1176,9 @@ class IndirectCallTransformer // Finally, rewire the cold block to jump to the else block, // not fall through to the check block. // - compiler->fgRemoveRefPred(checkBlock, coldBlock); + FlowEdge* const oldEdge = compiler->fgRemoveRefPred(checkBlock, coldBlock); coldBlock->SetKindAndTarget(BBJ_ALWAYS, elseBlock); - compiler->fgAddRefPred(elseBlock, coldBlock); + compiler->fgAddRefPred(elseBlock, coldBlock, oldEdge); } // When the current candidate hads sufficiently high likelihood, scan diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 1b4ec52c353fc9..5dd2eb351bb843 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -1986,7 +1986,8 @@ bool Compiler::fgNormalizeEHCase1() // handler. BasicBlock* newHndStart = BasicBlock::New(this, BBJ_ALWAYS, handlerStart); fgInsertBBbefore(handlerStart, newHndStart); - fgAddRefPred(handlerStart, newHndStart); + FlowEdge* newEdge = fgAddRefPred(handlerStart, newHndStart); + newEdge->setLikelihood(1.0); // Handler begins have an extra implicit ref count. // BasicBlock::New has already handled this for newHndStart. @@ -2156,7 +2157,8 @@ bool Compiler::fgNormalizeEHCase2() BasicBlock* newTryStart = BasicBlock::New(this, BBJ_ALWAYS, insertBeforeBlk); newTryStart->bbRefs = 0; fgInsertBBbefore(insertBeforeBlk, newTryStart); - fgAddRefPred(insertBeforeBlk, newTryStart); + FlowEdge* const newEdge = fgAddRefPred(insertBeforeBlk, newTryStart); + newEdge->setLikelihood(1.0); // It's possible for a try to start at the beginning of a method. If so, we need // to adjust the implicit ref counts as we've just created a new first bb @@ -2372,7 +2374,8 @@ bool Compiler::fgCreateFiltersForGenericExceptions() // Insert it right before the handler (and make it a pred of the handler) fgInsertBBbefore(handlerBb, filterBb); - fgAddRefPred(handlerBb, filterBb); + FlowEdge* const newEdge = fgAddRefPred(handlerBb, filterBb); + newEdge->setLikelihood(1.0); fgNewStmtAtEnd(filterBb, retFilt, handlerBb->firstStmt()->GetDebugInfo()); filterBb->bbCatchTyp = BBCT_FILTER; @@ -2678,7 +2681,8 @@ bool Compiler::fgNormalizeEHCase3() newLast->bbCodeOffsEnd = newLast->bbCodeOffs; // code size = 0. TODO: use BAD_IL_OFFSET instead? newLast->inheritWeight(insertAfterBlk); newLast->SetFlags(BBF_INTERNAL | BBF_NONE_QUIRK); - fgAddRefPred(newLast, insertAfterBlk); + FlowEdge* const newEdge = fgAddRefPred(newLast, insertAfterBlk); + newEdge->setLikelihood(1.0); // Move the insert pointer. More enclosing equivalent 'last' blocks will be inserted after this. insertAfterBlk = newLast; @@ -4340,7 +4344,8 @@ void Compiler::fgExtendEHRegionBefore(BasicBlock* block) // Change the bbTarget for bFilterLast from the old first 'block' to the new first 'bPrev' fgRemoveRefPred(bFilterLast->GetTarget(), bFilterLast); bFilterLast->SetTarget(bPrev); - fgAddRefPred(bPrev, bFilterLast); + FlowEdge* const newEdge = fgAddRefPred(bPrev, bFilterLast); + newEdge->setLikelihood(1.0); } } From 052a46452068e79fce084d20038fd024b89f48ed Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 31 Jan 2024 19:32:09 -0800 Subject: [PATCH 11/11] post merge fixes --- src/coreclr/jit/fgbasic.cpp | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 30d0d2030a783c..c8a142f07b7a69 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -630,8 +630,6 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas break; case BBJ_COND: - { - FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block); if (block->TrueTargetIs(oldTarget)) { @@ -648,6 +646,12 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas block->SetTrueTarget(newTarget); } + // fgRemoveRefPred should have removed the flow edge + FlowEdge* oldEdge = fgRemoveRefPred(oldTarget, block); + assert(oldEdge != nullptr); + + // TODO-NoFallThrough: Proliferate weight from oldEdge + // (as a quirk, we avoid doing so for the true target to reduce diffs for now) FlowEdge* const newEdge = fgAddRefPred(newTarget, block); if (block->KindIs(BBJ_ALWAYS)) { @@ -656,6 +660,7 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas else if (oldEdge->hasLikelihood()) { newEdge->setLikelihood(oldEdge->getLikelihood()); + } } else { @@ -682,7 +687,18 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas jumpTab[i] = newTarget; changed = true; FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block); - fgAddRefPred(newTarget, block, oldEdge); + FlowEdge* const newEdge = fgAddRefPred(newTarget, block, oldEdge); + + // Handle the profile update, once we get our hands on the old edge. + // (see notes in fgChangeSwitchBlock for why this extra step is necessary) + // + // We do it slightly differently here so we don't lose the old + // edge weight propagation that would sometimes happen + // + if ((oldEdge != nullptr) && !newEdge->hasLikelihood()) + { + newEdge->setLikelihood(oldEdge->getLikelihood()); + } } }