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.

Author: AndyAyersMS

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

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

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

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;

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

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