JIT: Replace BlockSet with BitVec, and remove BB epoch (dotnet#110034)

Part of dotnet#107749. Prerequisite for dotnet#110026.

Use postorder number-based BitVecs in RBO and block layout.
Use bbID-based BitVecs in fgIncorporateProfileData. This runs early enough in the JIT frontend such that I would expect bbIDs and bbNums to be 1:1, so I don't expect any TP impact from this change.
Switch descriptor creation still uses bbNums as a key into a BitVec as a workaround for BB epoch invariants -- I'll try switching this over to bbID in a follow-up to evaluate the TP cost of a sparser bitset.

Author: amanasifkhalid

src/coreclr/jit/CMakeLists.txt

@@ -299,7 +299,6 @@ set( JIT_HEADERS
   bitsetops.h
   bitvec.h
   block.h
-  blockset.h
   codegen.h
   codegeninterface.h
   compiler.h

src/coreclr/jit/block.cpp

@@ -617,8 +617,7 @@ void BasicBlock::dspSuccs(Compiler* compiler)
     // compute it ourselves here.
     if (bbKind == BBJ_SWITCH)
     {
-        // Create a set with all the successors. Don't use BlockSet, so we don't need to worry
-        // about the BlockSet epoch.
+        // Create a set with all the successors.
         unsigned     bbNumMax = compiler->fgBBNumMax;
         BitVecTraits bitVecTraits(bbNumMax + 1, compiler);
         BitVec       uniqueSuccBlocks(BitVecOps::MakeEmpty(&bitVecTraits));
@@ -1036,10 +1035,10 @@ unsigned JitPtrKeyFuncs<BasicBlock>::GetHashCode(const BasicBlock* ptr)
     unsigned hash = SsaStressHashHelper();
     if (hash != 0)
     {
-        return (hash ^ (ptr->bbNum << 16) ^ ptr->bbNum);
+        return (hash ^ (ptr->bbID << 16) ^ ptr->bbID);
     }
 #endif
-    return ptr->bbNum;
+    return ptr->bbID;
 }
 
 //------------------------------------------------------------------------

src/coreclr/jit/block.h

@@ -23,7 +23,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 // Defines VARSET_TP
 #include "varset.h"
 
-#include "blockset.h"
 #include "jitstd.h"
 #include "bitvec.h"
 #include "jithashtable.h"

src/coreclr/jit/blockset.h

@@ -36,7 +36,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 #include "regalloc.h"
 #include "sm.h"
 #include "cycletimer.h"
-#include "blockset.h"
 #include "arraystack.h"
 #include "priorityqueue.h"
 #include "hashbv.h"
@@ -5225,65 +5224,6 @@ class Compiler
         return getAllocator(cmk).allocate<T>(fgBBNumMax + 1);
     }
 
-    // BlockSets are relative to a specific set of BasicBlock numbers. If that changes
-    // (if the blocks are renumbered), this changes. BlockSets from different epochs
-    // cannot be meaningfully combined. Note that new blocks can be created with higher
-    // block numbers without changing the basic block epoch. These blocks *cannot*
-    // participate in a block set until the blocks are all renumbered, causing the epoch
-    // to change. This is useful if continuing to use previous block sets is valuable.
-    // If the epoch is zero, then it is uninitialized, and block sets can't be used.
-    unsigned fgCurBBEpoch = 0;
-
-    unsigned GetCurBasicBlockEpoch()
-    {
-        return fgCurBBEpoch;
-    }
-
-    // The number of basic blocks in the current epoch. When the blocks are renumbered,
-    // this is fgBBcount. As blocks are added, fgBBcount increases, fgCurBBEpochSize remains
-    // the same, until a new BasicBlock epoch is created, such as when the blocks are all renumbered.
-    unsigned fgCurBBEpochSize = 0;
-
-    // The number of "size_t" elements required to hold a bitset large enough for fgCurBBEpochSize
-    // bits. This is precomputed to avoid doing math every time BasicBlockBitSetTraits::GetArrSize() is called.
-    unsigned fgBBSetCountInSizeTUnits = 0;
-
-    void NewBasicBlockEpoch()
-    {
-        INDEBUG(unsigned oldEpochArrSize = fgBBSetCountInSizeTUnits);
-
-        // We have a new epoch. Compute and cache the size needed for new BlockSets.
-        fgCurBBEpoch++;
-        fgCurBBEpochSize = fgBBNumMax + 1;
-        fgBBSetCountInSizeTUnits =
-            roundUp(fgCurBBEpochSize, (unsigned)(sizeof(size_t) * 8)) / unsigned(sizeof(size_t) * 8);
-
-#ifdef DEBUG
-        if (verbose)
-        {
-            unsigned epochArrSize = BasicBlockBitSetTraits::GetArrSize(this);
-            printf("\nNew BlockSet epoch %d, # of blocks (including unused BB00): %u, bitset array size: %u (%s)",
-                   fgCurBBEpoch, fgCurBBEpochSize, epochArrSize, (epochArrSize <= 1) ? "short" : "long");
-            if ((fgCurBBEpoch != 1) && ((oldEpochArrSize <= 1) != (epochArrSize <= 1)))
-            {
-                // If we're not just establishing the first epoch, and the epoch array size has changed such that we're
-                // going to change our bitset representation from short (just a size_t bitset) to long (a pointer to an
-                // array of size_t bitsets), then print that out.
-                printf("; NOTE: BlockSet size was previously %s!", (oldEpochArrSize <= 1) ? "short" : "long");
-            }
-            printf("\n");
-        }
-#endif // DEBUG
-    }
-
-    void EnsureBasicBlockEpoch()
-    {
-        if (fgCurBBEpochSize != fgBBNumMax + 1)
-        {
-            NewBasicBlockEpoch();
-        }
-    }
-
     bool fgEnsureFirstBBisScratch();
     bool fgFirstBBisScratch();
     bool fgBBisScratch(BasicBlock* block);
@@ -6305,7 +6245,7 @@ class Compiler
     };
 
     template <bool hasEH>
-    void fgMoveHotJumps();
+    void fgMoveHotJumps(FlowGraphDfsTree* dfsTree);
 
     bool fgFuncletsAreCold();
 

src/coreclr/jit/compiler.h

@@ -69,30 +69,6 @@ class AllVarBitSetTraits : public CompAllocBitSetTraits
     static inline BitSetSupport::BitSetOpCounter* GetOpCounter(Compiler* comp);
 };
 
-///////////////////////////////////////////////////////////////////////////////
-//
-// BasicBlockBitSetTraits
-//
-// This class is customizes the bit set to represent sets of BasicBlocks.
-// The size of the bitset is determined by maximum assigned BasicBlock number
-// (Compiler::fgBBNumMax) (Note that fgBBcount is not equal to this during inlining,
-// when fgBBcount is the number of blocks in the inlined function, but the assigned
-// block numbers are higher than the inliner function. fgBBNumMax counts both.
-// Thus, if you only care about the inlinee, during inlining, this bit set will waste
-// the lower numbered block bits.) The Compiler* tracks the BasicBlock epochs.
-//
-class BasicBlockBitSetTraits : public CompAllocBitSetTraits
-{
-public:
-    static inline unsigned GetSize(Compiler* comp);
-
-    static inline unsigned GetArrSize(Compiler* comp);
-
-    static inline unsigned GetEpoch(class Compiler* comp);
-
-    static inline BitSetSupport::BitSetOpCounter* GetOpCounter(Compiler* comp);
-};
-
 ///////////////////////////////////////////////////////////////////////////////
 //
 // BitVecTraits

src/coreclr/jit/compilerbitsettraits.h

@@ -96,40 +96,6 @@ BitSetSupport::BitSetOpCounter* AllVarBitSetTraits::GetOpCounter(Compiler* comp)
 #endif
 }
 
-///////////////////////////////////////////////////////////////////////////////
-//
-// BasicBlockBitSetTraits
-//
-///////////////////////////////////////////////////////////////////////////////
-
-// static
-unsigned BasicBlockBitSetTraits::GetSize(Compiler* comp)
-{
-    return comp->fgCurBBEpochSize;
-}
-
-// static
-unsigned BasicBlockBitSetTraits::GetArrSize(Compiler* comp)
-{
-    // Assert that the epoch has been initialized. This is a convenient place to assert this because
-    // GetArrSize() is called for every function, via IsShort().
-    assert(GetEpoch(comp) != 0);
-
-    return comp->fgBBSetCountInSizeTUnits; // This is precomputed to avoid doing math every time this function is called
-}
-
-// static
-unsigned BasicBlockBitSetTraits::GetEpoch(Compiler* comp)
-{
-    return comp->GetCurBasicBlockEpoch();
-}
-
-// static
-BitSetSupport::BitSetOpCounter* BasicBlockBitSetTraits::GetOpCounter(Compiler* comp)
-{
-    return nullptr;
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 //
 // BitVecTraits

src/coreclr/jit/compilerbitsettraits.hpp

@@ -5400,30 +5400,6 @@ bool Compiler::fgRenumberBlocks()
         JITDUMP("=============== No blocks renumbered!\n");
     }
 
-    // Now update the BlockSet epoch, which depends on the block numbers.
-    // If any blocks have been renumbered then create a new BlockSet epoch.
-    // Even if we have not renumbered any blocks, we might still need to force
-    // a new BlockSet epoch, for one of several reasons. If there are any new
-    // blocks with higher numbers than the former maximum numbered block, then we
-    // need a new epoch with a new size matching the new largest numbered block.
-    // Also, if the number of blocks is different from the last time we set the
-    // BlockSet epoch, then we need a new epoch. This wouldn't happen if we
-    // renumbered blocks after every block addition/deletion, but it might be
-    // the case that we can change the number of blocks, then set the BlockSet
-    // epoch without renumbering, then change the number of blocks again, then
-    // renumber.
-    if (renumbered || newMaxBBNum)
-    {
-        NewBasicBlockEpoch();
-
-        // The key in the unique switch successor map is dependent on the block number, so invalidate that cache.
-        InvalidateUniqueSwitchSuccMap();
-    }
-    else
-    {
-        EnsureBasicBlockEpoch();
-    }
-
     // Tell our caller if any blocks actually were renumbered.
     return renumbered || newMaxBBNum;
 }

src/coreclr/jit/fgbasic.cpp

@@ -530,11 +530,7 @@ Compiler::SwitchUniqueSuccSet Compiler::GetDescriptorForSwitch(BasicBlock* switc
     {
         // We must compute the descriptor. Find which are dups, by creating a bit set with the unique successors.
         // We create a temporary bitset of blocks to compute the unique set of successor blocks,
-        // since adding a block's number twice leaves just one "copy" in the bitset. Note that
-        // we specifically don't use the BlockSet type, because doing so would require making a
-        // call to EnsureBasicBlockEpoch() to make sure the epoch is up-to-date. However, that
-        // can create a new epoch, thus invalidating all existing BlockSet objects, such as
-        // reachability information stored in the blocks. To avoid that, we just use a local BitVec.
+        // since adding a block's number twice leaves just one "copy" in the bitset.
 
         BitVecTraits blockVecTraits(fgBBNumMax + 1, this);
         BitVec       uniqueSuccBlocks(BitVecOps::MakeEmpty(&blockVecTraits));

src/coreclr/jit/fgflow.cpp

@@ -4463,8 +4463,11 @@ bool Compiler::fgReorderBlocks(bool useProfile)
 // Template parameters:
 //    hasEH - If true, method has EH regions, so check that we don't try to move blocks in different regions
 //
+// Parameters:
+//    dfsTree - The depth-first traversal of the flowgraph
+//
 template <bool hasEH>
-void Compiler::fgMoveHotJumps()
+void Compiler::fgMoveHotJumps(FlowGraphDfsTree* dfsTree)
 {
 #ifdef DEBUG
     if (verbose)
@@ -4477,17 +4480,22 @@ void Compiler::fgMoveHotJumps()
     }
 #endif // DEBUG
 
-    EnsureBasicBlockEpoch();
-    BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this));
-    BlockSetOps::AddElemD(this, visitedBlocks, fgFirstBB->bbNum);
+    assert(dfsTree != nullptr);
+    BitVecTraits traits(dfsTree->PostOrderTraits());
+    BitVec       visitedBlocks = BitVecOps::MakeEmpty(&traits);
 
     // If we have a funclet region, don't bother reordering anything in it.
     //
     BasicBlock* next;
     for (BasicBlock* block = fgFirstBB; block != fgFirstFuncletBB; block = next)
     {
         next = block->Next();
-        BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum);
+        if (!dfsTree->Contains(block))
+        {
+            continue;
+        }
+
+        BitVecOps::AddElemD(&traits, visitedBlocks, block->bbPostorderNum);
 
         // Don't bother trying to move cold blocks
         //
@@ -4534,7 +4542,8 @@ void Compiler::fgMoveHotJumps()
         }
 
         BasicBlock* target         = targetEdge->getDestinationBlock();
-        bool        isBackwardJump = BlockSetOps::IsMember(this, visitedBlocks, target->bbNum);
+        bool        isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum);
+        assert(dfsTree->Contains(target));
 
         if (isBackwardJump)
         {
@@ -4553,7 +4562,8 @@ void Compiler::fgMoveHotJumps()
                 //
                 targetEdge     = unlikelyEdge;
                 target         = targetEdge->getDestinationBlock();
-                isBackwardJump = BlockSetOps::IsMember(this, visitedBlocks, target->bbNum);
+                isBackwardJump = BitVecOps::IsMember(&traits, visitedBlocks, target->bbPostorderNum);
+                assert(dfsTree->Contains(target));
 
                 if (isBackwardJump)
                 {
@@ -4696,7 +4706,7 @@ void Compiler::fgDoReversePostOrderLayout()
             }
         }
 
-        fgMoveHotJumps</* hasEH */ false>();
+        fgMoveHotJumps</* hasEH */ false>(dfsTree);
 
         return;
     }
@@ -4769,7 +4779,7 @@ void Compiler::fgDoReversePostOrderLayout()
         fgInsertBBafter(pair.callFinally, pair.callFinallyRet);
     }
 
-    fgMoveHotJumps</* hasEH */ true>();
+    fgMoveHotJumps</* hasEH */ true>(dfsTree);
 }
 
 //-----------------------------------------------------------------------------