JIT: Allow initializing blocks without jump targets (#93928)

...and remove BasicBlock::bbTempJumpDest, per discussion in #93415. We still assert the jump target is set appropriately whenever it is read/written, and in the majority of cases, we still initialize blocks with their jump kind and target set simultaneously. This change improves usability for the few edge cases (like in Compiler::impImportLeave) where a block's jump target isn't known at initialization.
dotnet · Oct 24, 2023 · fe043b4 · fe043b4
1 parent 371d715
commit fe043b4
Show file tree

Hide file tree

Showing 13 changed files with 100 additions and 98 deletions.
diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp
@@ -28,10 +28,6 @@ size_t BasicBlock::s_Count;
 // The max # of tree nodes in any BB
 /* static */
 unsigned BasicBlock::s_nMaxTrees;
-
-// Temporary target to initialize blocks with jumps
-/* static */
-BasicBlock BasicBlock::bbTempJumpDest;
 #endif // DEBUG
 
 #ifdef DEBUG
@@ -1449,14 +1445,14 @@ bool BasicBlock::endsWithTailCallConvertibleToLoop(Compiler* comp, GenTree** tai
  *  Allocate a basic block but don't append it to the current BB list.
  */
 
-BasicBlock* Compiler::bbNewBasicBlock()
+BasicBlock* BasicBlock::bbNewBasicBlock(Compiler* compiler)
 {
     BasicBlock* block;
 
     /* Allocate the block descriptor and zero it out */
-    assert(fgSafeBasicBlockCreation);
+    assert(compiler->fgSafeBasicBlockCreation);
 
-    block = new (this, CMK_BasicBlock) BasicBlock;
+    block = new (compiler, CMK_BasicBlock) BasicBlock;
 
 #if MEASURE_BLOCK_SIZE
     BasicBlock::s_Count += 1;
@@ -1465,7 +1461,7 @@ BasicBlock* Compiler::bbNewBasicBlock()
 
 #ifdef DEBUG
     // fgLookupBB() is invalid until fgInitBBLookup() is called again.
-    fgBBs = (BasicBlock**)0xCDCD;
+    compiler->fgInvalidateBBLookup();
 #endif
 
     // TODO-Throughput: The following memset is pretty expensive - do something else?
@@ -1479,15 +1475,15 @@ BasicBlock* Compiler::bbNewBasicBlock()
     block->bbCodeOffsEnd = BAD_IL_OFFSET;
 
 #ifdef DEBUG
-    block->bbID = compBasicBlockID++;
+    block->bbID = compiler->compBasicBlockID++;
 #endif
 
     /* Give the block a number, set the ancestor count and weight */
 
-    ++fgBBcount;
-    block->bbNum = ++fgBBNumMax;
+    ++compiler->fgBBcount;
+    block->bbNum = ++compiler->fgBBNumMax;
 
-    if (compRationalIRForm)
+    if (compiler->compRationalIRForm)
     {
         block->bbFlags |= BBF_IS_LIR;
     }
@@ -1501,7 +1497,7 @@ BasicBlock* Compiler::bbNewBasicBlock()
     block->bbEntryState = nullptr;
 
 #ifdef DEBUG
-    if (verbose)
+    if (compiler->verbose)
     {
         printf("New Basic Block %s created.\n", block->dspToString());
     }
@@ -1510,21 +1506,21 @@ BasicBlock* Compiler::bbNewBasicBlock()
     // We will give all the blocks var sets after the number of tracked variables
     // is determined and frozen.  After that, if we dynamically create a basic block,
     // we will initialize its var sets.
-    if (fgBBVarSetsInited)
+    if (compiler->fgBBVarSetsInited)
     {
-        VarSetOps::AssignNoCopy(this, block->bbVarUse, VarSetOps::MakeEmpty(this));
-        VarSetOps::AssignNoCopy(this, block->bbVarDef, VarSetOps::MakeEmpty(this));
-        VarSetOps::AssignNoCopy(this, block->bbLiveIn, VarSetOps::MakeEmpty(this));
-        VarSetOps::AssignNoCopy(this, block->bbLiveOut, VarSetOps::MakeEmpty(this));
-        VarSetOps::AssignNoCopy(this, block->bbScope, VarSetOps::MakeEmpty(this));
+        VarSetOps::AssignNoCopy(compiler, block->bbVarUse, VarSetOps::MakeEmpty(compiler));
+        VarSetOps::AssignNoCopy(compiler, block->bbVarDef, VarSetOps::MakeEmpty(compiler));
+        VarSetOps::AssignNoCopy(compiler, block->bbLiveIn, VarSetOps::MakeEmpty(compiler));
+        VarSetOps::AssignNoCopy(compiler, block->bbLiveOut, VarSetOps::MakeEmpty(compiler));
+        VarSetOps::AssignNoCopy(compiler, block->bbScope, VarSetOps::MakeEmpty(compiler));
     }
     else
     {
-        VarSetOps::AssignNoCopy(this, block->bbVarUse, VarSetOps::UninitVal());
-        VarSetOps::AssignNoCopy(this, block->bbVarDef, VarSetOps::UninitVal());
-        VarSetOps::AssignNoCopy(this, block->bbLiveIn, VarSetOps::UninitVal());
-        VarSetOps::AssignNoCopy(this, block->bbLiveOut, VarSetOps::UninitVal());
-        VarSetOps::AssignNoCopy(this, block->bbScope, VarSetOps::UninitVal());
+        VarSetOps::AssignNoCopy(compiler, block->bbVarUse, VarSetOps::UninitVal());
+        VarSetOps::AssignNoCopy(compiler, block->bbVarDef, VarSetOps::UninitVal());
+        VarSetOps::AssignNoCopy(compiler, block->bbLiveIn, VarSetOps::UninitVal());
+        VarSetOps::AssignNoCopy(compiler, block->bbLiveOut, VarSetOps::UninitVal());
+        VarSetOps::AssignNoCopy(compiler, block->bbScope, VarSetOps::UninitVal());
     }
 
     block->bbMemoryUse     = emptyMemoryKindSet;
@@ -1550,10 +1546,15 @@ BasicBlock* Compiler::bbNewBasicBlock()
     return block;
 }
 
-BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind, BasicBlock* jumpDest /* = nullptr */)
+BasicBlock* BasicBlock::bbNewBasicBlock(Compiler* compiler, BBjumpKinds jumpKind, BasicBlock* jumpDest /* = nullptr */)
 {
-    BasicBlock* block = bbNewBasicBlock();
-    block->SetJumpKindAndTarget(jumpKind, jumpDest DEBUG_ARG(this));
+    BasicBlock* block = BasicBlock::bbNewBasicBlock(compiler);
+
+    // In some cases, we don't know a block's jump target during initialization, so don't check the jump kind/target
+    // yet.
+    // The checks will be done any time the jump kind/target is read or written to after initialization.
+    block->bbJumpKind = jumpKind;
+    block->bbJumpDest = jumpDest;
 
     if (jumpKind == BBJ_THROW)
     {
@@ -1563,17 +1564,19 @@ BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind, BasicBlock* jumpDest
     return block;
 }
 
-BasicBlock* Compiler::bbNewBasicBlock(BBswtDesc* jumpSwt)
+BasicBlock* BasicBlock::bbNewBasicBlock(Compiler* compiler, BBswtDesc* jumpSwt)
 {
-    BasicBlock* block = bbNewBasicBlock();
-    block->SetSwitchKindAndTarget(jumpSwt);
+    BasicBlock* block = BasicBlock::bbNewBasicBlock(compiler);
+    block->bbJumpKind = BBJ_SWITCH;
+    block->bbJumpSwt  = jumpSwt;
     return block;
 }
 
-BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind, unsigned jumpOffs)
+BasicBlock* BasicBlock::bbNewBasicBlock(Compiler* compiler, BBjumpKinds jumpKind, unsigned jumpOffs)
 {
-    BasicBlock* block = bbNewBasicBlock();
-    block->SetJumpKindAndTarget(jumpKind, jumpOffs);
+    BasicBlock* block = BasicBlock::bbNewBasicBlock(compiler);
+    block->bbJumpKind = jumpKind;
+    block->bbJumpOffs = jumpOffs;
     return block;
 }
 

diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h
@@ -538,13 +538,10 @@ struct BasicBlock : private LIR::Range
     };
 
 public:
-#ifdef DEBUG
-    // When creating a block with a jump, we require its jump kind and target be initialized simultaneously.
-    // In a few edge cases (for example, in Compiler::impImportLeave), we don't know the jump target at block creation.
-    // In these cases, temporarily set the jump target to bbTempJumpDest, and update the jump target later.
-    // We won't check jump targets against bbTempJumpDest in Release builds.
-    static BasicBlock bbTempJumpDest;
-#endif // DEBUG
+    static BasicBlock* bbNewBasicBlock(Compiler* compiler);
+    static BasicBlock* bbNewBasicBlock(Compiler* compiler, BBjumpKinds jumpKind, BasicBlock* jumpDest = nullptr);
+    static BasicBlock* bbNewBasicBlock(Compiler* compiler, BBswtDesc* jumpSwt);
+    static BasicBlock* bbNewBasicBlock(Compiler* compiler, BBjumpKinds jumpKind, unsigned jumpOffs);
 
     BBjumpKinds GetJumpKind() const
     {
@@ -633,10 +630,6 @@ struct BasicBlock : private LIR::Range
 
     void SetJumpDest(BasicBlock* jumpDest)
     {
-        // If bbJumpKind indicates this block has a jump,
-        // bbJumpDest should have previously been set in SetJumpKindAndTarget().
-        assert(HasJump() || !KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE));
-
         // SetJumpKindAndTarget() nulls jumpDest for non-jump kinds,
         // so don't use SetJumpDest() to null bbJumpDest without updating bbJumpKind.
         bbJumpDest = jumpDest;

diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp
@@ -857,7 +857,7 @@ BasicBlock* CodeGen::genCreateTempLabel()
 #endif
 
     // Label doesn't need a jump kind
-    BasicBlock* block = compiler->bbNewBasicBlock();
+    BasicBlock* block = BasicBlock::bbNewBasicBlock(compiler);
 
 #ifdef DEBUG
     compiler->fgSafeBasicBlockCreation = false;

diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -3221,11 +3221,6 @@ class Compiler
     bool fgSafeBasicBlockCreation;
 #endif
 
-    BasicBlock* bbNewBasicBlock();
-    BasicBlock* bbNewBasicBlock(BBjumpKinds jumpKind, BasicBlock* jumpDest = nullptr);
-    BasicBlock* bbNewBasicBlock(BBswtDesc* jumpSwt);
-    BasicBlock* bbNewBasicBlock(BBjumpKinds jumpKind, unsigned jumpOffs);
-
     /*
     XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
     XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
@@ -5718,6 +5713,10 @@ class Compiler
                                 void*         pCallBackData = nullptr,
                                 bool          computeStack  = false);
 
+#ifdef DEBUG
+    void fgInvalidateBBLookup();
+#endif // DEBUG
+
     /**************************************************************************
      *                          PROTECTED
      *************************************************************************/

diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp
@@ -221,7 +221,7 @@ bool Compiler::fgEnsureFirstBBisScratch()
 
     assert(fgFirstBBScratch == nullptr);
 
-    BasicBlock* block = bbNewBasicBlock(BBJ_NONE);
+    BasicBlock* block = BasicBlock::bbNewBasicBlock(this, BBJ_NONE);
 
     if (fgFirstBB != nullptr)
     {
@@ -802,6 +802,17 @@ BasicBlock* Compiler::fgFirstBlockOfHandler(BasicBlock* block)
     return ehGetDsc(block->getHndIndex())->ebdHndBeg;
 }
 
+#ifdef DEBUG
+//------------------------------------------------------------------------
+// fgInvalidateBBLookup: In non-Release builds, set fgBBs to a dummy value.
+// After calling this, fgInitBBLookup must be called before using fgBBs again.
+//
+void Compiler::fgInvalidateBBLookup()
+{
+    fgBBs = (BasicBlock**)0xCDCD;
+}
+#endif // DEBUG
+
 /*****************************************************************************
  *
  *  The following helps find a basic block given its PC offset.
@@ -3478,18 +3489,18 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F
         switch (jmpKind)
         {
             case BBJ_SWITCH:
-                curBBdesc = bbNewBasicBlock(swtDsc);
+                curBBdesc = BasicBlock::bbNewBasicBlock(this, swtDsc);
                 break;
 
             case BBJ_COND:
             case BBJ_ALWAYS:
             case BBJ_LEAVE:
                 noway_assert(jmpAddr != DUMMY_INIT(BAD_IL_OFFSET));
-                curBBdesc = bbNewBasicBlock(jmpKind, jmpAddr);
+                curBBdesc = BasicBlock::bbNewBasicBlock(this, jmpKind, jmpAddr);
                 break;
 
             default:
-                curBBdesc = bbNewBasicBlock(jmpKind);
+                curBBdesc = BasicBlock::bbNewBasicBlock(this, jmpKind);
                 break;
         }
 
@@ -4733,7 +4744,7 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
     if (!curr->KindIs(BBJ_SWITCH))
     {
         // Start the new block with no refs. When we set the preds below, this will get updated correctly.
-        newBlock         = bbNewBasicBlock(curr->GetJumpKind(), curr->GetJumpDest());
+        newBlock         = BasicBlock::bbNewBasicBlock(this, curr->GetJumpKind(), curr->GetJumpDest());
         newBlock->bbRefs = 0;
 
         for (BasicBlock* const succ : curr->Succs(this))
@@ -4749,7 +4760,7 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
     else
     {
         // Start the new block with no refs. When we set the preds below, this will get updated correctly.
-        newBlock         = bbNewBasicBlock(curr->GetJumpSwt());
+        newBlock         = BasicBlock::bbNewBasicBlock(this, curr->GetJumpSwt());
         newBlock->bbRefs = 0;
 
         // In the case of a switch statement there's more complicated logic in order to wire up the predecessor lists
@@ -5618,8 +5629,11 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst)
         {
             // If bSrc is an unconditional branch to the next block
             // then change it to a BBJ_NONE block
+            // (It's possible for bSrc's jump destination to not be set yet,
+            // so check with BasicBlock::HasJump before calling BasicBlock::JumpsToNext)
             //
-            if (bSrc->KindIs(BBJ_ALWAYS) && !(bSrc->bbFlags & BBF_KEEP_BBJ_ALWAYS) && bSrc->JumpsToNext())
+            if (bSrc->KindIs(BBJ_ALWAYS) && !(bSrc->bbFlags & BBF_KEEP_BBJ_ALWAYS) && bSrc->HasJump() &&
+                bSrc->JumpsToNext())
             {
                 bSrc->SetJumpKindAndTarget(BBJ_NONE DEBUG_ARG(this));
                 JITDUMP("Changed an unconditional jump from " FMT_BB " to the next block " FMT_BB
@@ -6226,7 +6240,7 @@ BasicBlock* Compiler::fgNewBBbefore(BBjumpKinds jumpKind,
 {
     // Create a new BasicBlock and chain it in
 
-    BasicBlock* newBlk = bbNewBasicBlock(jumpKind, jumpDest);
+    BasicBlock* newBlk = BasicBlock::bbNewBasicBlock(this, jumpKind, jumpDest);
     newBlk->bbFlags |= BBF_INTERNAL;
 
     fgInsertBBbefore(block, newBlk);
@@ -6268,7 +6282,7 @@ BasicBlock* Compiler::fgNewBBafter(BBjumpKinds jumpKind,
 {
     // Create a new BasicBlock and chain it in
 
-    BasicBlock* newBlk = bbNewBasicBlock(jumpKind, jumpDest);
+    BasicBlock* newBlk = BasicBlock::bbNewBasicBlock(this, jumpKind, jumpDest);
     newBlk->bbFlags |= BBF_INTERNAL;
 
     fgInsertBBafter(block, newBlk);

diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp
@@ -3039,11 +3039,10 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef
             assert(block->getTryIndex() < compHndBBtabCount);
         }
 
-        // Blocks with these jump kinds must have valid (non-temporary) jump targets
+        // Blocks with these jump kinds must have non-null jump targets
         if (block->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE))
         {
             assert(block->HasJump());
-            assert(!block->HasJumpTo(&BasicBlock::bbTempJumpDest));
         }
 
         // A branch or fall-through to a BBJ_CALLFINALLY block must come from the `try` region associated

diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp
@@ -1661,7 +1661,7 @@ PhaseStatus Compiler::fgPostImportationCleanup()
 
                         // What follows is similar to fgNewBBInRegion, but we can't call that
                         // here as the oldTryEntry is no longer in the main bb list.
-                        newTryEntry = bbNewBasicBlock(BBJ_NONE);
+                        newTryEntry = BasicBlock::bbNewBasicBlock(this, BBJ_NONE);
                         newTryEntry->bbFlags |= (BBF_IMPORTED | BBF_INTERNAL);
                         newTryEntry->bbRefs = 0;
 

diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp
@@ -2985,7 +2985,7 @@ void Compiler::fgInsertFuncletPrologBlock(BasicBlock* block)
 
     /* Allocate a new basic block */
 
-    BasicBlock* newHead = bbNewBasicBlock(BBJ_NONE);
+    BasicBlock* newHead = BasicBlock::bbNewBasicBlock(this, BBJ_NONE);
     newHead->bbFlags |= BBF_INTERNAL;
     newHead->inheritWeight(block);
     newHead->bbRefs = 0;

diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp
@@ -279,9 +279,9 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
     nullcheckOp->gtFlags |= GTF_RELOP_JMP_USED;
 
     // nullcheckBb conditionally jumps to fallbackBb, but we need to initialize fallbackBb last
-    // so we can place it after nullcheckBb. So use a temporary jump target for now.
-    BasicBlock* nullcheckBb = fgNewBBFromTreeAfter(BBJ_COND, prevBb, gtNewOperNode(GT_JTRUE, TYP_VOID, nullcheckOp),
-                                                   debugInfo DEBUG_ARG(&BasicBlock::bbTempJumpDest));
+    // so we can place it after nullcheckBb. So set the jump target later.
+    BasicBlock* nullcheckBb =
+        fgNewBBFromTreeAfter(BBJ_COND, prevBb, gtNewOperNode(GT_JTRUE, TYP_VOID, nullcheckOp), debugInfo);
 
     // Fallback basic block
     GenTree*    fallbackValueDef = gtNewStoreLclVarNode(rtLookupLcl->GetLclNum(), call);
@@ -741,18 +741,15 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement*
     // maxThreadStaticBlocksCondBB
 
     // maxThreadStaticBlocksCondBB conditionally jumps to fallbackBb, but fallbackBb must be initialized last
-    // so it can be placed after it. So use a temporary jump target for now.
-    BasicBlock* maxThreadStaticBlocksCondBB =
-        fgNewBBFromTreeAfter(BBJ_COND, prevBb, tlsValueDef, debugInfo DEBUG_ARG(&BasicBlock::bbTempJumpDest));
+    // so it can be placed after it. So set the jump target later.
+    BasicBlock* maxThreadStaticBlocksCondBB = fgNewBBFromTreeAfter(BBJ_COND, prevBb, tlsValueDef, debugInfo);
 
     fgInsertStmtAfter(maxThreadStaticBlocksCondBB, maxThreadStaticBlocksCondBB->firstStmt(),
                       fgNewStmtFromTree(maxThreadStaticBlocksCond));
 
-    // Similarly, give threadStaticBlockNulLCondBB a temporary jump target for now,
-    // and update it to jump to its real target (fastPathBb) after it is initialized.
+    // Similarly, set threadStaticBlockNulLCondBB to jump to fastPathBb once the latter exists.
     BasicBlock* threadStaticBlockNullCondBB =
-        fgNewBBFromTreeAfter(BBJ_COND, maxThreadStaticBlocksCondBB, threadStaticBlockBaseDef,
-                             debugInfo DEBUG_ARG(&BasicBlock::bbTempJumpDest));
+        fgNewBBFromTreeAfter(BBJ_COND, maxThreadStaticBlocksCondBB, threadStaticBlockBaseDef, debugInfo);
     fgInsertStmtAfter(threadStaticBlockNullCondBB, threadStaticBlockNullCondBB->firstStmt(),
                       fgNewStmtFromTree(threadStaticBlockNullCond));