Skip to content

Commit 25efee0

Browse files
amanasifkhalidamanasifkhalid
authored
JIT: Set bbFalseTarget upon BBJ_COND initialization/modification (#96265)
Part of #93020. Previously, bbFalseTarget was hard-coded to match bbNext in BasicBlock::SetNext. We still require bbFalseTarget to point to the next block for BBJ_COND blocks, but I've removed the logic for updating bbFalseTarget from SetNext, and placed calls to SetFalseTarget wherever bbFalseTarget needs to be updated because the BBJ_COND block has been created or moved relative to its false successor. This helps set us up to start removing logic that enforces the block's false successor is the next block.
1 parent 9057843 commit 25efee0

13 files changed

Lines changed: 130 additions & 120 deletions

‎src/coreclr/jit/block.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1123,7 +1123,7 @@ bool BasicBlock::bbFallsThrough() const
11231123
return false;
11241124

11251125
case BBJ_COND:
1126-
return NextIs(GetFalseTarget());
1126+
return true;
11271127

11281128
case BBJ_CALLFINALLY:
11291129
return !HasFlag(BBF_RETLESS_CALL);

‎src/coreclr/jit/block.h‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -584,12 +584,6 @@ struct BasicBlock : private LIR::Range
584584
{
585585
next->bbPrev = this;
586586
}
587-
588-
// BBJ_COND convenience: This ensures bbFalseTarget is always consistent with bbNext.
589-
// For now, if a BBJ_COND's bbTrueTarget is not taken, we expect to fall through,
590-
// so bbFalseTarget must be the next block.
591-
// TODO-NoFallThrough: Remove this once we allow bbFalseTarget to diverge from bbNext
592-
bbFalseTarget = next;
593587
}
594588

595589
bool IsFirst() const
@@ -703,9 +697,9 @@ struct BasicBlock : private LIR::Range
703697
void SetCond(BasicBlock* trueTarget)
704698
{
705699
assert(trueTarget != nullptr);
706-
bbKind = BBJ_COND;
707-
bbTrueTarget = trueTarget;
708-
// TODO-NoFallThrough: also set bbFalseTarget
700+
bbKind = BBJ_COND;
701+
bbTrueTarget = trueTarget;
702+
bbFalseTarget = bbNext;
709703
}
710704

711705
// Set both the block kind and target. This can clear `bbTarget` when setting

‎src/coreclr/jit/fgbasic.cpp‎

Lines changed: 67 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -4761,9 +4761,7 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
47614761
{
47624762
// We'd like to use fgNewBBafter(), but we need to update the preds list before linking in the new block.
47634763
// (We need the successors of 'curr' to be correct when we do this.)
4764-
BasicBlock* newBlock;
4765-
4766-
newBlock = BasicBlock::New(this);
4764+
BasicBlock* newBlock = BasicBlock::New(this);
47674765

47684766
// Start the new block with no refs. When we set the preds below, this will get updated correctly.
47694767
newBlock->bbRefs = 0;
@@ -4789,10 +4787,6 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
47894787
}
47904788
}
47914789

4792-
// Transfer the kind and target. Do this after the code above, to avoid null-ing out the old targets used by the
4793-
// above code.
4794-
newBlock->TransferTarget(curr);
4795-
47964790
newBlock->inheritWeight(curr);
47974791

47984792
// Set the new block's flags. Note that the new block isn't BBF_INTERNAL unless the old block is.
@@ -4821,6 +4815,11 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
48214815
// Remove flags from the old block that are no longer possible.
48224816
curr->RemoveFlags(BBF_HAS_JMP | BBF_RETLESS_CALL);
48234817

4818+
// Transfer the kind and target. Do this after the code above, to avoid null-ing out the old targets used by the
4819+
// above code (and so newBlock->bbNext is valid, so SetCond() can initialize bbFalseTarget if newBlock is a
4820+
// BBJ_COND).
4821+
newBlock->TransferTarget(curr);
4822+
48244823
// Default to fallthrough, and add the arc for that.
48254824
curr->SetKindAndTarget(BBJ_ALWAYS, newBlock);
48264825
curr->SetFlags(BBF_NONE_QUIRK);
@@ -5080,6 +5079,7 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)
50805079

50815080
if (curr->KindIs(BBJ_COND))
50825081
{
5082+
curr->SetFalseTarget(curr->Next());
50835083
fgReplacePred(succ, curr, newBlock);
50845084
if (curr->TrueTargetIs(succ))
50855085
{
@@ -5455,6 +5455,8 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
54555455
break;
54565456

54575457
case BBJ_COND:
5458+
bPrev->SetFalseTarget(block->Next());
5459+
54585460
/* Check if both sides of the BBJ_COND now jump to the same block */
54595461
if (bPrev->TrueTargetIs(bPrev->GetFalseTarget()))
54605462
{
@@ -5514,7 +5516,7 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block)
55145516
// fgConnectFallThrough: fix flow from a block that previously had a fall through
55155517
//
55165518
// Arguments:
5517-
// bSrc - source of fall through (may be null?)
5519+
// bSrc - source of fall through
55185520
// bDst - target of fall through
55195521
//
55205522
// Returns:
@@ -5523,87 +5525,77 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block)
55235525
//
55245526
BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst)
55255527
{
5528+
assert(bSrc != nullptr);
55265529
assert(fgPredsComputed);
55275530
BasicBlock* jmpBlk = nullptr;
55285531

5529-
/* If bSrc is non-NULL */
5532+
/* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */
55305533

5531-
if (bSrc != nullptr)
5534+
if (bSrc->KindIs(BBJ_COND) && !bSrc->NextIs(bDst))
55325535
{
5533-
/* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */
5534-
5535-
if (bSrc->bbFallsThrough() && !bSrc->NextIs(bDst))
5536-
{
5537-
switch (bSrc->GetKind())
5538-
{
5539-
case BBJ_CALLFINALLY:
5540-
case BBJ_COND:
5536+
// Add a new block after bSrc which jumps to 'bDst'
5537+
jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst);
5538+
bSrc->SetFalseTarget(jmpBlk);
5539+
fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc));
55415540

5542-
// Add a new block after bSrc which jumps to 'bDst'
5543-
jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst);
5544-
fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc));
5541+
// Record the loop number in the new block
5542+
jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum;
55455543

5546-
// Record the loop number in the new block
5547-
jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum;
5548-
5549-
// When adding a new jmpBlk we will set the bbWeight and bbFlags
5550-
//
5551-
if (fgHaveValidEdgeWeights && fgHaveProfileWeights())
5552-
{
5553-
FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc);
5554-
5555-
jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2;
5556-
if (bSrc->bbWeight == BB_ZERO_WEIGHT)
5557-
{
5558-
jmpBlk->bbWeight = BB_ZERO_WEIGHT;
5559-
}
5560-
5561-
if (jmpBlk->bbWeight == BB_ZERO_WEIGHT)
5562-
{
5563-
jmpBlk->SetFlags(BBF_RUN_RARELY);
5564-
}
5565-
5566-
weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin());
5567-
weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst);
5568-
//
5569-
// If the [min/max] values for our edge weight is within the slop factor
5570-
// then we will set the BBF_PROF_WEIGHT flag for the block
5571-
//
5572-
if (weightDiff <= slop)
5573-
{
5574-
jmpBlk->SetFlags(BBF_PROF_WEIGHT);
5575-
}
5576-
}
5577-
else
5578-
{
5579-
// We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight
5580-
if (bSrc->bbWeight < bDst->bbWeight)
5581-
{
5582-
jmpBlk->bbWeight = bSrc->bbWeight;
5583-
jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY);
5584-
}
5585-
else
5586-
{
5587-
jmpBlk->bbWeight = bDst->bbWeight;
5588-
jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY);
5589-
}
5590-
}
5544+
// When adding a new jmpBlk we will set the bbWeight and bbFlags
5545+
//
5546+
if (fgHaveValidEdgeWeights && fgHaveProfileWeights())
5547+
{
5548+
FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc);
55915549

5592-
fgReplacePred(bDst, bSrc, jmpBlk);
5550+
jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2;
5551+
if (bSrc->bbWeight == BB_ZERO_WEIGHT)
5552+
{
5553+
jmpBlk->bbWeight = BB_ZERO_WEIGHT;
5554+
}
55935555

5594-
JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n",
5595-
jmpBlk->GetTarget()->bbNum, bSrc->bbNum);
5596-
break;
5556+
if (jmpBlk->bbWeight == BB_ZERO_WEIGHT)
5557+
{
5558+
jmpBlk->SetFlags(BBF_RUN_RARELY);
5559+
}
55975560

5598-
default:
5599-
noway_assert(!"Unexpected bbKind");
5600-
break;
5561+
weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin());
5562+
weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst);
5563+
//
5564+
// If the [min/max] values for our edge weight is within the slop factor
5565+
// then we will set the BBF_PROF_WEIGHT flag for the block
5566+
//
5567+
if (weightDiff <= slop)
5568+
{
5569+
jmpBlk->SetFlags(BBF_PROF_WEIGHT);
56015570
}
56025571
}
5603-
else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext())
5572+
else
56045573
{
5605-
bSrc->SetFlags(BBF_NONE_QUIRK);
5574+
// We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight
5575+
if (bSrc->bbWeight < bDst->bbWeight)
5576+
{
5577+
jmpBlk->bbWeight = bSrc->bbWeight;
5578+
jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY);
5579+
}
5580+
else
5581+
{
5582+
jmpBlk->bbWeight = bDst->bbWeight;
5583+
jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY);
5584+
}
56065585
}
5586+
5587+
fgReplacePred(bDst, bSrc, jmpBlk);
5588+
5589+
JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n", jmpBlk->GetTarget()->bbNum,
5590+
bSrc->bbNum);
5591+
}
5592+
else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext())
5593+
{
5594+
bSrc->SetFlags(BBF_NONE_QUIRK);
5595+
}
5596+
else if (bSrc->KindIs(BBJ_COND) && bSrc->NextIs(bDst))
5597+
{
5598+
bSrc->SetFalseTarget(bDst);
56075599
}
56085600

56095601
return jmpBlk;

‎src/coreclr/jit/fgehopt.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2118,6 +2118,7 @@ void Compiler::fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock,
21182118
assert(predBlock->FalseTargetIs(nonCanonicalBlock));
21192119

21202120
BasicBlock* const newBlock = fgNewBBafter(BBJ_ALWAYS, predBlock, true, canonicalBlock);
2121+
predBlock->SetFalseTarget(newBlock);
21212122

21222123
JITDUMP("*** " FMT_BB " now falling through to empty " FMT_BB " and then to " FMT_BB "\n", predBlock->bbNum,
21232124
newBlock->bbNum, canonicalBlock->bbNum);

‎src/coreclr/jit/fgopt.cpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3703,17 +3703,17 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock*
37033703
fgInsertStmtAtEnd(block, cloneStmt);
37043704
}
37053705

3706+
// add an unconditional block after this block to jump to the target block's fallthrough block
3707+
//
3708+
assert(!target->IsLast());
3709+
BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget());
3710+
37063711
// Fix up block's flow
37073712
//
37083713
block->SetCond(target->GetTrueTarget());
37093714
fgAddRefPred(block->GetTrueTarget(), block);
37103715
fgRemoveRefPred(target, block);
37113716

3712-
// add an unconditional block after this block to jump to the target block's fallthrough block
3713-
//
3714-
assert(!target->IsLast());
3715-
BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget());
3716-
37173717
// The new block 'next' will inherit its weight from 'block'
37183718
//
37193719
next->inheritWeight(block);
@@ -4118,7 +4118,6 @@ bool Compiler::fgOptimizeBranch(BasicBlock* bJump)
41184118
bJump->CopyFlags(bDest, BBF_COPY_PROPAGATE);
41194119

41204120
bJump->SetCond(bDestNormalTarget);
4121-
bJump->SetFalseTarget(bJump->Next());
41224121

41234122
/* Update bbRefs and bbPreds */
41244123

@@ -6199,6 +6198,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase)
61996198
if (bDest->KindIs(BBJ_COND))
62006199
{
62016200
BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true, bDestNext);
6201+
bDest->SetFalseTarget(bFixup);
62026202
bFixup->inheritWeight(bDestNext);
62036203

62046204
fgRemoveRefPred(bDestNext, bDest);
@@ -6234,6 +6234,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase)
62346234

62356235
// Optimize the Conditional JUMP to go to the new target
62366236
block->SetTrueTarget(bNext->GetTarget());
6237+
block->SetFalseTarget(bNext->Next());
62376238

62386239
fgAddRefPred(bNext->GetTarget(), block, fgRemoveRefPred(bNext->GetTarget(), bNext));
62396240

‎src/coreclr/jit/flowgraph.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,10 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
270270
BasicBlock* top = block;
271271
unsigned char lpIndexFallThrough = BasicBlock::NOT_IN_LOOP;
272272

273-
if (top->KindIs(BBJ_COND))
273+
BBKinds oldJumpKind = top->GetKind();
274+
unsigned char lpIndex = top->bbNatLoopNum;
275+
276+
if (oldJumpKind == BBJ_COND)
274277
{
275278
lpIndexFallThrough = top->GetFalseTarget()->bbNatLoopNum;
276279
}
@@ -283,9 +286,6 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
283286

284287
bottom->TransferTarget(top);
285288

286-
BBKinds oldJumpKind = top->GetKind();
287-
unsigned char lpIndex = top->bbNatLoopNum;
288-
289289
// Update block flags
290290
const BasicBlockFlags originalFlags = top->GetFlagsRaw() | BBF_GC_SAFE_POINT;
291291

‎src/coreclr/jit/helperexpansion.cpp‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
286286
// Fallback basic block
287287
GenTree* fallbackValueDef = gtNewStoreLclVarNode(rtLookupLcl->GetLclNum(), call);
288288
BasicBlock* fallbackBb =
289-
fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fallbackValueDef, debugInfo, nullcheckBb->GetFalseTarget(), true);
289+
fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fallbackValueDef, debugInfo, nullcheckBb->Next(), true);
290290

291291
assert(fallbackBb->JumpsToNext());
292292
fallbackBb->SetFlags(BBF_NONE_QUIRK);
@@ -298,6 +298,9 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
298298
GenTree* fastpathValueDef = gtNewStoreLclVarNode(rtLookupLcl->GetLclNum(), fastPathValueClone);
299299
BasicBlock* fastPathBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fastpathValueDef, debugInfo, block);
300300

301+
// Set nullcheckBb's false jump target
302+
nullcheckBb->SetFalseTarget(fastPathBb);
303+
301304
BasicBlock* sizeCheckBb = nullptr;
302305
if (needsSizeCheck)
303306
{
@@ -339,6 +342,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
339342
GenTree* jtrue = gtNewOperNode(GT_JTRUE, TYP_VOID, sizeCheck);
340343
// sizeCheckBb fails - jump to fallbackBb
341344
sizeCheckBb = fgNewBBFromTreeAfter(BBJ_COND, prevBb, jtrue, debugInfo, fallbackBb);
345+
sizeCheckBb->SetFalseTarget(nullcheckBb);
342346
}
343347

344348
//
@@ -780,11 +784,13 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement*
780784
gtNewStoreLclVarNode(threadStaticBlockLclNum, gtCloneExpr(threadStaticBlockBaseLclValueUse));
781785
BasicBlock* fastPathBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, fallbackBb, fastPathValueDef, debugInfo, block, true);
782786

783-
// Set maxThreadStaticBlocksCondBB's true jump target
787+
// Set maxThreadStaticBlocksCondBB's jump targets
784788
maxThreadStaticBlocksCondBB->SetTrueTarget(fallbackBb);
789+
maxThreadStaticBlocksCondBB->SetFalseTarget(threadStaticBlockNullCondBB);
785790

786-
// Set threadStaticBlockNullCondBB's true jump target
791+
// Set threadStaticBlockNullCondBB's jump targets
787792
threadStaticBlockNullCondBB->SetTrueTarget(fastPathBb);
793+
threadStaticBlockNullCondBB->SetFalseTarget(fallbackBb);
788794

789795
//
790796
// Update preds in all new blocks
@@ -1095,8 +1101,7 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G
10951101
// Fallback basic block
10961102
// TODO-CQ: for JIT we can replace the original call with CORINFO_HELP_INITCLASS
10971103
// that only accepts a single argument
1098-
BasicBlock* helperCallBb =
1099-
fgNewBBFromTreeAfter(BBJ_ALWAYS, isInitedBb, call, debugInfo, isInitedBb->GetFalseTarget(), true);
1104+
BasicBlock* helperCallBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, isInitedBb, call, debugInfo, isInitedBb->Next(), true);
11001105
assert(helperCallBb->JumpsToNext());
11011106
helperCallBb->SetFlags(BBF_NONE_QUIRK);
11021107

@@ -1172,6 +1177,7 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G
11721177
fgAddRefPred(isInitedBb, prevBb);
11731178

11741179
// Both fastPathBb and helperCallBb have a single common pred - isInitedBb
1180+
isInitedBb->SetFalseTarget(helperCallBb);
11751181
fgAddRefPred(helperCallBb, isInitedBb);
11761182

11771183
//
@@ -1451,7 +1457,7 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,
14511457
// In theory, we could just emit the const U8 data to the data section and use GT_BLK here
14521458
// but that would be a bit less efficient since we would have to load the data from memory.
14531459
//
1454-
BasicBlock* fastpathBb = fgNewBBafter(BBJ_ALWAYS, lengthCheckBb, true, lengthCheckBb->GetFalseTarget());
1460+
BasicBlock* fastpathBb = fgNewBBafter(BBJ_ALWAYS, lengthCheckBb, true, lengthCheckBb->Next());
14551461
assert(fastpathBb->JumpsToNext());
14561462
fastpathBb->SetFlags(BBF_INTERNAL | BBF_NONE_QUIRK);
14571463

@@ -1514,6 +1520,7 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,
15141520
assert(prevBb->JumpsToNext());
15151521
fgAddRefPred(lengthCheckBb, prevBb);
15161522
// lengthCheckBb has two successors: block and fastpathBb
1523+
lengthCheckBb->SetFalseTarget(fastpathBb);
15171524
fgAddRefPred(fastpathBb, lengthCheckBb);
15181525
fgAddRefPred(block, lengthCheckBb);
15191526
// fastpathBb flows into block

0 commit comments

Comments
 (0)