Unify EH boundary handling (#2049)

src/coreclr/src/jit/assertionprop.cpp

@@ -379,7 +379,7 @@ void Compiler::optAddCopies()
 #if defined(FEATURE_EH_FUNCLETS)
                     // With funclets, this is only used for BBJ_CALLFINALLY/BBJ_ALWAYS pairs. For x86, it is also used
                     // as the "final step" block for leaving finallys.
-                    assert((block->bbPrev != nullptr) && block->bbPrev->isBBCallAlwaysPair());
+                    assert(block->isBBCallAlwaysPairTail());
 #endif // FEATURE_EH_FUNCLETS
 #ifdef DEBUG
                     if (verbose)

src/coreclr/src/jit/block.cpp

@@ -72,8 +72,7 @@ EHSuccessorIterPosition::EHSuccessorIterPosition(Compiler* comp, BasicBlock* blo
     // can occur within it, so clear m_curTry if it's non-null.
     if (m_curTry != nullptr)
     {
-        BasicBlock* beforeBlock = block->bbPrev;
-        if (beforeBlock != nullptr && beforeBlock->isBBCallAlwaysPair())
+        if (block->isBBCallAlwaysPairTail())
         {
             m_curTry = nullptr;
         }
@@ -176,17 +175,15 @@ flowList* Compiler::BlockPredsWithEH(BasicBlock* blk)
 
         // Now add all blocks handled by this handler (except for second blocks of BBJ_CALLFINALLY/BBJ_ALWAYS pairs;
         // these cannot cause transfer to the handler...)
-        BasicBlock* prevBB = nullptr;
-
         // TODO-Throughput: It would be nice if we could iterate just over the blocks in the try, via
         // something like:
         //   for (BasicBlock* bb = ehblk->ebdTryBeg; bb != ehblk->ebdTryLast->bbNext; bb = bb->bbNext)
         //     (plus adding in any filter blocks outside the try whose exceptions are handled here).
         // That doesn't work, however: funclets have caused us to sometimes split the body of a try into
         // more than one sequence of contiguous blocks.  We need to find a better way to do this.
-        for (BasicBlock *bb = fgFirstBB; bb != nullptr; prevBB = bb, bb = bb->bbNext)
+        for (BasicBlock* bb = fgFirstBB; bb != nullptr; bb = bb->bbNext)
         {
-            if (bbInExnFlowRegions(tryIndex, bb) && (prevBB == nullptr || !prevBB->isBBCallAlwaysPair()))
+            if (bbInExnFlowRegions(tryIndex, bb) && !bb->isBBCallAlwaysPairTail())
             {
                 res = new (this, CMK_FlowList) flowList(bb, res);
 
@@ -1379,7 +1376,7 @@ BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind)
 }
 
 //------------------------------------------------------------------------
-// isBBCallAlwaysPair: Determine if this is hte first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pari
+// isBBCallAlwaysPair: Determine if this is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
 
 // Return Value:
 //    True iff "this" is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair

src/coreclr/src/jit/flowgraph.cpp

@@ -10668,11 +10668,11 @@ void Compiler::fgUnreachableBlock(BasicBlock* block)
     }
 #endif // DEBUG
 
-    noway_assert(block->bbPrev != nullptr); // Can use this function to remove the first block
+    noway_assert(block->bbPrev != nullptr); // Can't use this function to remove the first block
 
 #if defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_)
-    assert(!block->bbPrev->isBBCallAlwaysPair()); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
-#endif                                            // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_)
+    assert(!block->isBBCallAlwaysPairTail()); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
+#endif                                        // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_)
 
     /* First walk the statement trees in this basic block and delete each stmt */
 
@@ -11060,8 +11060,8 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
     {
         noway_assert(block->isEmpty());
 
-        /* The block cannot follow a non-retless BBJ_CALLFINALLY (because we don't know who may jump to it) */
-        noway_assert((bPrev == nullptr) || !bPrev->isBBCallAlwaysPair());
+        // The block cannot follow a non-retless BBJ_CALLFINALLY (because we don't know who may jump to it).
+        noway_assert(!block->isBBCallAlwaysPairTail());
 
         /* This cannot be the last basic block */
         noway_assert(block != fgLastBB);
@@ -11303,7 +11303,7 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
                 if ((bPrev->bbJumpDest == bPrev->bbNext) &&
                     !fgInDifferentRegions(bPrev, bPrev->bbJumpDest)) // We don't remove a branch from Hot -> Cold
                 {
-                    if ((bPrev == fgFirstBB) || !bPrev->bbPrev->isBBCallAlwaysPair())
+                    if ((bPrev == fgFirstBB) || !bPrev->isBBCallAlwaysPairTail())
                     {
                         // It's safe to change the jump type
                         bPrev->bbJumpKind = BBJ_NONE;
@@ -11672,6 +11672,8 @@ bool Compiler::fgExpandRarelyRunBlocks()
 
                 // Check for a BBJ_CALLFINALLY followed by a rarely run paired BBJ_ALWAYS
                 //
+                // TODO-Cleanup: How can this be hit? If bbPrev starts a CallAlwaysPair, then this
+                // block must be BBJ_ALWAYS, not BBJ_CALLFINALLY.
                 if (bPrev->isBBCallAlwaysPair())
                 {
                     /* Is the next block rarely run? */
@@ -13908,7 +13910,7 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block)
             // A GOTO cannot be to the next block since that
             // should have been fixed by the  optimization above
             // An exception is made for a jump from Hot to Cold
-            noway_assert(block->bbJumpDest != block->bbNext || ((bPrev != nullptr) && bPrev->isBBCallAlwaysPair()) ||
+            noway_assert(block->bbJumpDest != block->bbNext || block->isBBCallAlwaysPairTail() ||
                          fgInDifferentRegions(block, block->bbNext));
 
             /* Cannot remove the first BB */
@@ -14586,7 +14588,7 @@ bool Compiler::fgOptimizeBranchToNext(BasicBlock* block, BasicBlock* bNext, Basi
             if (!(block->bbFlags & BBF_KEEP_BBJ_ALWAYS))
             {
                 // We can't remove if the BBJ_ALWAYS is part of a BBJ_CALLFINALLY pair
-                if ((bPrev == nullptr) || !bPrev->isBBCallAlwaysPair())
+                if (!block->isBBCallAlwaysPairTail())
                 {
                     /* the unconditional jump is to the next BB  */
                     block->bbJumpKind = BBJ_NONE;
@@ -16954,7 +16956,7 @@ void Compiler::fgDebugCheckUpdate()
 #if defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_)
             // With funclets, we never get rid of the BBJ_ALWAYS part of a BBJ_CALLFINALLY/BBJ_ALWAYS pair,
             // even if we can prove that the finally block never returns.
-            && (prev == NULL || block->bbJumpKind != BBJ_ALWAYS || !prev->isBBCallAlwaysPair())
+            && !block->isBBCallAlwaysPairTail()
 #endif // FEATURE_EH_FUNCLETS
                 )
         {
@@ -17001,7 +17003,7 @@ void Compiler::fgDebugCheckUpdate()
             }
         }
 
-        bool prevIsCallAlwaysPair = ((prev != nullptr) && prev->isBBCallAlwaysPair());
+        bool prevIsCallAlwaysPair = block->isBBCallAlwaysPairTail();
 
         // Check for an unnecessary jumps to the next block
         bool doAssertOnJumpToNextBlock = false; // unless we have a BBJ_COND or BBJ_ALWAYS we can not assert

src/coreclr/src/jit/liveness.cpp

@@ -2399,49 +2399,35 @@ void Compiler::fgInterBlockLocalVarLiveness()
         return;
     }
 
-    /*-------------------------------------------------------------------------
-     * Variables involved in exception-handlers and finally blocks need
-     * to be specially marked
-     */
+    //-------------------------------------------------------------------------
+    // Variables involved in exception-handlers and finally blocks need
+    // to be specially marked
+    //
     BasicBlock* block;
 
     VARSET_TP exceptVars(VarSetOps::MakeEmpty(this));  // vars live on entry to a handler
     VARSET_TP finallyVars(VarSetOps::MakeEmpty(this)); // vars live on exit of a 'finally' block
-    VARSET_TP filterVars(VarSetOps::MakeEmpty(this));  // vars live on exit from a 'filter'
 
     for (block = fgFirstBB; block; block = block->bbNext)
     {
-        if (block->bbCatchTyp != BBCT_NONE)
+        if (block->hasEHBoundaryIn())
         {
-            /* Note the set of variables live on entry to exception handler */
+            // Note the set of variables live on entry to exception handler.
             VarSetOps::UnionD(this, exceptVars, block->bbLiveIn);
         }
 
-        if (block->bbJumpKind == BBJ_EHFILTERRET)
-        {
-            /* Get the set of live variables on exit from a 'filter' */
-            VarSetOps::UnionD(this, filterVars, block->bbLiveOut);
-        }
-        else if (block->bbJumpKind == BBJ_EHFINALLYRET)
-        {
-            /* Get the set of live variables on exit from a 'finally' block */
-
-            VarSetOps::UnionD(this, finallyVars, block->bbLiveOut);
-        }
-#if defined(FEATURE_EH_FUNCLETS)
-        // Funclets are called and returned from, as such we can only count on the frame
-        // pointer being restored, and thus everything live in or live out must be on the
-        // stack
-        if (block->bbFlags & BBF_FUNCLET_BEG)
-        {
-            VarSetOps::UnionD(this, exceptVars, block->bbLiveIn);
-        }
-        if ((block->bbJumpKind == BBJ_EHFINALLYRET) || (block->bbJumpKind == BBJ_EHFILTERRET) ||
-            (block->bbJumpKind == BBJ_EHCATCHRET))
+        if (block->hasEHBoundaryOut())
         {
+            // Get the set of live variables on exit from an exception region.
             VarSetOps::UnionD(this, exceptVars, block->bbLiveOut);
+            if (block->bbJumpKind == BBJ_EHFINALLYRET)
+            {
+                // Live on exit from finally.
+                // We track these separately because, in addition to having EH live-out semantics,
+                // they are must-init.
+                VarSetOps::UnionD(this, finallyVars, block->bbLiveOut);
+            }
         }
-#endif // FEATURE_EH_FUNCLETS
     }
 
     LclVarDsc* varDsc;
@@ -2475,8 +2461,7 @@ void Compiler::fgInterBlockLocalVarLiveness()
         // or on exit from a filter handler or finally.
 
         bool isFinallyVar = VarSetOps::IsMember(this, finallyVars, varDsc->lvVarIndex);
-        if (isFinallyVar || VarSetOps::IsMember(this, exceptVars, varDsc->lvVarIndex) ||
-            VarSetOps::IsMember(this, filterVars, varDsc->lvVarIndex))
+        if (isFinallyVar || VarSetOps::IsMember(this, exceptVars, varDsc->lvVarIndex))
         {
             // Mark the variable appropriately.
             lvaSetVarLiveInOutOfHandler(varNum);

src/coreclr/src/jit/ssabuilder.cpp

@@ -937,7 +937,7 @@ void SsaBuilder::AddMemoryDefToHandlerPhis(MemoryKind memoryKind, BasicBlock* bl
     if (m_pCompiler->ehBlockHasExnFlowDsc(block))
     {
         // Don't do anything for a compiler-inserted BBJ_ALWAYS that is a "leave helper".
-        if (block->bbJumpKind == BBJ_ALWAYS && (block->bbFlags & BBF_INTERNAL) && (block->bbPrev->isBBCallAlwaysPair()))
+        if ((block->bbFlags & BBF_INTERNAL) && block->isBBCallAlwaysPairTail())
         {
             return;
         }