Improve EH dead code elimination (#93428)

* Improve EH dead code elimination

Update `fgRemoveDeadBlocks()` to only treat EH handlers as reachable
if their corresponding `try` block entry is reachable.

This causes more dead code to be eliminated in handlers that are
not reachable.

There is a little more work to do to completely clean these up:
1. If a `try` is not reachable, we should get rid of its entire EH table
entry. Currently, its block is generated as `int3` (on xarch).
2. Even though the handler is not reachable, we still generate the prolog
followed by `int3` for the body.

Contributes to #82335

* Fix BBJ_EHFILTERRET handling in fgRemoveBlockAsPred

fgRemoveBlockAsPred had special handling to not decrease the bbRefs count
of the filter-handler targeted by BBJ_EHFILTERRET. This is incorrect, as
the filter-handler already has an extra "beginning of handler" extra
ref count. Possibly this code was never invoked before as filters were
not previously removed.

Also, fix the JitDump output of the filter ret block in block dumps.

* Put back arm32 BBJ_CALLFINALLY/BBJ_ALWAYS pair check

Author: BruceForstall

src/coreclr/jit/block.cpp

@@ -637,7 +637,7 @@ void BasicBlock::dspJumpKind()
             break;
 
         case BBJ_EHFILTERRET:
-            printf(" (fltret)");
+            printf(" -> " FMT_BB " (fltret)", bbJumpDest->bbNum);
             break;
 
         case BBJ_EHCATCHRET:

src/coreclr/jit/clrjit.natvis

@@ -21,7 +21,7 @@ Documentation for VS debugger format specifiers: https://docs.microsoft.com/en-u
   </Type>
 
   <Type Name="BasicBlock">
-    <DisplayString Condition="bbJumpKind==BBJ_COND || bbJumpKind==BBJ_ALWAYS || bbJumpKind==BBJ_LEAVE || bbJumpKind==BBJ_EHCATCHRET || bbJumpKind==BBJ_CALLFINALLY">BB{bbNum,d}->BB{bbJumpDest->bbNum,d}; {bbJumpKind,en}</DisplayString>
+    <DisplayString Condition="bbJumpKind==BBJ_COND || bbJumpKind==BBJ_ALWAYS || bbJumpKind==BBJ_LEAVE || bbJumpKind==BBJ_EHCATCHRET || bbJumpKind==BBJ_CALLFINALLY || bbJumpKind==BBJ_EHFILTERRET">BB{bbNum,d}->BB{bbJumpDest->bbNum,d}; {bbJumpKind,en}</DisplayString>
     <DisplayString Condition="bbJumpKind==BBJ_SWITCH">BB{bbNum,d}; {bbJumpKind,en}; {bbJumpSwt->bbsCount} cases</DisplayString>
     <DisplayString Condition="bbJumpKind==BBJ_EHFINALLYRET">BB{bbNum,d}; {bbJumpKind,en}; {bbJumpEhf->bbeCount} succs</DisplayString>
     <DisplayString>BB{bbNum,d}; {bbJumpKind,en}</DisplayString>

src/coreclr/jit/fgbasic.cpp

@@ -268,7 +268,7 @@ bool Compiler::fgEnsureFirstBBisScratch()
         }
 
         // The first block has an implicit ref count which we must
-        // remove. Note the ref count could be greater that one, if
+        // remove. Note the ref count could be greater than one, if
         // the first block is not scratch and is targeted by a
         // branch.
         assert(fgFirstBB->bbRefs >= 1);

src/coreclr/jit/fgdiagnostic.cpp

@@ -2077,7 +2077,8 @@ void Compiler::fgTableDispBasicBlock(BasicBlock* block, int ibcColWidth /* = 0 *
                 break;
 
             case BBJ_EHFILTERRET:
-                printf("%*s        (fltret)", maxBlockNumWidth - 2, "");
+                printf("-> " FMT_BB "%*s (fltret)", block->GetJumpDest()->bbNum,
+                       maxBlockNumWidth - max(CountDigits(block->GetJumpDest()->bbNum), 2), "");
                 break;
 
             case BBJ_EHCATCHRET:

src/coreclr/jit/fgflow.cpp

@@ -346,6 +346,7 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block)
         case BBJ_CALLFINALLY:
         case BBJ_ALWAYS:
         case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
             fgRemoveRefPred(block->GetJumpDest(), block);
             break;
 
@@ -358,11 +359,6 @@ void Compiler::fgRemoveBlockAsPred(BasicBlock* block)
             fgRemoveRefPred(block->Next(), block);
             break;
 
-        case BBJ_EHFILTERRET:
-            block->GetJumpDest()->bbRefs++; // To compensate the bbRefs-- inside fgRemoveRefPred
-            fgRemoveRefPred(block->GetJumpDest(), block);
-            break;
-
         case BBJ_EHFINALLYRET:
             for (BasicBlock* const succ : block->EHFinallyRetSuccs())
             {

src/coreclr/jit/fgopt.cpp

@@ -437,6 +437,11 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock)
             // to properly set the info.compProfilerCallback flag.
             continue;
         }
+        else if ((block->bbFlags & BBF_DONT_REMOVE) && block->isEmpty() && block->KindIs(BBJ_THROW))
+        {
+            // We already converted a non-removable block to a throw; don't bother processing it again.
+            continue;
+        }
         else if (!canRemoveBlock(block))
         {
             continue;
@@ -458,6 +463,8 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock)
 
             // Unmark the block as removed, clear BBF_INTERNAL, and set BBJ_IMPORTED
 
+            JITDUMP("Converting BBF_DONT_REMOVE block " FMT_BB " to BBJ_THROW\n", block->bbNum);
+
             // The successors may be unreachable after this change.
             changed |= block->NumSucc() > 0;
 
@@ -631,34 +638,6 @@ bool Compiler::fgRemoveDeadBlocks()
 {
     JITDUMP("\n*************** In fgRemoveDeadBlocks()");
 
-    jitstd::list<BasicBlock*> worklist(jitstd::allocator<void>(getAllocator(CMK_Reachability)));
-
-    worklist.push_back(fgFirstBB);
-
-    // Do not remove handler blocks
-    for (EHblkDsc* const HBtab : EHClauses(this))
-    {
-        if (HBtab->HasFilter())
-        {
-            worklist.push_back(HBtab->ebdFilter);
-        }
-        worklist.push_back(HBtab->ebdHndBeg);
-    }
-
-#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
-    // For ARM code, prevent creating retless calls by adding the BBJ_ALWAYS to the "fgAlwaysBlks" list.
-    for (BasicBlock* const block : Blocks())
-    {
-        if (block->KindIs(BBJ_CALLFINALLY))
-        {
-            assert(block->isBBCallAlwaysPair());
-
-            // Don't remove the BBJ_ALWAYS block that is only here for the unwinder.
-            worklist.push_back(block->Next());
-        }
-    }
-#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
-
     unsigned prevFgCurBBEpoch = fgCurBBEpoch;
     EnsureBasicBlockEpoch();
 
@@ -673,6 +652,9 @@ bool Compiler::fgRemoveDeadBlocks()
 
     BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this));
 
+    jitstd::list<BasicBlock*> worklist(jitstd::allocator<void>(getAllocator(CMK_Reachability)));
+    worklist.push_back(fgFirstBB);
+
     // Visit all the reachable blocks, everything else can be removed
     while (!worklist.empty())
     {
@@ -690,6 +672,43 @@ bool Compiler::fgRemoveDeadBlocks()
         {
             worklist.push_back(succ);
         }
+
+        // Add all the "EH" successors. For every `try`, add its handler (including filter) to the worklist.
+        if (bbIsTryBeg(block))
+        {
+            // Due to EH normalization, a block can only be the start of a single `try` region, with the exception
+            // of mutually-protect regions.
+            assert(block->hasTryIndex());
+            unsigned  tryIndex = block->getTryIndex();
+            EHblkDsc* ehDsc    = ehGetDsc(tryIndex);
+            for (;;)
+            {
+                worklist.push_back(ehDsc->ebdHndBeg);
+                if (ehDsc->HasFilter())
+                {
+                    worklist.push_back(ehDsc->ebdFilter);
+                }
+                tryIndex = ehDsc->ebdEnclosingTryIndex;
+                if (tryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
+                {
+                    break;
+                }
+                ehDsc = ehGetDsc(tryIndex);
+                if (ehDsc->ebdTryBeg != block)
+                {
+                    break;
+                }
+            }
+        }
+
+#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+        // For ARM code, always keep BBJ_CALLFINALLY/BBJ_ALWAYS as a pair
+        if (block->KindIs(BBJ_CALLFINALLY))
+        {
+            assert(block->isBBCallAlwaysPair());
+            worklist.push_back(block->Next());
+        }
+#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
     }
 
     // Track if there is any unreachable block. Even if it is marked with
@@ -702,14 +721,14 @@ bool Compiler::fgRemoveDeadBlocks()
     // any of the fgFirstBB, handler, filter or BBJ_ALWAYS (Arm) blocks.
     auto isBlockRemovable = [&](BasicBlock* block) -> bool {
         bool isVisited   = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum);
-        bool isRemovable = (!isVisited || block->bbRefs == 0);
+        bool isRemovable = !isVisited || (block->bbRefs == 0);
 
 #if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
-        isRemovable &=
-            !block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
+        // Can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair, even if bbRefs == 0.
+        isRemovable &= !block->isBBCallAlwaysPairTail();
 #endif
-        hasUnreachableBlock |= isRemovable;
 
+        hasUnreachableBlock |= isRemovable;
         return isRemovable;
     };
 
@@ -738,6 +757,7 @@ bool Compiler::fgRemoveDeadBlocks()
     fgVerifyHandlerTab();
     fgDebugCheckBBlist(false);
 #endif // DEBUG
+
     return hasUnreachableBlock;
 }