JIT: Allow moving handler regions in fgRelocateEHRegions

src/coreclr/jit/codegenlinear.cpp

@@ -600,8 +600,19 @@ void CodeGen::genCodeForBBlist()
         /* Both stacks should always be empty on exit from a basic block */
         noway_assert(genStackLevel == 0);
 
-#ifdef TARGET_AMD64
-        bool emitNopBeforeEHRegion = false;
+        bool emitNop = false;
+
+#ifdef FEATURE_EH_WINDOWS_X86
+        // If a try region is not succeeded by its handler, it might fall into its non-EH successor.
+        // If the try region ends with a call instruction, and we don't emit a jump to the non-EH successor,
+        // make sure we emit a NOP instead so the call instruction's return address is within the try region.
+        //
+        if (!compiler->UsesFunclets() && block->hasTryIndex() && GetEmitter()->emitIsLastInsCall())
+        {
+            EHblkDsc* const HBtab = compiler->ehGetDsc(block->getTryIndex());
+            emitNop               = (HBtab->ebdTryLast == block) && HBtab->HasCatchHandler();
+        }
+#elif defined(TARGET_AMD64)
         // On AMD64, we need to generate a NOP after a call that is the last instruction of the block, in several
         // situations, to support proper exception handling semantics. This is mostly to ensure that when the stack
         // walker computes an instruction pointer for a frame, that instruction pointer is in the correct EH region.
@@ -626,7 +637,7 @@ void CodeGen::genCodeForBBlist()
                     case BBJ_ALWAYS:
                         // We might skip generating the jump via a peephole optimization.
                         // If that happens, make sure a NOP is emitted as the last instruction in the block.
-                        emitNopBeforeEHRegion = true;
+                        emitNop = true;
                         break;
 
                     case BBJ_THROW:
@@ -651,7 +662,7 @@ void CodeGen::genCodeForBBlist()
                 }
             }
         }
-#endif // TARGET_AMD64
+#endif // defined(TARGET_AMD64)
 
 #if FEATURE_LOOP_ALIGN
         auto SetLoopAlignBackEdge = [=](const BasicBlock* block, const BasicBlock* target) {
@@ -772,12 +783,12 @@ void CodeGen::genCodeForBBlist()
                 // If this block jumps to the next one, we might be able to skip emitting the jump
                 if (block->CanRemoveJumpToNext(compiler))
                 {
-#ifdef TARGET_AMD64
-                    if (emitNopBeforeEHRegion)
+#if defined(TARGET_AMD64) || defined(FEATURE_EH_WINDOWS_X86)
+                    if (emitNop)
                     {
                         instGen(INS_nop);
                     }
-#endif // TARGET_AMD64
+#endif // defined(TARGET_AMD64) || defined(FEATURE_EH_WINDOWS_X86)
 
                     removedJmp = true;
                     break;

src/coreclr/jit/compiler.cpp

@@ -4142,8 +4142,6 @@ void Compiler::compSetOptimizationLevel()
             codeGen->SetAlignLoops(JitConfig.JitAlignLoops() == 1);
         }
     }
-
-    fgCanRelocateEHRegions = true;
 }
 
 #if defined(TARGET_ARMARCH) || defined(TARGET_RISCV64)

src/coreclr/jit/compiler.h

@@ -5210,10 +5210,9 @@ class Compiler
     // - Rationalization links all nodes into linear form which is kept until
     //   the end of compilation. The first and last nodes are stored in the block.
     NodeThreading fgNodeThreading;
-    bool          fgCanRelocateEHRegions; // true if we are allowed to relocate the EH regions
-    weight_t      fgCalledCount;          // count of the number of times this method was called
-                                          // This is derived from the profile data
-                                          // or is BB_UNITY_WEIGHT when we don't have profile data
+    weight_t      fgCalledCount; // count of the number of times this method was called
+                                 // This is derived from the profile data
+                                 // or is BB_UNITY_WEIGHT when we don't have profile data
 
     bool fgFuncletsCreated; // true if the funclet creation phase has been run
 

src/coreclr/jit/emitxarch.cpp

@@ -2402,7 +2402,6 @@ unsigned emitter::emitOutputRexOrSimdPrefixIfNeeded(instruction ins, BYTE* dst,
     return 0;
 }
 
-#ifdef TARGET_AMD64
 /*****************************************************************************
  * Is the last instruction emitted a call instruction?
  */
@@ -2416,6 +2415,7 @@ bool emitter::emitIsLastInsCall()
     return false;
 }
 
+#ifdef TARGET_AMD64
 /*****************************************************************************
  * We're about to create an epilog. If the last instruction we output was a 'call',
  * then we need to insert a NOP, to allow for proper exception-handling behavior.

src/coreclr/jit/emitxarch.h

@@ -922,10 +922,10 @@ void emitIns_Call(EmitCallType          callType,
                   bool                  isJump   = false);
 // clang-format on
 
-#ifdef TARGET_AMD64
 // Is the last instruction emitted a call instruction?
 bool emitIsLastInsCall();
 
+#ifdef TARGET_AMD64
 // Insert a NOP at the end of the current instruction group if the last emitted instruction was a 'call',
 // because the next instruction group will be an epilog.
 void emitOutputPreEpilogNOP();

src/coreclr/jit/fgopt.cpp

@@ -933,12 +933,9 @@ bool Compiler::fgCanCompactBlocks(BasicBlock* block, BasicBlock* bNext)
 
     // We cannot compact two blocks in different EH regions.
     //
-    if (fgCanRelocateEHRegions)
+    if (!BasicBlock::sameEHRegion(block, bNext))
     {
-        if (!BasicBlock::sameEHRegion(block, bNext))
-        {
-            return false;
-        }
+        return false;
     }
 
     // If there is a switch predecessor don't bother because we'd have to update the uniquesuccs as well

src/coreclr/jit/jiteh.cpp

@@ -4166,75 +4166,35 @@ bool Compiler::fgRelocateEHRegions()
         printf("*************** In fgRelocateEHRegions()\n");
 #endif
 
-    if (fgCanRelocateEHRegions)
-    {
-        unsigned  XTnum;
-        EHblkDsc* HBtab;
+    unsigned  XTnum;
+    EHblkDsc* HBtab;
 
-        for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++)
+    for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++)
+    {
+        // Nested EH regions cannot be moved.
+        // Also we don't want to relocate an EH region that has a filter
+        if ((HBtab->ebdHandlerNestingLevel == 0) && !HBtab->HasFilter())
         {
-            // Nested EH regions cannot be moved.
-            // Also we don't want to relocate an EH region that has a filter
-            if ((HBtab->ebdHandlerNestingLevel == 0) && !HBtab->HasFilter())
-            {
-                bool movedTry = false;
-#if DEBUG
-                bool movedHnd = false;
-#endif // DEBUG
+            bool movedTry = false;
+            bool movedHnd = false;
 
-                // Only try to move the outermost try region
-                if (HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
-                {
-                    // Move the entire try region if it can be moved
-                    if (HBtab->ebdTryBeg->isRunRarely())
-                    {
-                        BasicBlock* bTryLastBB = fgRelocateEHRange(XTnum, FG_RELOCATE_TRY);
-                        if (bTryLastBB != NULL)
-                        {
-                            result   = true;
-                            movedTry = true;
-                        }
-                    }
-#if DEBUG
-                    if (verbose && movedTry)
-                    {
-                        printf("\nAfter relocating an EH try region");
-                        fgDispBasicBlocks();
-                        fgDispHandlerTab();
-
-                        // Make sure that the predecessor lists are accurate
-                        if (expensiveDebugCheckLevel >= 2)
-                        {
-                            fgDebugCheckBBlist();
-                        }
-                    }
-#endif // DEBUG
-                }
-
-                // Currently it is not good to move the rarely run handler regions to the end of the method
-                // because fgDetermineFirstColdBlock() must put the start of any handler region in the hot
-                // section.
-
-#if 0
-                // Now try to move the entire handler region if it can be moved.
-                // Don't try to move a finally handler unless we already moved the try region.
-                if (HBtab->ebdHndBeg->isRunRarely() &&
-                    !HBtab->ebdHndBeg->hasTryIndex() &&
-                    (movedTry || !HBtab->HasFinallyHandler()))
+            // Only try to move the outermost try region
+            if (HBtab->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
+            {
+                // Move the entire try region if it can be moved
+                if (HBtab->ebdTryBeg->isRunRarely())
                 {
-                    BasicBlock* bHndLastBB = fgRelocateEHRange(XTnum, FG_RELOCATE_HANDLER);
-                    if (bHndLastBB != NULL)
+                    BasicBlock* bTryLastBB = fgRelocateEHRange(XTnum, FG_RELOCATE_TRY);
+                    if (bTryLastBB != NULL)
                     {
                         result   = true;
-                        movedHnd = true;
+                        movedTry = true;
                     }
                 }
-#endif // 0
-
 #if DEBUG
-                if (verbose && movedHnd)
+                if (verbose && movedTry)
                 {
-                    printf("\nAfter relocating an EH handler region");
+                    printf("\nAfter relocating an EH try region");
                     fgDispBasicBlocks();
                     fgDispHandlerTab();
 
@@ -4246,6 +4206,34 @@ bool Compiler::fgRelocateEHRegions()
                 }
 #endif // DEBUG
             }
+
+            // Now try to move the entire handler region if it can be moved.
+            // Don't try to move a finally handler unless we already moved the try region.
+            if (HBtab->ebdHndBeg->isRunRarely() && !HBtab->ebdHndBeg->hasTryIndex() &&
+                (movedTry || !HBtab->HasFinallyHandler()))
+            {
+                BasicBlock* bHndLastBB = fgRelocateEHRange(XTnum, FG_RELOCATE_HANDLER);
+                if (bHndLastBB != NULL)
+                {
+                    result   = true;
+                    movedHnd = true;
+                }
+            }
+
+#if DEBUG
+            if (verbose && movedHnd)
+            {
+                printf("\nAfter relocating an EH handler region");
+                fgDispBasicBlocks();
+                fgDispHandlerTab();
+
+                // Make sure that the predecessor lists are accurate
+                if (expensiveDebugCheckLevel >= 2)
+                {
+                    fgDebugCheckBBlist();
+                }
+            }
+#endif // DEBUG
         }
     }