From a05d34befcf741aeb6d7ef8b8117569c91eeb031 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 14 Jan 2025 07:33:34 -0800 Subject: [PATCH] JIT: move second pass of EH opts earlier (#111364) to avoid messing up loop exit canonicalization (which is sensitive to loops with internal EH flow). Fixed #110959. Also, remove some of the ceremony around `fgModified` -- seems like this is no longer useful/needed. Defer deciding if we need a PSPSym until we create funclets. This removes unused PSPSym allocations in some cases. Also, there is no benefit to tracking the PSPSym, so stop doing that. --- src/coreclr/jit/compiler.cpp | 24 ++++++++++++------------ src/coreclr/jit/fgehopt.cpp | 6 ------ src/coreclr/jit/fgopt.cpp | 6 ------ src/coreclr/jit/flowgraph.cpp | 12 ++++++++++++ src/coreclr/jit/lclvars.cpp | 19 +++++++------------ src/coreclr/jit/optimizer.cpp | 9 --------- 6 files changed, 31 insertions(+), 45 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 52975e90bf8854..8385050b517bb4 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4825,6 +4825,18 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_COMPUTE_BLOCK_WEIGHTS, &Compiler::fgComputeBlockWeights); + // Try again to remove empty try finally/fault clauses + // + DoPhase(this, PHASE_EMPTY_FINALLY_2, &Compiler::fgRemoveEmptyFinally); + + // Remove empty try regions (try/finally) + // + DoPhase(this, PHASE_EMPTY_TRY_2, &Compiler::fgRemoveEmptyTry); + + // Remove empty try regions (try/catch/fault) + // + DoPhase(this, PHASE_EMPTY_TRY_CATCH_FAULT_2, &Compiler::fgRemoveEmptyTryCatchOrTryFault); + // Invert loops // DoPhase(this, PHASE_INVERT_LOOPS, &Compiler::optInvertLoops); @@ -4860,18 +4872,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_UNROLL_LOOPS, &Compiler::optUnrollLoops); - // Try again to remove empty try finally/fault clauses - // - DoPhase(this, PHASE_EMPTY_FINALLY_2, &Compiler::fgRemoveEmptyFinally); - - // Remove empty try regions (try/finally) - // - DoPhase(this, PHASE_EMPTY_TRY_2, &Compiler::fgRemoveEmptyTry); - - // Remove empty try regions (try/catch/fault) - // - DoPhase(this, PHASE_EMPTY_TRY_CATCH_FAULT_2, &Compiler::fgRemoveEmptyTryCatchOrTryFault); - // Compute dominators and exceptional entry blocks // DoPhase(this, PHASE_COMPUTE_DOMINATORS, &Compiler::fgComputeDominators); diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 18fa193db493cc..b7ecbd51176cdc 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -2458,12 +2458,6 @@ PhaseStatus Compiler::fgTailMergeThrows() assert(numCandidates < optNoReturnCallCount); optNoReturnCallCount -= numCandidates; - // If we altered flow, reset fgModified. Given where we sit in the - // phase list, flow-dependent side data hasn't been built yet, so - // nothing needs invalidation. - // - assert(fgModified); - fgModified = false; return PhaseStatus::MODIFIED_EVERYTHING; } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 9da0f846c4c3fb..c214a83152c9e7 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6780,12 +6780,6 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) madeChanges |= fgHeadMerge(block, early); } - // If we altered flow, reset fgModified. Given where we sit in the - // phase list, flow-dependent side data hasn't been built yet, so - // nothing needs invalidation. - // - fgModified = false; - return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 6a41de3c7ea34a..282335d1e23947 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2989,6 +2989,18 @@ PhaseStatus Compiler::fgCreateFunclets() assert(UsesFunclets()); assert(!fgFuncletsCreated); + // Allocate the PSPSym, if needed. PSPSym is not used by the NativeAOT ABI + if (!IsTargetAbi(CORINFO_NATIVEAOT_ABI)) + { + if (ehNeedsPSPSym()) + { + lvaPSPSym = lvaGrabTempWithImplicitUse(false DEBUGARG("PSPSym")); + LclVarDsc* lclPSPSym = lvaGetDesc(lvaPSPSym); + lclPSPSym->lvType = TYP_I_IMPL; + lvaSetVarDoNotEnregister(lvaPSPSym DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr)); + } + } + fgCreateFuncletPrologBlocks(); unsigned XTnum; diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 883ae5325bb7dd..44f7f7e3664787 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -4073,6 +4073,13 @@ void Compiler::lvaSortByRefCount() } #endif + // No benefit in tracking the PSPSym (if any) + // + if (lclNum == lvaPSPSym) + { + varDsc->lvTracked = 0; + } + // Are we not optimizing and we have exception handlers? // if so mark all args and locals "do not enregister". // @@ -4747,18 +4754,6 @@ PhaseStatus Compiler::lvaMarkLocalVars() #endif // FEATURE_EH_WINDOWS_X86 - // PSPSym is not used by the NativeAOT ABI - if (!IsTargetAbi(CORINFO_NATIVEAOT_ABI)) - { - if (UsesFunclets() && ehNeedsPSPSym()) - { - lvaPSPSym = lvaGrabTempWithImplicitUse(false DEBUGARG("PSPSym")); - LclVarDsc* lclPSPSym = lvaGetDesc(lvaPSPSym); - lclPSPSym->lvType = TYP_I_IMPL; - lvaSetVarDoNotEnregister(lvaPSPSym DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr)); - } - } - #ifdef JIT32_GCENCODER // LocAllocSPvar is only required by the implicit frame layout expected by the VM on x86. Whether // a function contains a Localloc is conveyed in the GC information, in the InfoHdrSmall.localloc diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index e7ee7c2f230471..2ed5909eab2aa5 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -2351,7 +2351,6 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block) PhaseStatus Compiler::optInvertLoops() { noway_assert(opts.OptimizationEnabled()); - noway_assert(fgModified == false); #if defined(OPT_CONFIG) if (!JitConfig.JitDoLoopInversion()) @@ -2387,13 +2386,6 @@ PhaseStatus Compiler::optInvertLoops() } } - if (fgModified) - { - // Reset fgModified here as we've done a consistent set of edits. - // - fgModified = false; - } - return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } @@ -2410,7 +2402,6 @@ PhaseStatus Compiler::optInvertLoops() PhaseStatus Compiler::optOptimizeFlow() { noway_assert(opts.OptimizationEnabled()); - noway_assert(fgModified == false); fgUpdateFlowGraph(/* doTailDuplication */ true); fgReorderBlocks(/* useProfile */ false);