Improve some BasicBlock asserts (dotnet#96231)

1. Ensure that the `bbTarget` field is never read except by
block kinds for which `HasTarget()` is `true`.
2. Remove `BBJ_COND` from these kinds, since it now has its
own true/false targets.
3. Add a `TransferTargets()` function which is like `CopyTargets()`
but it takes memory ownership of the target descriptors for switch/
ehfinallyret which are then invalidated, instead of creating a new copy.
4. Stop using `JumpsToNext()` for `BBJ_COND`

src/coreclr/jit/block.cpp

@@ -816,7 +816,9 @@ bool BasicBlock::CloneBlockState(
 }
 
 //------------------------------------------------------------------------
-// CopyTarget: Copy the block kind and targets.
+// CopyTarget: Copy the block kind and targets. The targets in the `from` block remain valid.
+// Use `TransferTarget` to copy the pointer to the target descriptor (e.g., for BBJ_SWITCH/BBJ_EHFINALLYRET)
+// after which the `from` block target is invalid.
 //
 // Arguments:
 //    compiler - Jit compiler instance
@@ -835,10 +837,61 @@ void BasicBlock::CopyTarget(Compiler* compiler, const BasicBlock* from)
         case BBJ_COND:
             SetCond(from->GetTrueTarget());
             break;
+        case BBJ_ALWAYS:
+            SetKindAndTarget(from->GetKind(), from->GetTarget());
+            CopyFlags(from, BBF_NONE_QUIRK);
+            break;
+        case BBJ_CALLFINALLY:
+        case BBJ_CALLFINALLYRET:
+        case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
+        case BBJ_LEAVE:
+            SetKindAndTarget(from->GetKind(), from->GetTarget());
+            break;
         default:
+            SetKindAndTarget(from->GetKind()); // Clear the target
+            break;
+    }
+    assert(KindIs(from->GetKind()));
+}
+
+//------------------------------------------------------------------------
+// TransferTarget: Like CopyTarget, but copies the target descriptors for block types which have
+// them (BBJ_SWITCH/BBJ_EHFINALLYRET), that is, take their memory, after which the `from` block
+// target is invalid.
+//
+// Arguments:
+//    from - Block to transfer from
+//
+void BasicBlock::TransferTarget(BasicBlock* from)
+{
+    switch (from->GetKind())
+    {
+        case BBJ_SWITCH:
+            SetSwitch(from->GetSwitchTargets());
+            from->bbSwtTargets = nullptr; // Make sure nobody uses the descriptor after this.
+            break;
+        case BBJ_EHFINALLYRET:
+            SetEhf(from->GetEhfTargets());
+            from->bbEhfTargets = nullptr; // Make sure nobody uses the descriptor after this.
+            break;
+        case BBJ_COND:
+            SetCond(from->GetTrueTarget());
+            break;
+        case BBJ_ALWAYS:
             SetKindAndTarget(from->GetKind(), from->GetTarget());
             CopyFlags(from, BBF_NONE_QUIRK);
             break;
+        case BBJ_CALLFINALLY:
+        case BBJ_CALLFINALLYRET:
+        case BBJ_EHCATCHRET:
+        case BBJ_EHFILTERRET:
+        case BBJ_LEAVE:
+            SetKindAndTarget(from->GetKind(), from->GetTarget());
+            break;
+        default:
+            SetKindAndTarget(from->GetKind()); // Clear the target
+            break;
     }
     assert(KindIs(from->GetKind()));
 }
@@ -1472,7 +1525,7 @@ BasicBlock* BasicBlock::New(Compiler* compiler)
 #endif
 
     // TODO-Throughput: The following memset is pretty expensive - do something else?
-    // Note that some fields have to be initialized to 0 (like bbFPStateX87)
+    // Note that some fields have to be initialized to 0.
     memset((void*)block, 0, sizeof(*block));
 
     // scopeInfo needs to be able to differentiate between blocks which
@@ -1574,17 +1627,15 @@ BasicBlock* BasicBlock::New(Compiler* compiler, BBKinds kind, BasicBlock* target
 
 BasicBlock* BasicBlock::New(Compiler* compiler, BBehfDesc* ehfTargets)
 {
-    BasicBlock* block   = BasicBlock::New(compiler);
-    block->bbKind       = BBJ_EHFINALLYRET;
-    block->bbEhfTargets = ehfTargets;
+    BasicBlock* block = BasicBlock::New(compiler);
+    block->SetEhf(ehfTargets);
     return block;
 }
 
 BasicBlock* BasicBlock::New(Compiler* compiler, BBswtDesc* swtTargets)
 {
-    BasicBlock* block   = BasicBlock::New(compiler);
-    block->bbKind       = BBJ_SWITCH;
-    block->bbSwtTargets = swtTargets;
+    BasicBlock* block = BasicBlock::New(compiler);
+    block->SetSwitch(swtTargets);
     return block;
 }
 

src/coreclr/jit/block.h

@@ -633,35 +633,28 @@ struct BasicBlock : private LIR::Range
     bool HasTarget() const
     {
         // These block types should always have bbTarget set
-        return KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_CALLFINALLYRET, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET,
-                      BBJ_LEAVE);
+        return KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_CALLFINALLYRET, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE);
     }
 
     BasicBlock* GetTarget() const
     {
-        // BBJ_COND should use GetTrueTarget, and BBJ_EHFINALLYRET/BBJ_SWITCH don't use bbTarget
-        assert(!KindIs(BBJ_COND, BBJ_EHFINALLYRET, BBJ_SWITCH));
-
-        // If bbKind indicates this block has a jump, bbTarget cannot be null
-        assert(!HasTarget() || HasInitializedTarget());
+        // Only block kinds that use `bbTarget` can access it, and it must be non-null.
+        assert(HasInitializedTarget());
         return bbTarget;
     }
 
     void SetTarget(BasicBlock* target)
     {
-        // BBJ_COND should use SetTrueTarget, and BBJ_EHFINALLYRET/BBJ_SWITCH don't use bbTarget
-        assert(!KindIs(BBJ_COND, BBJ_EHFINALLYRET, BBJ_SWITCH));
-
         // SetKindAndTarget() nulls target for non-jump kinds,
         // so don't use SetTarget() to null bbTarget without updating bbKind.
         bbTarget = target;
-        assert(!HasTarget() || HasInitializedTarget());
+        assert(HasInitializedTarget());
     }
 
     BasicBlock* GetTrueTarget() const
     {
         assert(KindIs(BBJ_COND));
-        assert(HasInitializedTarget());
+        assert(bbTrueTarget != nullptr);
         return bbTrueTarget;
     }
 
@@ -675,7 +668,7 @@ struct BasicBlock : private LIR::Range
     bool TrueTargetIs(const BasicBlock* target) const
     {
         assert(KindIs(BBJ_COND));
-        assert(HasInitializedTarget());
+        assert(bbTrueTarget != nullptr);
         assert(target != nullptr);
         return (bbTrueTarget == target);
     }
@@ -707,25 +700,24 @@ struct BasicBlock : private LIR::Range
         return (bbFalseTarget == target);
     }
 
-    void SetCond(BasicBlock* target)
+    void SetCond(BasicBlock* trueTarget)
     {
-        assert(target != nullptr);
+        assert(trueTarget != nullptr);
         bbKind       = BBJ_COND;
-        bbTrueTarget = target;
+        bbTrueTarget = trueTarget;
+        // TODO-NoFallThrough: also set bbFalseTarget
     }
 
+    // Set both the block kind and target. This can clear `bbTarget` when setting
+    // block kinds that don't use `bbTarget`.
     void SetKindAndTarget(BBKinds kind, BasicBlock* target = nullptr)
     {
-        // For BBJ_COND/BBJ_EHFINALLYRET/BBJ_SWITCH, use SetCond/SetEhf/SetSwitch
-        assert(kind != BBJ_COND);
-        assert(kind != BBJ_EHFINALLYRET);
-        assert(kind != BBJ_SWITCH);
-
         bbKind   = kind;
         bbTarget = target;
 
-        // If bbKind indicates this block has a jump, bbTarget cannot be null
-        assert(!HasTarget() || HasInitializedTarget());
+        // If bbKind indicates this block has a jump, bbTarget cannot be null.
+        // You shouldn't use this to set a BBJ_COND, BBJ_SWITCH, or BBJ_EHFINALLYRET.
+        assert(HasTarget() ? HasInitializedTarget() : (bbTarget == nullptr));
     }
 
     bool HasInitializedTarget() const
@@ -736,16 +728,12 @@ struct BasicBlock : private LIR::Range
 
     bool TargetIs(const BasicBlock* target) const
     {
-        // BBJ_COND should use TrueTargetIs, and BBJ_EHFINALLYRET/BBJ_SWITCH don't use bbTarget
-        assert(!KindIs(BBJ_COND, BBJ_EHFINALLYRET, BBJ_SWITCH));
-        assert(HasInitializedTarget());
-        return (bbTarget == target);
+        return (GetTarget() == target);
     }
 
     bool JumpsToNext() const
     {
-        assert(HasInitializedTarget());
-        return (bbTarget == bbNext);
+        return (GetTarget() == bbNext);
     }
 
     BBswtDesc* GetSwitchTargets() const
@@ -1674,9 +1662,12 @@ struct BasicBlock : private LIR::Range
     static bool CloneBlockState(
         Compiler* compiler, BasicBlock* to, const BasicBlock* from, unsigned varNum = (unsigned)-1, int varVal = 0);
 
-    // Copy the block kind and targets.
+    // Copy the block kind and targets. The `from` block is untouched.
     void CopyTarget(Compiler* compiler, const BasicBlock* from);
 
+    // Copy the block kind and take memory ownership of the targets.
+    void TransferTarget(BasicBlock* from);
+
     void MakeLIR(GenTree* firstNode, GenTree* lastNode);
     bool IsLIR() const;
 
@@ -1952,7 +1943,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
 
             // If both fall-through and branch successors are identical, then only include
             // them once in the iteration (this is the same behavior as NumSucc()/GetSucc()).
-            if (block->JumpsToNext())
+            if (block->TrueTargetIs(block->GetFalseTarget()))
             {
                 m_end = &m_succs[1];
             }

src/coreclr/jit/fgbasic.cpp

@@ -4763,26 +4763,7 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
     // (We need the successors of 'curr' to be correct when we do this.)
     BasicBlock* newBlock;
 
-    // For each successor of the original block, set the new block as their predecessor.
-    // Note we are using the "rational" version of the successor iterator that does not hide the finallyret arcs.
-    // Without these arcs, a block 'b' may not be a member of succs(preds(b))
-    switch (curr->GetKind())
-    {
-        case BBJ_COND:
-            newBlock = BasicBlock::New(this, BBJ_COND, curr->GetTrueTarget());
-            break;
-
-        case BBJ_EHFINALLYRET:
-            newBlock = BasicBlock::New(this, curr->GetEhfTargets());
-            break;
-
-        case BBJ_SWITCH:
-            newBlock = BasicBlock::New(this, curr->GetSwitchTargets());
-            break;
-
-        default:
-            newBlock = BasicBlock::New(this, curr->GetKind(), curr->GetTarget());
-    }
+    newBlock = BasicBlock::New(this);
 
     // Start the new block with no refs. When we set the preds below, this will get updated correctly.
     newBlock->bbRefs = 0;
@@ -4795,6 +4776,8 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
     }
     else
     {
+        // For each successor of the original block, set the new block as their predecessor.
+
         for (BasicBlock* const succ : curr->Succs(this))
         {
             if (succ != newBlock)
@@ -4806,6 +4789,10 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
         }
     }
 
+    // Transfer the kind and target. Do this after the code above, to avoid null-ing out the old targets used by the
+    // above code.
+    newBlock->TransferTarget(curr);
+
     newBlock->inheritWeight(curr);
 
     // Set the new block's flags. Note that the new block isn't BBF_INTERNAL unless the old block is.
@@ -4834,12 +4821,13 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
     // Remove flags from the old block that are no longer possible.
     curr->RemoveFlags(BBF_HAS_JMP | BBF_RETLESS_CALL);
 
-    // Default to fallthru, and add the arc for that.
-    curr->SetFlags(BBF_NONE_QUIRK);
+    // Default to fallthrough, and add the arc for that.
     curr->SetKindAndTarget(BBJ_ALWAYS, newBlock);
-    fgAddRefPred(newBlock, curr);
+    curr->SetFlags(BBF_NONE_QUIRK);
     assert(curr->JumpsToNext());
 
+    fgAddRefPred(newBlock, curr);
+
     return newBlock;
 }
 

src/coreclr/jit/fgdiagnostic.cpp

@@ -76,7 +76,7 @@ void Compiler::fgDebugCheckUpdate()
      * no unreachable blocks  -> no blocks have countOfInEdges() = 0
      * no empty blocks        -> !block->isEmpty(), unless non-removable or multiple in-edges
      * no un-imported blocks  -> no blocks have BBF_IMPORTED not set (this is
-     *                           kind of redundand with the above, but to make sure)
+     *                           kind of redundant with the above, but to make sure)
      * no un-compacted blocks -> BBJ_ALWAYS with jump to block with no other jumps to it (countOfInEdges() = 1)
      */
 
@@ -132,21 +132,11 @@ void Compiler::fgDebugCheckUpdate()
             }
         }
 
-        // 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
-
-        if (block->KindIs(BBJ_COND))
+        // Check for an unnecessary jumps to the next block.
+        // A conditional branch should never jump to the next block as it can be folded into a BBJ_ALWAYS.
+        if (block->KindIs(BBJ_COND) && block->TrueTargetIs(block->GetFalseTarget()))
         {
-            // A conditional branch should never jump to the next block as it can be folded into a BBJ_ALWAYS.
-            doAssertOnJumpToNextBlock = true;
-        }
-
-        if (doAssertOnJumpToNextBlock)
-        {
-            if (block->JumpsToNext())
-            {
-                noway_assert(!"Unnecessary jump to the next block!");
-            }
+            noway_assert(!"Unnecessary jump to the next block!");
         }
 
         // For a BBJ_CALLFINALLY block we make sure that we are followed by a BBJ_CALLFINALLYRET block

src/coreclr/jit/flowgraph.cpp

@@ -267,29 +267,25 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
         // I want to create:
         // top -> poll -> bottom (lexically)
         // so that we jump over poll to get to bottom.
-        BasicBlock*   top            = block;
-        BasicBlock*   topFallThrough = nullptr;
-        BasicBlock*   topJumpTarget;
+        BasicBlock*   top                = block;
         unsigned char lpIndexFallThrough = BasicBlock::NOT_IN_LOOP;
 
         if (top->KindIs(BBJ_COND))
         {
-            topFallThrough     = top->GetFalseTarget();
-            topJumpTarget      = top->GetTrueTarget();
-            lpIndexFallThrough = topFallThrough->bbNatLoopNum;
-        }
-        else
-        {
-            topJumpTarget = top->GetTarget();
+            lpIndexFallThrough = top->GetFalseTarget()->bbNatLoopNum;
         }
 
-        BasicBlock* poll          = fgNewBBafter(BBJ_ALWAYS, top, true);
-        bottom                    = fgNewBBafter(top->GetKind(), poll, true, topJumpTarget);
-        BBKinds       oldJumpKind = top->GetKind();
-        unsigned char lpIndex     = top->bbNatLoopNum;
+        BasicBlock* poll = fgNewBBafter(BBJ_ALWAYS, top, true);
+        bottom           = fgNewBBafter(top->GetKind(), poll, true);
+
         poll->SetTarget(bottom);
         assert(poll->JumpsToNext());
 
+        bottom->TransferTarget(top);
+
+        BBKinds       oldJumpKind = top->GetKind();
+        unsigned char lpIndex     = top->bbNatLoopNum;
+
         // Update block flags
         const BasicBlockFlags originalFlags = top->GetFlagsRaw() | BBF_GC_SAFE_POINT;
 

src/coreclr/jit/ifconversion.cpp

@@ -549,7 +549,7 @@ void OptIfConversionDsc::IfConvertDump()
 bool OptIfConversionDsc::optIfConvert()
 {
     // Does the block end by branching via a JTRUE after a compare?
-    if (!m_startBlock->KindIs(BBJ_COND) || m_startBlock->NumSucc() != 2)
+    if (!m_startBlock->KindIs(BBJ_COND) || (m_startBlock->NumSucc() != 2))
     {
         return false;
     }
@@ -739,8 +739,7 @@ bool OptIfConversionDsc::optIfConvert()
 
     // Update the flow from the original block.
     m_comp->fgRemoveAllRefPreds(m_startBlock->GetFalseTarget(), m_startBlock);
-    assert(m_startBlock->HasInitializedTarget());
-    m_startBlock->SetKind(BBJ_ALWAYS);
+    m_startBlock->SetKindAndTarget(BBJ_ALWAYS, m_startBlock->GetTrueTarget());
 
 #ifdef DEBUG
     if (m_comp->verbose)