Reduce differences between successor iterators (#93445)

* Reduce differences between successor iterators

The NumSucc/GetSucc/Succs iterators have two variants. It is hard to understand
the distinction. The goal of this change is to reduce the differences,
so the only difference is the SWITCH case (iterating either all successors
or only unique successors).

So:
1. BBJ_EHFILTERRET: always return the single successor, which is the first
block of the filter handler.
2. BBJ_EHFINALLYRET: Move to standard successor iterator

* Review comments

Author: BruceForstall

src/coreclr/jit/block.cpp

@@ -1081,14 +1081,13 @@ unsigned BasicBlock::NumSucc() const
     {
         case BBJ_THROW:
         case BBJ_RETURN:
-        case BBJ_EHFINALLYRET:
         case BBJ_EHFAULTRET:
-        case BBJ_EHFILTERRET:
             return 0;
 
         case BBJ_CALLFINALLY:
         case BBJ_ALWAYS:
         case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
         case BBJ_LEAVE:
         case BBJ_NONE:
             return 1;
@@ -1103,6 +1102,23 @@ unsigned BasicBlock::NumSucc() const
                 return 2;
             }
 
+        case BBJ_EHFINALLYRET:
+            // We may call this method before we realize we have invalid IL. Tolerate.
+            //
+            if (!hasHndIndex())
+            {
+                return 0;
+            }
+
+            // We may call this before we've computed the BBJ_EHFINALLYRET successors in the importer. Tolerate.
+            //
+            if (bbJumpEhf == nullptr)
+            {
+                return 0;
+            }
+
+            return bbJumpEhf->bbeCount;
+
         case BBJ_SWITCH:
             return bbJumpSwt->bbsCount;
 
@@ -1128,6 +1144,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const
         case BBJ_CALLFINALLY:
         case BBJ_ALWAYS:
         case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
         case BBJ_LEAVE:
             return bbJumpDest;
 
@@ -1146,6 +1163,9 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const
                 return bbJumpDest;
             }
 
+        case BBJ_EHFINALLYRET:
+            return bbJumpEhf->bbeSuccs[i];
+
         case BBJ_SWITCH:
             return bbJumpSwt->bbsDstTab[i];
 

src/coreclr/jit/block.h

@@ -887,15 +887,7 @@ struct BasicBlock : private LIR::Range
     // GetSucc() without a Compiler*.
     //
     // The behavior of NumSucc()/GetSucc() is different when passed a Compiler* for blocks that end in:
-    // (1) BBJ_EHFINALLYRET (a return from a finally block)
-    // (2) BBJ_EHFILTERRET (a return from EH filter block)
-    // (3) BBJ_SWITCH
-    //
-    // For BBJ_EHFINALLYRET, if no Compiler* is passed, then the block is considered to have no
-    // successor. If Compiler* is passed, we use the actual successors.
-    //
-    // Similarly, BBJ_EHFILTERRET blocks are assumed to have no successors if Compiler* is not passed; if
-    // Compiler* is passed, NumSucc/GetSucc yields the first block of the try block's handler.
+    // (1) BBJ_SWITCH
     //
     // For BBJ_SWITCH, if Compiler* is not passed, then all switch successors are returned. If Compiler*
     // is passed, then only unique switch successors are returned; the duplicate successors are omitted.
@@ -1751,9 +1743,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
     {
         case BBJ_THROW:
         case BBJ_RETURN:
-        case BBJ_EHFINALLYRET:
         case BBJ_EHFAULTRET:
-        case BBJ_EHFILTERRET:
             // We don't need m_succs.
             m_begin = nullptr;
             m_end   = nullptr;
@@ -1762,6 +1752,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
         case BBJ_CALLFINALLY:
         case BBJ_ALWAYS:
         case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
         case BBJ_LEAVE:
             m_succs[0] = block->GetJumpDest();
             m_begin    = &m_succs[0];
@@ -1791,6 +1782,22 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
             }
             break;
 
+        case BBJ_EHFINALLYRET:
+            // We don't use the m_succs in-line data; use the existing successor table in the block.
+            // We must tolerate iterating successors early in the system, before EH_FINALLYRET successors have
+            // been computed.
+            if (block->GetJumpEhf() == nullptr)
+            {
+                m_begin = nullptr;
+                m_end   = nullptr;
+            }
+            else
+            {
+                m_begin = block->GetJumpEhf()->bbeSuccs;
+                m_end   = block->GetJumpEhf()->bbeSuccs + block->GetJumpEhf()->bbeCount;
+            }
+            break;
+
         case BBJ_SWITCH:
             // We don't use the m_succs in-line data for switches; use the existing jump table in the block.
             assert(block->GetJumpSwt() != nullptr);

src/coreclr/jit/compiler.hpp

@@ -616,12 +616,6 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func)
 {
     switch (bbJumpKind)
     {
-        case BBJ_EHFILTERRET:
-            RETURN_ON_ABORT(func(bbJumpDest));
-            RETURN_ON_ABORT(VisitEHSuccessors(comp, this, func));
-            RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, this, bbJumpDest, func));
-            break;
-
         case BBJ_EHFINALLYRET:
             for (unsigned i = 0; i < bbJumpEhf->bbeCount; i++)
             {
@@ -639,6 +633,7 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func)
 
         case BBJ_CALLFINALLY:
         case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
         case BBJ_LEAVE:
             RETURN_ON_ABORT(func(bbJumpDest));
             RETURN_ON_ABORT(VisitEHSuccessors(comp, this, func));
@@ -728,10 +723,6 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func)
 {
     switch (bbJumpKind)
     {
-        case BBJ_EHFILTERRET:
-            RETURN_ON_ABORT(func(bbJumpDest));
-            break;
-
         case BBJ_EHFINALLYRET:
             for (unsigned i = 0; i < bbJumpEhf->bbeCount; i++)
             {
@@ -741,6 +732,7 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func)
 
         case BBJ_CALLFINALLY:
         case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
         case BBJ_LEAVE:
         case BBJ_ALWAYS:
             RETURN_ON_ABORT(func(bbJumpDest));

src/coreclr/jit/importer.cpp

@@ -11471,7 +11471,9 @@ void Compiler::impImportBlock(BasicBlock* block)
         // This will re-import all the successors of block (as well as each of their predecessors)
         impReimportSpillClique(block);
 
-        // For blocks that haven't been imported yet, we still need to mark them as pending import.
+        // We don't expect to see BBJ_EHFILTERRET here.
+        assert(!block->KindIs(BBJ_EHFILTERRET));
+
         for (BasicBlock* const succ : block->Succs())
         {
             if ((succ->bbFlags & BBF_IMPORTED) == 0)
@@ -11484,10 +11486,15 @@ void Compiler::impImportBlock(BasicBlock* block)
     {
         // otherwise just import the successors of block
 
-        /* Does this block jump to any other blocks? */
-        for (BasicBlock* const succ : block->Succs())
+        // Does this block jump to any other blocks?
+        // Filter successor from BBJ_EHFILTERRET have already been handled above in the call
+        // to impVerifyEHBlock().
+        if (!block->KindIs(BBJ_EHFILTERRET))
         {
-            impImportBlockPending(succ);
+            for (BasicBlock* const succ : block->Succs())
+            {
+                impImportBlockPending(succ);
+            }
         }
     }
 }