JIT: Switch basic block successors to a visitor pattern (#86543)
authorJakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Mon, 22 May 2023 22:00:37 +0000 (00:00 +0200)
committerGitHub <noreply@github.com>
Mon, 22 May 2023 22:00:37 +0000 (00:00 +0200)
BasicBlock::GetAllSuccs seems to be very slow.

src/coreclr/jit/block.cpp
src/coreclr/jit/block.h
src/coreclr/jit/compiler.hpp
src/coreclr/jit/dataflow.h
src/coreclr/jit/fgbasic.cpp
src/coreclr/jit/liveness.cpp
src/coreclr/jit/lsrabuild.cpp
src/coreclr/jit/promotionliveness.cpp
src/coreclr/jit/redundantbranchopts.cpp
src/coreclr/jit/ssabuilder.cpp
src/coreclr/jit/valuenum.cpp

index 10486bc..07bc95c 100644 (file)
@@ -69,7 +69,10 @@ unsigned SsaStressHashHelper()
 #endif
 
 EHSuccessorIterPosition::EHSuccessorIterPosition(Compiler* comp, BasicBlock* block)
-    : m_remainingRegSuccs(block->NumSucc(comp)), m_curRegSucc(nullptr), m_curTry(comp->ehGetBlockExnFlowDsc(block))
+    : m_remainingRegSuccs(block->NumSucc(comp))
+    , m_numRegSuccs(m_remainingRegSuccs)
+    , m_curRegSucc(nullptr)
+    , m_curTry(comp->ehGetBlockExnFlowDsc(block))
 {
     // If "block" is a "leave helper" block (the empty BBJ_ALWAYS block that pairs with a
     // preceding BBJ_CALLFINALLY block to implement a "leave" IL instruction), then no exceptions
@@ -96,8 +99,8 @@ void EHSuccessorIterPosition::FindNextRegSuccTry(Compiler* comp, BasicBlock* blo
     // Must now consider the next regular successor, if any.
     while (m_remainingRegSuccs > 0)
     {
+        m_curRegSucc = block->GetSucc(m_numRegSuccs - m_remainingRegSuccs, comp);
         m_remainingRegSuccs--;
-        m_curRegSucc = block->GetSucc(m_remainingRegSuccs, comp);
         if (comp->bbIsTryBeg(m_curRegSucc))
         {
             assert(m_curRegSucc->hasTryIndex()); // Since it is a try begin.
index 7beab20..7dd19c6 100644 (file)
@@ -222,6 +222,7 @@ class EHSuccessorIterPosition
     // successor of BB1.  This captures the iteration over the successors of BB1
     // for this purpose.  (In reverse order; we're done when this field is 0).
     unsigned m_remainingRegSuccs;
+    unsigned m_numRegSuccs;
 
     // The current "regular" successor of "m_block" that we're considering.
     BasicBlock* m_curRegSucc;
@@ -245,7 +246,7 @@ public:
     EHSuccessorIterPosition(Compiler* comp, BasicBlock* block);
 
     // Constructs a position that "points" past the last EH successor of `block` ("end" position).
-    EHSuccessorIterPosition() : m_remainingRegSuccs(0), m_curTry(nullptr)
+    EHSuccessorIterPosition() : m_remainingRegSuccs(0), m_numRegSuccs(0), m_curTry(nullptr)
     {
     }
 
@@ -614,6 +615,12 @@ inline BasicBlockFlags& operator &=(BasicBlockFlags& a, BasicBlockFlags b)
     return a = (BasicBlockFlags)((unsigned __int64)a & (unsigned __int64)b);
 }
 
+enum class BasicBlockVisit
+{
+    Continue,
+    Abort,
+};
+
 // clang-format on
 
 //------------------------------------------------------------------------
@@ -1350,6 +1357,9 @@ struct BasicBlock : private LIR::Range
         return Successors<AllSuccessorIterPosition>(comp, this);
     }
 
+    template <typename TFunc>
+    BasicBlockVisit VisitAllSuccs(Compiler* comp, TFunc func);
+
     // BBSuccList: adapter class for forward iteration of block successors, using range-based `for`,
     // normally used via BasicBlock::Succs(), e.g.:
     //    for (BasicBlock* const target : block->Succs()) ...
index f8f1715..e36d7cb 100644 (file)
@@ -306,6 +306,314 @@ inline EHblkDsc* Compiler::ehGetBlockHndDsc(BasicBlock* block)
     return ehGetDsc(block->getHndIndex());
 }
 
+#define RETURN_ON_ABORT(expr)                                                                                          \
+    if (expr == BasicBlockVisit::Abort)                                                                                \
+    {                                                                                                                  \
+        return BasicBlockVisit::Abort;                                                                                 \
+    }
+
+//------------------------------------------------------------------------------
+// VisitEHSuccessors: Given a block inside a handler region, visit all handlers
+// that control may flow to as part of EH.
+//
+// Arguments:
+//   comp  - Compiler instance
+//   block - The block
+//   func  - Callback
+//
+// Returns:
+//   Whether or not the visiting should proceed.
+//
+// Remarks:
+//   This encapsulates the "exception handling" successors of a block. That is,
+//   if a basic block BB1 occurs in a try block, we consider the first basic
+//   block BB2 of the corresponding handler to be an "EH successor" of BB1.
+//
+//   TODO-BUG: This function currently does not take into account that filters
+//   are invoked in the first pass of EH, which means that enclosed finally
+//   blocks may be successors of filter blocks (as part of the second pass of
+//   EH). See fgGetHandlerLiveVars for code that does take this into account.
+//
+template <typename TFunc>
+static BasicBlockVisit VisitEHSuccessors(Compiler* comp, BasicBlock* block, TFunc func)
+{
+    EHblkDsc* eh = comp->ehGetBlockExnFlowDsc(block);
+    if (eh == nullptr)
+    {
+        return BasicBlockVisit::Continue;
+    }
+
+    while (true)
+    {
+        // If the original block whose EH successors we're iterating over
+        // is a BBJ_CALLFINALLY, that finally clause's first block
+        // will be yielded as a normal successor.  Don't also yield as
+        // an exceptional successor.
+        BasicBlock* flowBlock = eh->ExFlowBlock();
+        if (!block->KindIs(BBJ_CALLFINALLY) || (block->bbJumpDest != flowBlock))
+        {
+            RETURN_ON_ABORT(func(flowBlock));
+        }
+
+        if (eh->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
+        {
+            break;
+        }
+
+        eh = comp->ehGetDsc(eh->ebdEnclosingTryIndex);
+    }
+
+    return BasicBlockVisit::Continue;
+}
+
+//------------------------------------------------------------------------------
+// VisitSuccessorEHSuccessors: Given a block and one of its regular successors,
+// if that regular successor is the beginning of a try, then also visit its
+// handlers.
+//
+// Arguments:
+//   comp  - Compiler instance
+//   block - The block
+//   succ  - A regular successor of block
+//   func  - Callback
+//
+// Returns:
+//   Whether or not the visiting should proceed.
+//
+// Remarks:
+//   Because we make the conservative assumption that control flow can jump
+//   from a try block to its handler at any time, the immediate (regular
+//   control flow) predecessor(s) of the first block of a try block are also
+//   considered to have the first block of the handler as an EH successor.
+//
+//   As an example: for liveness this makes variables that are "live-in" to the
+//   handler become "live-out" for these try-predecessor block, so that they
+//   become live-in to the try -- which we require.
+//
+//   TODO-Cleanup: Is the above comment true today or is this code unnecessary?
+//   For a block T with an EH successor E liveness takes care to consider the
+//   live-in set E as "volatile" variables that are fully live at all points
+//   within the block T, including being a part of T's live-in set. That means
+//   that if T is the beginning of a try, then any predecessor of T will
+//   naturally also have E's live-in set as part of its live-out set.
+//
+template <typename TFunc>
+static BasicBlockVisit VisitSuccessorEHSuccessors(Compiler* comp, BasicBlock* block, BasicBlock* succ, TFunc func)
+{
+    if (!comp->bbIsTryBeg(succ))
+    {
+        return BasicBlockVisit::Continue;
+    }
+
+    unsigned tryIndex = succ->getTryIndex();
+    if (comp->bbInExnFlowRegions(tryIndex, block))
+    {
+        // Already yielded as an EH successor of block itself
+        return BasicBlockVisit::Continue;
+    }
+
+    EHblkDsc* eh = comp->ehGetDsc(tryIndex);
+
+    do
+    {
+        RETURN_ON_ABORT(func(eh->ExFlowBlock()));
+
+        if (eh->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
+        {
+            break;
+        }
+
+        eh = comp->ehGetDsc(eh->ebdEnclosingTryIndex);
+    } while (eh->ebdTryBeg == succ);
+
+    return BasicBlockVisit::Continue;
+}
+
+//------------------------------------------------------------------------------
+// VisitAllSuccsInternal: Internal helper function to visit all successors
+// (including EH successors) of a block.
+//
+// Arguments:
+//   comp  - Compiler instance
+//   block - The block
+//   func  - Callback
+//
+// Returns:
+//   Whether or not the visiting was aborted.
+//
+template <typename TFunc>
+static BasicBlockVisit VisitAllSuccsInternal(Compiler* comp, BasicBlock* block, TFunc func)
+{
+    switch (block->bbJumpKind)
+    {
+        case BBJ_EHFILTERRET:
+            RETURN_ON_ABORT(func(block->bbJumpDest));
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+            RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, block->bbJumpDest, func));
+            break;
+
+        case BBJ_EHFINALLYRET:
+        {
+            EHblkDsc* ehDsc = comp->ehGetDsc(block->getHndIndex());
+            assert(ehDsc->HasFinallyHandler());
+
+            BasicBlock* begBlk;
+            BasicBlock* endBlk;
+            comp->ehGetCallFinallyBlockRange(block->getHndIndex(), &begBlk, &endBlk);
+
+            BasicBlock* finBeg = ehDsc->ebdHndBeg;
+
+            for (BasicBlock* bcall = begBlk; bcall != endBlk; bcall = bcall->bbNext)
+            {
+                if ((bcall->bbJumpKind != BBJ_CALLFINALLY) || (bcall->bbJumpDest != finBeg))
+                {
+                    continue;
+                }
+
+                assert(bcall->isBBCallAlwaysPair());
+
+                RETURN_ON_ABORT(func(bcall->bbNext));
+            }
+
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+
+            for (BasicBlock* bcall = begBlk; bcall != endBlk; bcall = bcall->bbNext)
+            {
+                if ((bcall->bbJumpKind != BBJ_CALLFINALLY) || (bcall->bbJumpDest != finBeg))
+                {
+                    continue;
+                }
+
+                assert(bcall->isBBCallAlwaysPair());
+                RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, bcall->bbNext, func));
+            }
+
+            break;
+        }
+
+        case BBJ_CALLFINALLY:
+        case BBJ_EHCATCHRET:
+        case BBJ_LEAVE:
+            RETURN_ON_ABORT(func(block->bbJumpDest));
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+            RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, block->bbJumpDest, func));
+            break;
+
+        case BBJ_ALWAYS:
+            RETURN_ON_ABORT(func(block->bbJumpDest));
+
+            // If "block" is a "leave helper" block (the empty BBJ_ALWAYS block
+            // that pairs with a preceding BBJ_CALLFINALLY block to implement a
+            // "leave" IL instruction), then no exceptions can occur within it
+            // and we skip its normal EH successors.
+            if (!block->isBBCallAlwaysPairTail())
+            {
+                RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+            }
+            RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, block->bbJumpDest, func));
+            break;
+
+        case BBJ_NONE:
+            RETURN_ON_ABORT(func(block->bbNext));
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+            RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, block->bbNext, func));
+            break;
+
+        case BBJ_COND:
+            RETURN_ON_ABORT(func(block->bbNext));
+
+            if (block->bbJumpDest != block->bbNext)
+            {
+                RETURN_ON_ABORT(func(block->bbJumpDest));
+            }
+
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+            RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, block->bbNext, func));
+
+            if (block->bbJumpDest != block->bbNext)
+            {
+                RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, block->bbJumpDest, func));
+            }
+            break;
+
+        case BBJ_SWITCH:
+        {
+            Compiler::SwitchUniqueSuccSet sd = comp->GetDescriptorForSwitch(block);
+            for (unsigned i = 0; i < sd.numDistinctSuccs; i++)
+            {
+                RETURN_ON_ABORT(func(sd.nonDuplicates[i]));
+            }
+
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+
+            for (unsigned i = 0; i < sd.numDistinctSuccs; i++)
+            {
+                RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, block, sd.nonDuplicates[i], func));
+            }
+
+            break;
+        }
+
+        case BBJ_THROW:
+        case BBJ_RETURN:
+        case BBJ_EHFAULTRET:
+            RETURN_ON_ABORT(VisitEHSuccessors(comp, block, func));
+            break;
+
+        default:
+            unreached();
+    }
+
+    return BasicBlockVisit::Continue;
+}
+
+//------------------------------------------------------------------------------
+// VisitAllSuccs: Visit all successors (including EH successors) of this block.
+//
+// Arguments:
+//   comp  - Compiler instance
+//   func  - Callback
+//
+// Returns:
+//   Whether or not the visiting was aborted.
+//
+template <typename TFunc>
+BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func)
+{
+#ifdef DEBUG
+    // Compare callback-based successors with old iterator based successors.
+    BasicBlock* succs[64];
+    unsigned    index = 0;
+
+    VisitAllSuccsInternal(comp, this, [&succs, &index](BasicBlock* succ) {
+        if (index < ArrLen(succs))
+        {
+            succs[index] = succ;
+        }
+
+        index++;
+        return BasicBlockVisit::Continue;
+    });
+
+    unsigned index2 = 0;
+    for (BasicBlock* succ : GetAllSuccs(comp))
+    {
+        if (index2 < ArrLen(succs))
+        {
+            assert(succs[index2] == succ);
+        }
+
+        index2++;
+    }
+
+    assert(index == index2);
+#endif
+
+    return VisitAllSuccsInternal(comp, this, func);
+}
+
+#undef RETURN_ON_ABORT
+
 #if defined(FEATURE_EH_FUNCLETS)
 
 /*****************************************************************************
index 8460ffb..7601b33 100644 (file)
@@ -67,10 +67,10 @@ void DataFlow::ForwardAnalysis(TCallback& callback)
 
         if (callback.EndMerge(block))
         {
-            for (BasicBlock* succ : block->GetAllSuccs(m_pCompiler))
-            {
+            block->VisitAllSuccs(m_pCompiler, [&worklist](BasicBlock* succ) {
                 worklist.insert(worklist.end(), succ);
-            }
+                return BasicBlockVisit::Continue;
+            });
         }
     }
 }
index 61f2c84..c4b274b 100644 (file)
@@ -6042,12 +6042,18 @@ bool Compiler::fgMightHaveLoop()
     {
         BitVecOps::AddElemD(&blockVecTraits, blocksSeen, block->bbNum);
 
-        for (BasicBlock* const succ : block->GetAllSuccs(this))
-        {
+        BasicBlockVisit result = block->VisitAllSuccs(this, [&](BasicBlock* succ) {
             if (BitVecOps::IsMember(&blockVecTraits, blocksSeen, succ->bbNum))
             {
-                return true;
+                return BasicBlockVisit::Abort;
             }
+
+            return BasicBlockVisit::Continue;
+        });
+
+        if (result == BasicBlockVisit::Abort)
+        {
+            return true;
         }
     }
     return false;
index fb46b08..ec3b6f9 100644 (file)
@@ -1263,15 +1263,16 @@ class LiveVarAnalysis
         }
 
         // Additionally, union in all the live-in tracked vars of successors.
-        for (BasicBlock* succ : block->GetAllSuccs(m_compiler))
-        {
+        block->VisitAllSuccs(m_compiler, [=](BasicBlock* succ) {
             VarSetOps::UnionD(m_compiler, m_liveOut, succ->bbLiveIn);
             m_memoryLiveOut |= succ->bbMemoryLiveIn;
             if (succ->bbNum <= block->bbNum)
             {
                 m_hasPossibleBackEdge = true;
             }
-        }
+
+            return BasicBlockVisit::Continue;
+        });
 
         /* For lvaKeepAliveAndReportThis methods, "this" has to be kept alive everywhere
            Note that a function may end in a throw on an infinite loop (as opposed to a return).
index dee213d..c03c53c 100644 (file)
@@ -2562,19 +2562,20 @@ void           LinearScan::buildIntervals()
             {
                 VarSetOps::DiffD(compiler, expUseSet, nextBlock->bbLiveIn);
             }
-            for (BasicBlock* succ : block->GetAllSuccs(compiler))
-            {
+
+            block->VisitAllSuccs(compiler, [=, &expUseSet](BasicBlock* succ) {
                 if (VarSetOps::IsEmpty(compiler, expUseSet))
                 {
-                    break;
+                    return BasicBlockVisit::Abort;
                 }
 
-                if (isBlockVisited(succ))
+                if (!isBlockVisited(succ))
                 {
-                    continue;
+                    VarSetOps::DiffD(compiler, expUseSet, succ->bbLiveIn);
                 }
-                VarSetOps::DiffD(compiler, expUseSet, succ->bbLiveIn);
-            }
+
+                return BasicBlockVisit::Continue;
+            });
 
             if (!VarSetOps::IsEmpty(compiler, expUseSet))
             {
index c936478..a345637 100644 (file)
@@ -352,11 +352,11 @@ bool PromotionLiveness::PerBlockLiveness(BasicBlock* block)
 
     BasicBlockLiveness& bbInfo = m_bbInfo[block->bbNum];
     BitVecOps::ClearD(m_bvTraits, bbInfo.LiveOut);
-    for (BasicBlock* succ : block->GetAllSuccs(m_compiler))
-    {
+    block->VisitAllSuccs(m_compiler, [=, &bbInfo](BasicBlock* succ) {
         BitVecOps::UnionD(m_bvTraits, bbInfo.LiveOut, m_bbInfo[succ->bbNum].LiveIn);
         m_hasPossibleBackEdge |= succ->bbNum <= block->bbNum;
-    }
+        return BasicBlockVisit::Continue;
+    });
 
     BitVecOps::LivenessD(m_bvTraits, m_liveIn, bbInfo.VarDef, bbInfo.VarUse, bbInfo.LiveOut);
 
index de0dd3d..d9c6c33 100644 (file)
@@ -2102,21 +2102,26 @@ bool Compiler::optReachable(BasicBlock* const fromBlock, BasicBlock* const toBlo
             continue;
         }
 
-        for (BasicBlock* succ : nextBlock->GetAllSuccs(this))
-        {
+        BasicBlockVisit result = nextBlock->VisitAllSuccs(this, [this, toBlock, &stack](BasicBlock* succ) {
             if (succ == toBlock)
             {
-                return true;
+                return BasicBlockVisit::Abort;
             }
 
             if (BitVecOps::IsMember(optReachableBitVecTraits, optReachableBitVec, succ->bbNum))
             {
-                continue;
+                return BasicBlockVisit::Continue;
             }
 
             BitVecOps::AddElemD(optReachableBitVecTraits, optReachableBitVec, succ->bbNum);
 
             stack.Push(succ);
+            return BasicBlockVisit::Continue;
+        });
+
+        if (result == BasicBlockVisit::Abort)
+        {
+            return true;
         }
     }
 
index 32dee88..b78786a 100644 (file)
@@ -1172,8 +1172,7 @@ void SsaBuilder::BlockRenameVariables(BasicBlock* block)
 //
 void SsaBuilder::AddPhiArgsToSuccessors(BasicBlock* block)
 {
-    for (BasicBlock* succ : block->GetAllSuccs(m_pCompiler))
-    {
+    block->VisitAllSuccs(m_pCompiler, [this, block](BasicBlock* succ) {
         // Walk the statements for phi nodes.
         for (Statement* const stmt : succ->Statements())
         {
@@ -1359,7 +1358,9 @@ void SsaBuilder::AddPhiArgsToSuccessors(BasicBlock* block)
                 tryInd = succTry->ebdEnclosingTryIndex;
             }
         }
-    }
+
+        return BasicBlockVisit::Continue;
+    });
 }
 
 //------------------------------------------------------------------------
index c9123c9..6b28b47 100644 (file)
@@ -9238,15 +9238,14 @@ struct ValueNumberState
 
         SetVisitBit(blk->bbNum, BVB_complete);
 
-        for (BasicBlock* succ : blk->GetAllSuccs(m_comp))
-        {
+        blk->VisitAllSuccs(m_comp, [&](BasicBlock* succ) {
 #ifdef DEBUG_VN_VISIT
             JITDUMP("   Succ(" FMT_BB ").\n", succ->bbNum);
 #endif // DEBUG_VN_VISIT
 
             if (GetVisitBit(succ->bbNum, BVB_complete))
             {
-                continue;
+                return BasicBlockVisit::Continue;
             }
 #ifdef DEBUG_VN_VISIT
             JITDUMP("     Not yet completed.\n");
@@ -9269,8 +9268,8 @@ struct ValueNumberState
                 JITDUMP("     All preds complete, adding to allDone.\n");
 #endif // DEBUG_VN_VISIT
 
-                assert(!GetVisitBit(succ->bbNum, BVB_onAllDone)); // Only last completion of last succ should add to
-                                                                  // this.
+                // Only last completion of last succ should add to this.
+                assert(!GetVisitBit(succ->bbNum, BVB_onAllDone));
                 m_toDoAllPredsDone.Push(succ);
                 SetVisitBit(succ->bbNum, BVB_onAllDone);
             }
@@ -9289,7 +9288,9 @@ struct ValueNumberState
                     SetVisitBit(succ->bbNum, BVB_onNotAllDone);
                 }
             }
-        }
+
+            return BasicBlockVisit::Continue;
+        });
     }
 
     bool ToDoExists()