From 5156353ea42d439cef222c12df62c139eb44da58 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 20 Nov 2024 09:43:24 -0500 Subject: [PATCH 1/6] Remove late fgRenumberBlocks call --- src/coreclr/jit/flowgraph.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index dd4d95ab8356d..16cea78cbd7a7 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -152,13 +152,8 @@ PhaseStatus Compiler::fgInsertGCPolls() block = curBasicBlock; } - // If we split a block to create a GC Poll, call fgUpdateChangedFlowGraph. // We should never split blocks unless we're optimizing. - if (createdPollBlocks) - { - noway_assert(opts.OptimizationEnabled()); - fgRenumberBlocks(); - } + assert(!createdPollBlocks || opts.OptimizationEnabled()); return result; } From d6f08fc934547c992c64884c309f4c631993e17b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 20 Nov 2024 10:04:30 -0500 Subject: [PATCH 2/6] Remove fgRenumberBlocks call during EH normalization --- src/coreclr/jit/jiteh.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index e3d1714702404..96bc754d31716 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -2123,8 +2123,8 @@ void Compiler::fgNormalizeEH() if (modified) { JITDUMP("Added at least one basic block in fgNormalizeEH.\n"); - fgRenumberBlocks(); - // fgRenumberBlocks() will dump all the blocks and the handler table, so we don't need to do it here. + JITDUMPEXEC(fgDispBasicBlocks()); + JITDUMPEXEC(fgDispHandlerTab()); INDEBUG(fgVerifyHandlerTab()); } else From c6f3c20292454fad238f2ca957bac5d407941275 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 20 Nov 2024 10:10:18 -0500 Subject: [PATCH 3/6] Remove fgRenumberBlocks call after import --- src/coreclr/jit/fgopt.cpp | 14 +------------- src/coreclr/jit/optimizer.cpp | 4 ++-- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 072bb25a82a89..4f90783e6ea4f 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -752,25 +752,13 @@ PhaseStatus Compiler::fgPostImportationCleanup() } } - // Did we alter any flow or EH? - // - const bool madeFlowChanges = (addedBlocks > 0) || (delCnt > 0) || (removedBlks > 0); - - // Renumber the basic blocks if so. - // - if (madeFlowChanges) - { - JITDUMP("\nRenumbering the basic blocks for fgPostImportationCleanup\n"); - fgRenumberBlocks(); - } - #ifdef DEBUG fgVerifyHandlerTab(); #endif // DEBUG // Did we make any changes? // - const bool madeChanges = madeFlowChanges || addedTemps; + const bool madeChanges = (addedBlocks > 0) || (delCnt > 0) || (removedBlks > 0) || addedTemps; // Note that we have now run post importation cleanup, // so we can enable more stringent checking. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 28a19fe53dbe8..56378328f5525 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -1302,7 +1302,7 @@ PhaseStatus Compiler::optUnrollLoops() } JITDUMP("A nested loop was unrolled. Doing another pass (pass %d)\n", passes + 1); - fgRenumberBlocks(); + // fgRenumberBlocks(); fgInvalidateDfsTree(); m_dfsTree = fgComputeDfs(); m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); @@ -1335,7 +1335,7 @@ PhaseStatus Compiler::optUnrollLoops() m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); } - fgRenumberBlocks(); + // fgRenumberBlocks(); DBEXEC(verbose, fgDispBasicBlocks()); } From 6e333de6f7991077f776bcc5022b3535ab234996 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 20 Nov 2024 13:41:46 -0500 Subject: [PATCH 4/6] Move fgRenumberBlocks prereq call into optSetBlockWeights --- src/coreclr/jit/compiler.cpp | 2 -- src/coreclr/jit/optimizer.cpp | 10 +++------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 41986d0320b5b..6cd4c52125d6f 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5888,8 +5888,6 @@ void Compiler::RecomputeFlowGraphAnnotations() // Recompute reachability sets, dominators, and loops. optResetLoopInfo(); - fgRenumberBlocks(); - fgInvalidateDfsTree(); fgDfsBlocksAndRemove(); optFindLoops(); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 56378328f5525..16e8e1d971446 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -40,6 +40,7 @@ DataFlow::DataFlow(Compiler* pCompiler) PhaseStatus Compiler::optSetBlockWeights() { noway_assert(opts.OptimizationEnabled()); + bool madeChanges = fgRenumberBlocks(); assert(m_dfsTree != nullptr); if (m_domTree == nullptr) @@ -57,7 +58,6 @@ PhaseStatus Compiler::optSetBlockWeights() optFindAndScaleGeneralLoopBlocks(); } - bool madeChanges = false; bool firstBBDominatesAllReturns = true; const bool usingProfileWeights = fgIsUsingProfileWeights(); @@ -1302,7 +1302,7 @@ PhaseStatus Compiler::optUnrollLoops() } JITDUMP("A nested loop was unrolled. Doing another pass (pass %d)\n", passes + 1); - // fgRenumberBlocks(); + fgRenumberBlocks(); fgInvalidateDfsTree(); m_dfsTree = fgComputeDfs(); m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); @@ -1335,7 +1335,7 @@ PhaseStatus Compiler::optUnrollLoops() m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); } - // fgRenumberBlocks(); + fgRenumberBlocks(); DBEXEC(verbose, fgDispBasicBlocks()); } @@ -2687,8 +2687,6 @@ PhaseStatus Compiler::optFindLoopsPhase() } #endif - fgRenumberBlocks(); - assert(m_dfsTree != nullptr); optFindLoops(); @@ -2713,8 +2711,6 @@ void Compiler::optFindLoops() m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); } - fgRenumberBlocks(); - // Starting now we require all loops to be in canonical form. optLoopsCanonical = true; From 14ad1c4c54a80617ac6dd9e5e3a6abe25aa41c67 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 20 Nov 2024 16:46:05 -0500 Subject: [PATCH 5/6] More low-churn fgRenumberBlocks removals --- src/coreclr/jit/compiler.cpp | 16 +++++++++------- src/coreclr/jit/fginline.cpp | 8 -------- src/coreclr/jit/gschecks.cpp | 6 ------ src/coreclr/jit/helperexpansion.cpp | 5 ----- src/coreclr/jit/optimizer.cpp | 7 +++---- 5 files changed, 12 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 6cd4c52125d6f..ee52ba485d82c 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4761,13 +4761,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl }; DoPhase(this, PHASE_POST_MORPH, postMorphPhase); - // If we needed to create any new BasicBlocks then renumber the blocks - // - if (fgBBcount > preMorphBBCount) - { - fgRenumberBlocks(); - } - // GS security checks for unsafe buffers // DoPhase(this, PHASE_GS_COOKIE, &Compiler::gsPhase); @@ -4928,6 +4921,15 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl if (doLoopHoisting) { + if (doBranchOpt) + { + // bbNum quirk: RBO uses the current BB epoch size to determine if loop hoisting created blocks. + // Ensure the BB epoch is up to date before running loop hoisting to avoid false positives. + // TODO-bbNum: Either replace this check with a breadcrumb like 'fgModified', + // or relax jump threading's prerequisites. + EnsureBasicBlockEpoch(); + } + // Hoist invariant code out of loops // DoPhase(this, PHASE_HOIST_LOOP_CODE, &Compiler::optHoistLoopCode); diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 9d5cdbba32e0c..d7abf54dfc394 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -913,14 +913,6 @@ PhaseStatus Compiler::fgInline() #endif // DEBUG - if (madeChanges) - { - // Optional quirk to keep this as zero diff. Some downstream phases are bbNum sensitive - // but rely on the ambient bbNums. - // - fgRenumberBlocks(); - } - if (fgPgoConsistent) { Metrics.ProfileConsistentAfterInline++; diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index 06802181eea22..ed2d7538a3978 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -35,12 +35,6 @@ PhaseStatus Compiler::gsPhase() gsCopyShadowParams(); } - // If we needed to create any new BasicBlocks then renumber the blocks - if (fgBBcount > prevBBCount) - { - fgRenumberBlocks(); - } - madeChanges = true; } else diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index 4a30e7325e2af..56fd853acb0d6 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -1225,11 +1225,6 @@ PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) if (result == PhaseStatus::MODIFIED_EVERYTHING) { fgInvalidateDfsTree(); - - if (opts.OptimizationEnabled()) - { - fgRenumberBlocks(); - } } return result; diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 16e8e1d971446..6aa300b88509f 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -40,7 +40,7 @@ DataFlow::DataFlow(Compiler* pCompiler) PhaseStatus Compiler::optSetBlockWeights() { noway_assert(opts.OptimizationEnabled()); - bool madeChanges = fgRenumberBlocks(); + bool madeChanges = false; assert(m_dfsTree != nullptr); if (m_domTree == nullptr) @@ -54,6 +54,7 @@ PhaseStatus Compiler::optSetBlockWeights() if (m_dfsTree->HasCycle()) { + madeChanges = fgRenumberBlocks(); optMarkLoopHeads(); optFindAndScaleGeneralLoopBlocks(); } @@ -1335,13 +1336,11 @@ PhaseStatus Compiler::optUnrollLoops() m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); } - fgRenumberBlocks(); - DBEXEC(verbose, fgDispBasicBlocks()); } #ifdef DEBUG - fgDebugCheckBBlist(true); + fgDebugCheckBBlist(); #endif // DEBUG return anyIRchange ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; From aa7051709e6b8b56544e7f4590218299eb86118b Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 21 Nov 2024 13:30:45 -0500 Subject: [PATCH 6/6] Remove BB epoch quirk --- src/coreclr/jit/compiler.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 0b37345ce6161..163d01b8e0de5 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4921,15 +4921,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl if (doLoopHoisting) { - if (doBranchOpt) - { - // bbNum quirk: RBO uses the current BB epoch size to determine if loop hoisting created blocks. - // Ensure the BB epoch is up to date before running loop hoisting to avoid false positives. - // TODO-bbNum: Either replace this check with a breadcrumb like 'fgModified', - // or relax jump threading's prerequisites. - EnsureBasicBlockEpoch(); - } - // Hoist invariant code out of loops // DoPhase(this, PHASE_HOIST_LOOP_CODE, &Compiler::optHoistLoopCode);