Explicitly represent BBJ_EHFINALLYRET successors (#93377)

* Explicitly represent BBJ_EHFINALLYRET successors

Currently, BBJ_EHFINALLYRET blocks have no explicit successors in the IR.
To implement successor iteration, a very expensive process is followed to
(1) find the region of blocks where a BBJ_CALLFINALLY block calling the
`finally` might be found, (2) search the region for such blocks, and (3)
return as a successor all the BBJ_ALWAYS blocks in the corresponding
BBJ_CALLFINALLY/BBJ_ALWAYS pair.

Change the IR to explicitly represent and maintain this list of successors
for BBJ_EHFINALLYRET blocks. The representation is a simple array of
`BasicBlock*`, similar to how BBJ_SWITCH block targets are represented.

Fixes #84278

Notes:
1. The BBJ_EHFINALLYRET successors are computed in `impFixPredLists()`.
There are various dumpers that run before this, so we need to tolerate
incomplete successor information in some places.
2. `ehGetCallFinallyBlockRange()` is still used by some code. I changed the
semantics to return a `[first..last]` range inclusive of `last` instead of
the previous `[beginning..end)` range exclusive of `end`. This makes it easier
to use with our BasicBlock iterators.

* Review suggestions

src/coreclr/jit/block.cpp

@@ -613,8 +613,28 @@ void BasicBlock::dspJumpKind()
     switch (bbJumpKind)
     {
         case BBJ_EHFINALLYRET:
+        {
+            printf(" ->");
+
+            // Early in compilation, we display the jump kind before the BBJ_EHFINALLYRET successors have been set.
+            if (bbJumpEhf == nullptr)
+            {
+                printf(" ????");
+            }
+            else
+            {
+                const unsigned     jumpCnt = bbJumpEhf->bbeCount;
+                BasicBlock** const jumpTab = bbJumpEhf->bbeSuccs;
+
+                for (unsigned i = 0; i < jumpCnt; i++)
+                {
+                    printf("%c" FMT_BB, (i == 0) ? ' ' : ',', jumpTab[i]->bbNum);
+                }
+            }
+
             printf(" (finret)");
             break;
+        }
 
         case BBJ_EHFAULTRET:
             printf(" (falret)");
@@ -1126,6 +1146,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const
             else
             {
                 assert(i == 1);
+                assert(bbNext != bbJumpDest);
                 return bbJumpDest;
             }
 
@@ -1164,7 +1185,15 @@ unsigned BasicBlock::NumSucc(Compiler* comp)
             {
                 return 0;
             }
-            return comp->fgNSuccsOfFinallyRet(this);
+
+            // 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_CALLFINALLY:
         case BBJ_ALWAYS:
@@ -1213,15 +1242,14 @@ BasicBlock* BasicBlock::GetSucc(unsigned i, Compiler* comp)
     switch (bbJumpKind)
     {
         case BBJ_EHFILTERRET:
-        {
             // Handler is the (sole) normal successor of the filter.
             assert(comp->fgFirstBlockOfHandler(this) == bbJumpDest);
             return bbJumpDest;
-        }
 
         case BBJ_EHFINALLYRET:
-            // Note: the following call is expensive.
-            return comp->fgSuccOfFinallyRet(this, i);
+            assert(bbJumpEhf != nullptr);
+            assert(i < bbJumpEhf->bbeCount);
+            return bbJumpEhf->bbeSuccs[i];
 
         case BBJ_CALLFINALLY:
         case BBJ_ALWAYS:
@@ -1240,6 +1268,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i, Compiler* comp)
             else
             {
                 assert(i == 1);
+                assert(bbNext != bbJumpDest);
                 return bbJumpDest;
             }
 
@@ -1645,6 +1674,24 @@ BBswtDesc::BBswtDesc(Compiler* comp, const BBswtDesc* other)
     }
 }
 
+//------------------------------------------------------------------------
+// BBehfDesc copy ctor: copy a EHFINALLYRET descriptor
+//
+// Arguments:
+//    comp - compiler instance
+//    other - existing descriptor to copy
+//
+BBehfDesc::BBehfDesc(Compiler* comp, const BBehfDesc* other) : bbeCount(other->bbeCount)
+{
+    // Allocate and fill in a new dst tab
+    //
+    bbeSuccs = new (comp, CMK_BasicBlock) BasicBlock*[bbeCount];
+    for (unsigned i = 0; i < bbeCount; i++)
+    {
+        bbeSuccs[i] = other->bbeSuccs[i];
+    }
+}
+
 //------------------------------------------------------------------------
 // unmarkLoopAlign: Unmarks the LOOP_ALIGN flag from the block and reduce the
 //                  loop alignment count.

src/coreclr/jit/block.h

@@ -104,6 +104,7 @@ struct BasicBlockList;
 struct FlowEdge;
 struct EHblkDsc;
 struct BBswtDesc;
+struct BBehfDesc;
 
 struct StackEntry
 {
@@ -355,6 +356,20 @@ class BBSwitchTargetList
     BBArrayIterator end() const;
 };
 
+// BBEhfSuccList: adapter class for forward iteration of BBJ_EHFINALLYRET blocks, using range-based `for`,
+// normally used via BasicBlock::EHFinallyRetSuccs(), e.g.:
+//    for (BasicBlock* const succ : block->EHFinallyRetSuccs()) ...
+//
+class BBEhfSuccList
+{
+    BBehfDesc* m_bbeDesc;
+
+public:
+    BBEhfSuccList(BBehfDesc* bbeDesc);
+    BBArrayIterator begin() const;
+    BBArrayIterator end() const;
+};
+
 //------------------------------------------------------------------------
 // BasicBlockFlags: a bitmask of flags for BasicBlock
 //
@@ -519,6 +534,7 @@ struct BasicBlock : private LIR::Range
         unsigned    bbJumpOffs; // PC offset (temporary only)
         BasicBlock* bbJumpDest; // basic block
         BBswtDesc*  bbJumpSwt;  // switch descriptor
+        BBehfDesc*  bbJumpEhf;  // BBJ_EHFINALLYRET descriptor
     };
 
 public:
@@ -646,6 +662,26 @@ struct BasicBlock : private LIR::Range
         bbJumpSwt  = jumpSwt;
     }
 
+    BBehfDesc* GetJumpEhf() const
+    {
+        assert(KindIs(BBJ_EHFINALLYRET));
+        return bbJumpEhf;
+    }
+
+    void SetJumpEhf(BBehfDesc* jumpEhf)
+    {
+        assert(KindIs(BBJ_EHFINALLYRET));
+        bbJumpEhf = jumpEhf;
+    }
+
+    void SetJumpKindAndTarget(BBjumpKinds jumpKind, BBehfDesc* jumpEhf)
+    {
+        assert(jumpKind == BBJ_EHFINALLYRET);
+        assert(jumpEhf != nullptr);
+        bbJumpKind = jumpKind;
+        bbJumpEhf  = jumpEhf;
+    }
+
     BasicBlockFlags bbFlags;
 
     static_assert_no_msg((BBF_SPLIT_NONEXIST & BBF_SPLIT_LOST) == 0);
@@ -856,22 +892,17 @@ struct BasicBlock : private LIR::Range
     // (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 figure out the actual successors. Some cases will want one behavior,
-    // other cases the other. For example, IL verification requires that these blocks end in an empty operand
-    // stack, and since the dataflow analysis of IL verification is concerned only with the contents of the
-    // operand stack, we can consider the finally block to have no successors. But a more general dataflow
-    // analysis that is tracking the contents of local variables might want to consider *all* successors,
-    // and would pass the current Compiler object.
+    // 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.
     //
     // 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.
     //
-    // Note that for BBJ_COND, which has two successors (fall through and condition true branch target),
-    // only the unique targets are returned. Thus, if both targets are the same, NumSucc() will only return 1
-    // instead of 2.
+    // Note that for BBJ_COND, which has two successors (fall through (condition false), and condition true
+    // branch target), only the unique targets are returned. Thus, if both targets are the same, NumSucc()
+    // will only return 1 instead of 2.
     //
     // NumSucc: Returns the number of successors of "this".
     unsigned NumSucc() const;
@@ -881,7 +912,7 @@ struct BasicBlock : private LIR::Range
     BasicBlock* GetSucc(unsigned i) const;
     BasicBlock* GetSucc(unsigned i, Compiler* comp);
 
-    // SwitchTargets: convenience methods for enabling range-based `for` iteration over a switch block's targets, e.g.:
+    // SwitchTargets: convenience method for enabling range-based `for` iteration over a switch block's targets, e.g.:
     //    for (BasicBlock* const bTarget : block->SwitchTargets()) ...
     //
     BBSwitchTargetList SwitchTargets() const
@@ -890,6 +921,16 @@ struct BasicBlock : private LIR::Range
         return BBSwitchTargetList(bbJumpSwt);
     }
 
+    // EHFinallyRetSuccs: convenience method for enabling range-based `for` iteration over BBJ_EHFINALLYRET block
+    // successors, e.g.:
+    //    for (BasicBlock* const succ : block->EHFinallyRetSuccs()) ...
+    //
+    BBEhfSuccList EHFinallyRetSuccs() const
+    {
+        assert(bbJumpKind == BBJ_EHFINALLYRET);
+        return BBEhfSuccList(bbJumpEhf);
+    }
+
     BasicBlock* GetUniquePred(Compiler* comp) const;
 
     BasicBlock* GetUniqueSucc() const;
@@ -1668,6 +1709,39 @@ inline BBArrayIterator BBSwitchTargetList::end() const
     return BBArrayIterator(m_bbsDesc->bbsDstTab + m_bbsDesc->bbsCount);
 }
 
+// BBehfDesc -- descriptor for a BBJ_EHFINALLYRET block
+//
+struct BBehfDesc
+{
+    BasicBlock** bbeSuccs; // array of `BasicBlock*` pointing to BBJ_EHFINALLYRET block successors
+    unsigned     bbeCount; // size of `bbeSuccs` array
+
+    BBehfDesc() : bbeSuccs(nullptr), bbeCount(0)
+    {
+    }
+
+    BBehfDesc(Compiler* comp, const BBehfDesc* other);
+};
+
+// BBEhfSuccList out-of-class-declaration implementations (here due to C++ ordering requirements).
+//
+
+inline BBEhfSuccList::BBEhfSuccList(BBehfDesc* bbeDesc) : m_bbeDesc(bbeDesc)
+{
+    assert(m_bbeDesc != nullptr);
+    assert((m_bbeDesc->bbeSuccs != nullptr) || (m_bbeDesc->bbeCount == 0));
+}
+
+inline BBArrayIterator BBEhfSuccList::begin() const
+{
+    return BBArrayIterator(m_bbeDesc->bbeSuccs);
+}
+
+inline BBArrayIterator BBEhfSuccList::end() const
+{
+    return BBArrayIterator(m_bbeDesc->bbeSuccs + m_bbeDesc->bbeCount);
+}
+
 // BBSuccList out-of-class-declaration implementations
 //
 inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)

src/coreclr/jit/clrjit.natvis

@@ -23,6 +23,7 @@ Documentation for VS debugger format specifiers: https://docs.microsoft.com/en-u
   <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_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>
   </Type>
 

src/coreclr/jit/compiler.h

@@ -2322,11 +2322,11 @@ class Compiler
     // lives in a filter.)
     unsigned ehGetCallFinallyRegionIndex(unsigned finallyIndex, bool* inTryRegion);
 
-    // Find the range of basic blocks in which all BBJ_CALLFINALLY will be found that target the 'finallyIndex' region's
-    // handler. Set begBlk to the first block, and endBlk to the block after the last block of the range
-    // (nullptr if the last block is the last block in the program).
+    // Find the range of basic blocks in which all BBJ_CALLFINALLY will be found that target the 'finallyIndex'
+    // region's handler. Set `firstBlock` to the first block, and `lastBlock` to the last block of the range
+    // (the range is inclusive of `firstBlock` and `lastBlock`). Thus, the range is [firstBlock .. lastBlock].
     // Precondition: 'finallyIndex' is the EH region of a try/finally clause.
-    void ehGetCallFinallyBlockRange(unsigned finallyIndex, BasicBlock** begBlk, BasicBlock** endBlk);
+    void ehGetCallFinallyBlockRange(unsigned finallyIndex, BasicBlock** firstBlock, BasicBlock** lastBlock);
 
 #ifdef DEBUG
     // Given a BBJ_CALLFINALLY block and the EH region index of the finally it is calling, return
@@ -5386,21 +5386,6 @@ class Compiler
     PhaseStatus fgInsertGCPolls();
     BasicBlock* fgCreateGCPoll(GCPollType pollType, BasicBlock* block);
 
-    // Requires that "block" is a block that returns from
-    // a finally.  Returns the number of successors (jump targets of
-    // of blocks in the covered "try" that did a "LEAVE".)
-    unsigned fgNSuccsOfFinallyRet(BasicBlock* block);
-
-    // Requires that "block" is a block that returns (in the sense of BBJ_EHFINALLYRET) from
-    // a finally.  Returns its "i"th successor (jump targets of
-    // of blocks in the covered "try" that did a "LEAVE".)
-    // Requires that "i" < fgNSuccsOfFinallyRet(block).
-    BasicBlock* fgSuccOfFinallyRet(BasicBlock* block, unsigned i);
-
-private:
-    // Factor out common portions of the impls of the methods above.
-    void fgSuccOfFinallyRetWork(BasicBlock* block, unsigned i, BasicBlock** bres, unsigned* nres);
-
 public:
     // For many purposes, it is desirable to be able to enumerate the *distinct* targets of a switch statement,
     // skipping duplicate targets.  (E.g., in flow analyses that are only interested in the set of possible targets.)
@@ -5476,6 +5461,12 @@ class Compiler
 
     void fgReplaceSwitchJumpTarget(BasicBlock* blockSwitch, BasicBlock* newTarget, BasicBlock* oldTarget);
 
+    void fgChangeEhfBlock(BasicBlock* oldBlock, BasicBlock* newBlock);
+
+    void fgReplaceEhfSuccessor(BasicBlock* block, BasicBlock* oldSucc, BasicBlock* newSucc);
+
+    void fgRemoveEhfSuccessor(BasicBlock* block, BasicBlock* succ);
+
     void fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, BasicBlock* oldTarget);
 
     void fgReplacePred(BasicBlock* block, BasicBlock* oldPred, BasicBlock* newPred);
@@ -5614,8 +5605,7 @@ class Compiler
 
     void fgRemoveReturnBlock(BasicBlock* block);
 
-    /* Helper code that has been factored out */
-    inline void fgConvertBBToThrowBB(BasicBlock* block);
+    void fgConvertBBToThrowBB(BasicBlock* block);
 
     bool fgCastNeeded(GenTree* tree, var_types toType);