Revert "JIT: Allow BBJ_COND false target to diverge from bbNext in la…

…yout optimization phase (dotnet#96609)"

This reverts commit 5598dac.

src/coreclr/jit/block.cpp

@@ -301,20 +301,18 @@ bool BasicBlock::CanRemoveJumpToNext(Compiler* compiler) const
 }
 
 //------------------------------------------------------------------------
-// CanRemoveJumpToTarget: determine if jump to target can be omitted
+// CanRemoveJumpToFalseTarget: determine if jump to false target can be omitted
 //
 // Arguments:
-//    target - true/false target of BBJ_COND block
 //    compiler - current compiler instance
 //
 // Returns:
-//    true if block is a BBJ_COND that can fall into target
+//    true if block is a BBJ_COND that can fall into its false target
 //
-bool BasicBlock::CanRemoveJumpToTarget(BasicBlock* target, Compiler* compiler) const
+bool BasicBlock::CanRemoveJumpToFalseTarget(Compiler* compiler) const
 {
     assert(KindIs(BBJ_COND));
-    assert(TrueTargetIs(target) || FalseTargetIs(target));
-    return NextIs(target) && !compiler->fgInDifferentRegions(this, target);
+    return NextIs(bbFalseTarget) && !hasAlign() && !compiler->fgInDifferentRegions(this, bbFalseTarget);
 }
 
 //------------------------------------------------------------------------
@@ -1171,7 +1169,7 @@ unsigned BasicBlock::NumSucc() const
             return 1;
 
         case BBJ_COND:
-            if (bbTrueTarget == bbFalseTarget)
+            if (bbTarget == bbNext)
             {
                 return 1;
             }
@@ -1296,7 +1294,7 @@ unsigned BasicBlock::NumSucc(Compiler* comp)
             return 1;
 
         case BBJ_COND:
-            if (bbTrueTarget == bbFalseTarget)
+            if (bbTarget == bbNext)
             {
                 return 1;
             }

src/coreclr/jit/block.h

@@ -616,7 +616,7 @@ struct BasicBlock : private LIR::Range
 
     bool CanRemoveJumpToNext(Compiler* compiler) const;
 
-    bool CanRemoveJumpToTarget(BasicBlock* target, Compiler* compiler) const;
+    bool CanRemoveJumpToFalseTarget(Compiler* compiler) const;
 
     unsigned GetTargetOffs() const
     {
@@ -669,6 +669,7 @@ struct BasicBlock : private LIR::Range
     {
         assert(KindIs(BBJ_COND));
         assert(bbTrueTarget != nullptr);
+        assert(target != nullptr);
         return (bbTrueTarget == target);
     }
 
@@ -695,16 +696,15 @@ struct BasicBlock : private LIR::Range
     {
         assert(KindIs(BBJ_COND));
         assert(bbFalseTarget != nullptr);
+        assert(target != nullptr);
         return (bbFalseTarget == target);
     }
 
     void SetCond(BasicBlock* trueTarget, BasicBlock* falseTarget)
     {
-        // Switch lowering may temporarily set a block to a BBJ_COND
-        // with a null false target if it is the last block in the list.
-        // This invalid state is eventually fixed, so allow it in the below assert.
-        assert((falseTarget != nullptr) || (falseTarget == bbNext));
         assert(trueTarget != nullptr);
+        // TODO-NoFallThrough: Allow falseTarget to diverge from bbNext
+        assert(falseTarget == bbNext);
         bbKind        = BBJ_COND;
         bbTrueTarget  = trueTarget;
         bbFalseTarget = falseTarget;

src/coreclr/jit/codegenarm.cpp

@@ -1317,13 +1317,6 @@ void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
     regNumber reg = genConsumeReg(op);
     inst_RV_RV(INS_tst, reg, reg, genActualType(op));
     inst_JMP(EJ_ne, compiler->compCurBB->GetTrueTarget());
-
-    // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-    {
-        inst_JMP(EJ_jmp, falseTarget);
-    }
 }
 
 //------------------------------------------------------------------------

src/coreclr/jit/codegenarm64.cpp

@@ -4650,13 +4650,6 @@ void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
     GenTree*  op  = jtrue->gtGetOp1();
     regNumber reg = genConsumeReg(op);
     GetEmitter()->emitIns_J_R(INS_cbnz, emitActualTypeSize(op), compiler->compCurBB->GetTrueTarget(), reg);
-
-    // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-    {
-        inst_JMP(EJ_jmp, falseTarget);
-    }
 }
 
 //------------------------------------------------------------------------
@@ -4884,13 +4877,6 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree)
 
         GetEmitter()->emitIns_J_R(ins, attr, compiler->compCurBB->GetTrueTarget(), reg);
     }
-
-    // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-    {
-        inst_JMP(EJ_jmp, falseTarget);
-    }
 }
 
 //---------------------------------------------------------------------

src/coreclr/jit/codegencommon.cpp

@@ -397,7 +397,7 @@ void CodeGen::genMarkLabelsForCodegen()
                 block->GetTrueTarget()->SetFlags(BBF_HAS_LABEL);
 
                 // If we need a jump to the false target, give it a label
-                if (!block->CanRemoveJumpToTarget(block->GetFalseTarget(), compiler))
+                if (!block->CanRemoveJumpToFalseTarget(compiler))
                 {
                     JITDUMP("  " FMT_BB " : branch target\n", block->GetFalseTarget()->bbNum);
                     block->GetFalseTarget()->SetFlags(BBF_HAS_LABEL);

src/coreclr/jit/codegenlinear.cpp

@@ -2640,10 +2640,9 @@ void CodeGen::genCodeForJcc(GenTreeCC* jcc)
     inst_JCC(jcc->gtCondition, compiler->compCurBB->GetTrueTarget());
 
     // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
+    if (!compiler->compCurBB->CanRemoveJumpToFalseTarget(compiler))
     {
-        inst_JMP(EJ_jmp, falseTarget);
+        inst_JMP(EJ_jmp, compiler->compCurBB->GetFalseTarget());
     }
 }
 

src/coreclr/jit/codegenloongarch64.cpp

@@ -4330,13 +4330,6 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree)
     assert(regs != 0);
 
     emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // 5-bits;
-
-    // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-    {
-        inst_JMP(EJ_jmp, falseTarget);
-    }
 }
 
 //---------------------------------------------------------------------
@@ -4881,31 +4874,16 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
 
         case GT_JCC:
         {
-#if !FEATURE_FIXED_OUT_ARGS
             BasicBlock* tgtBlock = compiler->compCurBB->KindIs(BBJ_COND) ? compiler->compCurBB->GetTrueTarget()
                                                                          : compiler->compCurBB->GetTarget();
+#if !FEATURE_FIXED_OUT_ARGS
             assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed());
 #endif // !FEATURE_FIXED_OUT_ARGS
 
             GenTreeCC* jcc = treeNode->AsCC();
             assert(jcc->gtCondition.Is(GenCondition::EQ, GenCondition::NE));
             instruction ins = jcc->gtCondition.Is(GenCondition::EQ) ? INS_bceqz : INS_bcnez;
-
-            if (compiler->compCurBB->KindIs(BBJ_COND))
-            {
-                emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), (int)1 /* cc */);
-
-                // If we cannot fall into the false target, emit a jump to it
-                BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-                if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-                {
-                    inst_JMP(EJ_jmp, falseTarget);
-                }
-            }
-            else
-            {
-                emit->emitIns_J(ins, compiler->compCurBB->GetTarget(), (int)1 /* cc */);
-            }
+            emit->emitIns_J(ins, tgtBlock, (int)1 /* cc */);
         }
         break;
 

src/coreclr/jit/codegenriscv64.cpp

@@ -4240,13 +4240,6 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree)
     assert(regs != 0);
 
     emit->emitIns_J(ins, compiler->compCurBB->GetTrueTarget(), regs); // 5-bits;
-
-    // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-    {
-        inst_JMP(EJ_jmp, falseTarget);
-    }
 }
 
 //---------------------------------------------------------------------

src/coreclr/jit/codegenxarch.cpp

@@ -1466,13 +1466,6 @@ void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
     regNumber reg = genConsumeReg(op);
     inst_RV_RV(INS_test, reg, reg, genActualType(op));
     inst_JMP(EJ_jne, compiler->compCurBB->GetTrueTarget());
-
-    // If we cannot fall into the false target, emit a jump to it
-    BasicBlock* falseTarget = compiler->compCurBB->GetFalseTarget();
-    if (!compiler->compCurBB->CanRemoveJumpToTarget(falseTarget, compiler))
-    {
-        inst_JMP(EJ_jmp, falseTarget);
-    }
 }
 
 //------------------------------------------------------------------------

src/coreclr/jit/compiler.h

@@ -5919,7 +5919,7 @@ class Compiler
 
     void fgCompactBlocks(BasicBlock* block, BasicBlock* bNext);
 
-    BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst, bool noFallThroughQuirk = false);
+    BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst);
 
     bool fgRenumberBlocks();
 
@@ -5966,6 +5966,8 @@ class Compiler
 
     bool fgOptimizeSwitchBranches(BasicBlock* block);
 
+    bool fgOptimizeBranchToNext(BasicBlock* block, BasicBlock* bNext, BasicBlock* bPrev);
+
     bool fgOptimizeSwitchJumps();
 #ifdef DEBUG
     void fgPrintEdgeWeights();

src/coreclr/jit/fgbasic.cpp

@@ -693,35 +693,15 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* newTarget, Bas
             break;
 
         case BBJ_COND:
-        {
-            FlowEdge* oldEdge = fgGetPredForBlock(oldTarget, block);
-            assert(oldEdge != nullptr);
 
+            // Functionally equivalent to above
             if (block->TrueTargetIs(oldTarget))
             {
-                if (block->FalseTargetIs(oldTarget))
-                {
-                    assert(oldEdge->getDupCount() == 2);
-                    fgRemoveConditionalJump(block);
-                    assert(block->KindIs(BBJ_ALWAYS));
-                    block->SetTarget(newTarget);
-                }
-                else
-                {
-                    block->SetTrueTarget(newTarget);
-                }
-            }
-            else
-            {
-                assert(block->FalseTargetIs(oldTarget));
-                block->SetFalseTarget(newTarget);
+                block->SetTrueTarget(newTarget);
+                fgRemoveRefPred(oldTarget, block);
+                fgAddRefPred(newTarget, block);
             }
-
-            assert(oldEdge->getDupCount() == 1);
-            fgRemoveRefPred(oldTarget, block);
-            fgAddRefPred(newTarget, block);
             break;
-        }
 
         case BBJ_SWITCH:
         {
@@ -5072,17 +5052,13 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)
 
     if (curr->KindIs(BBJ_COND))
     {
+        curr->SetFalseTarget(curr->Next());
+        fgReplacePred(succ, curr, newBlock);
         if (curr->TrueTargetIs(succ))
         {
+            // Now 'curr' jumps to newBlock
             curr->SetTrueTarget(newBlock);
         }
-        else
-        {
-            assert(curr->FalseTargetIs(succ));
-            curr->SetFalseTarget(newBlock);
-        }
-
-        fgReplacePred(succ, curr, newBlock);
         fgAddRefPred(newBlock, curr);
     }
     else if (curr->KindIs(BBJ_SWITCH))
@@ -5292,12 +5268,6 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
 
         /* At this point the bbPreds and bbRefs had better be zero */
         noway_assert((block->bbRefs == 0) && (block->bbPreds == nullptr));
-
-        if (bPrev->KindIs(BBJ_COND) && bPrev->FalseTargetIs(block))
-        {
-            assert(bNext != nullptr);
-            bPrev->SetFalseTarget(bNext);
-        }
     }
     else // block is empty
     {
@@ -5396,15 +5366,24 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
                     break;
 
                 case BBJ_COND:
-                    if (predBlock->TrueTargetIs(block))
+                    /* The links for the direct predecessor case have already been updated above */
+                    if (!predBlock->TrueTargetIs(block))
                     {
-                        predBlock->SetTrueTarget(succBlock);
+                        break;
                     }
 
-                    if (predBlock->FalseTargetIs(block))
+                    /* Check if both sides of the BBJ_COND now jump to the same block */
+                    if (predBlock->FalseTargetIs(succBlock))
                     {
-                        predBlock->SetFalseTarget(succBlock);
+                        // Make sure we are replacing "block" with "succBlock" in predBlock->bbTarget.
+                        noway_assert(predBlock->TrueTargetIs(block));
+                        predBlock->SetTrueTarget(succBlock);
+                        fgRemoveConditionalJump(predBlock);
+                        break;
                     }
+
+                    noway_assert(predBlock->TrueTargetIs(block));
+                    predBlock->SetTrueTarget(succBlock);
                     break;
 
                 case BBJ_CALLFINALLY:
@@ -5439,9 +5418,7 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
                 break;
 
             case BBJ_COND:
-                // block should not be a target anymore
-                assert(!bPrev->TrueTargetIs(block));
-                assert(!bPrev->FalseTargetIs(block));
+                bPrev->SetFalseTarget(block->Next());
 
                 /* Check if both sides of the BBJ_COND now jump to the same block */
                 if (bPrev->TrueTargetIs(bPrev->GetFalseTarget()))
@@ -5509,26 +5486,16 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block)
 //   Newly inserted block after bSrc that jumps to bDst,
 //   or nullptr if bSrc already falls through to bDst
 //
-BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst, bool noFallThroughQuirk /* = false */)
+BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst)
 {
     assert(bSrc != nullptr);
     assert(fgPredsComputed);
     BasicBlock* jmpBlk = nullptr;
 
     /* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */
 
-    if (bSrc->KindIs(BBJ_COND) && bSrc->FalseTargetIs(bDst) && !bSrc->NextIs(bDst))
+    if (bSrc->KindIs(BBJ_COND) && !bSrc->NextIs(bDst))
     {
-        assert(fgGetPredForBlock(bDst, bSrc) != nullptr);
-
-        // Allow the caller to decide whether to use the old logic of maintaining implicit fallthrough behavior,
-        // or to allow BBJ_COND blocks' false targets to diverge from bbNext.
-        // TODO-NoFallThrough: Remove this quirk once callers' dependencies on implicit fallthrough are gone.
-        if (noFallThroughQuirk)
-        {
-            return jmpBlk;
-        }
-
         // Add a new block after bSrc which jumps to 'bDst'
         jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst);
         bSrc->SetFalseTarget(jmpBlk);
@@ -5699,14 +5666,8 @@ bool Compiler::fgRenumberBlocks()
  */
 bool Compiler::fgIsForwardBranch(BasicBlock* bJump, BasicBlock* bDest, BasicBlock* bSrc /* = NULL */)
 {
-    if (bJump->KindIs(BBJ_COND))
-    {
-        assert(bJump->TrueTargetIs(bDest) || bJump->FalseTargetIs(bDest));
-    }
-    else
-    {
-        assert(bJump->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLYRET) && bJump->TargetIs(bDest));
-    }
+    assert((bJump->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLYRET) && bJump->TargetIs(bDest)) ||
+           (bJump->KindIs(BBJ_COND) && bJump->TrueTargetIs(bDest)));
 
     bool        result = false;
     BasicBlock* bTemp  = (bSrc == nullptr) ? bJump : bSrc;