Skip to content

Commit

Permalink
JIT: Set bbFalseTarget upon BBJ_COND initialization/modification (#…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@amanasifkhalid
amanasifkhalid authored Jan 1, 2024
1 parent 9057843 commit 25efee0
Show file tree
Hide file tree
Showing 13 changed files with 130 additions and 120 deletions.
2 changes: 1 addition & 1 deletion src/coreclr/jit/block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1123,7 +1123,7 @@ bool BasicBlock::bbFallsThrough() const
return false;

case BBJ_COND:
return NextIs(GetFalseTarget());
return true;

case BBJ_CALLFINALLY:
return !HasFlag(BBF_RETLESS_CALL);
Expand Down
12 changes: 3 additions & 9 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,12 +584,6 @@ struct BasicBlock : private LIR::Range
{
next->bbPrev = this;
}

// BBJ_COND convenience: This ensures bbFalseTarget is always consistent with bbNext.
// For now, if a BBJ_COND's bbTrueTarget is not taken, we expect to fall through,
// so bbFalseTarget must be the next block.
// TODO-NoFallThrough: Remove this once we allow bbFalseTarget to diverge from bbNext
bbFalseTarget = next;
}

bool IsFirst() const
Expand Down Expand Up @@ -703,9 +697,9 @@ struct BasicBlock : private LIR::Range
void SetCond(BasicBlock* trueTarget)
{
assert(trueTarget != nullptr);
bbKind = BBJ_COND;
bbTrueTarget = trueTarget;
// TODO-NoFallThrough: also set bbFalseTarget
bbKind = BBJ_COND;
bbTrueTarget = trueTarget;
bbFalseTarget = bbNext;
}

// Set both the block kind and target. This can clear `bbTarget` when setting
Expand Down
142 changes: 67 additions & 75 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4761,9 +4761,7 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
{
// We'd like to use fgNewBBafter(), but we need to update the preds list before linking in the new block.
// (We need the successors of 'curr' to be correct when we do this.)
BasicBlock* newBlock;

newBlock = BasicBlock::New(this);
BasicBlock* 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;
Expand All @@ -4789,10 +4787,6 @@ 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.
Expand Down Expand Up @@ -4821,6 +4815,11 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
// Remove flags from the old block that are no longer possible.
curr->RemoveFlags(BBF_HAS_JMP | BBF_RETLESS_CALL);

// Transfer the kind and target. Do this after the code above, to avoid null-ing out the old targets used by the
// above code (and so newBlock->bbNext is valid, so SetCond() can initialize bbFalseTarget if newBlock is a
// BBJ_COND).
newBlock->TransferTarget(curr);

// Default to fallthrough, and add the arc for that.
curr->SetKindAndTarget(BBJ_ALWAYS, newBlock);
curr->SetFlags(BBF_NONE_QUIRK);
Expand Down Expand Up @@ -5080,6 +5079,7 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)

if (curr->KindIs(BBJ_COND))
{
curr->SetFalseTarget(curr->Next());
fgReplacePred(succ, curr, newBlock);
if (curr->TrueTargetIs(succ))
{
Expand Down Expand Up @@ -5455,6 +5455,8 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
break;

case BBJ_COND:
bPrev->SetFalseTarget(block->Next());

/* Check if both sides of the BBJ_COND now jump to the same block */
if (bPrev->TrueTargetIs(bPrev->GetFalseTarget()))
{
Expand Down Expand Up @@ -5514,7 +5516,7 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block)
// fgConnectFallThrough: fix flow from a block that previously had a fall through
//
// Arguments:
// bSrc - source of fall through (may be null?)
// bSrc - source of fall through
// bDst - target of fall through
//
// Returns:
Expand All @@ -5523,87 +5525,77 @@ void Compiler::fgPrepareCallFinallyRetForRemoval(BasicBlock* block)
//
BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst)
{
assert(bSrc != nullptr);
assert(fgPredsComputed);
BasicBlock* jmpBlk = nullptr;

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

if (bSrc != nullptr)
if (bSrc->KindIs(BBJ_COND) && !bSrc->NextIs(bDst))
{
/* If bSrc falls through to a block that is not bDst, we will insert a jump to bDst */

if (bSrc->bbFallsThrough() && !bSrc->NextIs(bDst))
{
switch (bSrc->GetKind())
{
case BBJ_CALLFINALLY:
case BBJ_COND:
// Add a new block after bSrc which jumps to 'bDst'
jmpBlk = fgNewBBafter(BBJ_ALWAYS, bSrc, true, bDst);
bSrc->SetFalseTarget(jmpBlk);
fgAddRefPred(jmpBlk, bSrc, fgGetPredForBlock(bDst, bSrc));

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

// Record the loop number in the new block
jmpBlk->bbNatLoopNum = bSrc->bbNatLoopNum;

// When adding a new jmpBlk we will set the bbWeight and bbFlags
//
if (fgHaveValidEdgeWeights && fgHaveProfileWeights())
{
FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc);

jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2;
if (bSrc->bbWeight == BB_ZERO_WEIGHT)
{
jmpBlk->bbWeight = BB_ZERO_WEIGHT;
}

if (jmpBlk->bbWeight == BB_ZERO_WEIGHT)
{
jmpBlk->SetFlags(BBF_RUN_RARELY);
}

weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin());
weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst);
//
// If the [min/max] values for our edge weight is within the slop factor
// then we will set the BBF_PROF_WEIGHT flag for the block
//
if (weightDiff <= slop)
{
jmpBlk->SetFlags(BBF_PROF_WEIGHT);
}
}
else
{
// We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight
if (bSrc->bbWeight < bDst->bbWeight)
{
jmpBlk->bbWeight = bSrc->bbWeight;
jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY);
}
else
{
jmpBlk->bbWeight = bDst->bbWeight;
jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY);
}
}
// When adding a new jmpBlk we will set the bbWeight and bbFlags
//
if (fgHaveValidEdgeWeights && fgHaveProfileWeights())
{
FlowEdge* const newEdge = fgGetPredForBlock(jmpBlk, bSrc);

fgReplacePred(bDst, bSrc, jmpBlk);
jmpBlk->bbWeight = (newEdge->edgeWeightMin() + newEdge->edgeWeightMax()) / 2;
if (bSrc->bbWeight == BB_ZERO_WEIGHT)
{
jmpBlk->bbWeight = BB_ZERO_WEIGHT;
}

JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n",
jmpBlk->GetTarget()->bbNum, bSrc->bbNum);
break;
if (jmpBlk->bbWeight == BB_ZERO_WEIGHT)
{
jmpBlk->SetFlags(BBF_RUN_RARELY);
}

default:
noway_assert(!"Unexpected bbKind");
break;
weight_t weightDiff = (newEdge->edgeWeightMax() - newEdge->edgeWeightMin());
weight_t slop = BasicBlock::GetSlopFraction(bSrc, bDst);
//
// If the [min/max] values for our edge weight is within the slop factor
// then we will set the BBF_PROF_WEIGHT flag for the block
//
if (weightDiff <= slop)
{
jmpBlk->SetFlags(BBF_PROF_WEIGHT);
}
}
else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext())
else
{
bSrc->SetFlags(BBF_NONE_QUIRK);
// We set the bbWeight to the smaller of bSrc->bbWeight or bDst->bbWeight
if (bSrc->bbWeight < bDst->bbWeight)
{
jmpBlk->bbWeight = bSrc->bbWeight;
jmpBlk->CopyFlags(bSrc, BBF_RUN_RARELY);
}
else
{
jmpBlk->bbWeight = bDst->bbWeight;
jmpBlk->CopyFlags(bDst, BBF_RUN_RARELY);
}
}

fgReplacePred(bDst, bSrc, jmpBlk);

JITDUMP("Added an unconditional jump to " FMT_BB " after block " FMT_BB "\n", jmpBlk->GetTarget()->bbNum,
bSrc->bbNum);
}
else if (bSrc->KindIs(BBJ_ALWAYS) && bSrc->HasInitializedTarget() && bSrc->JumpsToNext())
{
bSrc->SetFlags(BBF_NONE_QUIRK);
}
else if (bSrc->KindIs(BBJ_COND) && bSrc->NextIs(bDst))
{
bSrc->SetFalseTarget(bDst);
}

return jmpBlk;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/fgehopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2118,6 +2118,7 @@ void Compiler::fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock,
assert(predBlock->FalseTargetIs(nonCanonicalBlock));

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

JITDUMP("*** " FMT_BB " now falling through to empty " FMT_BB " and then to " FMT_BB "\n", predBlock->bbNum,
newBlock->bbNum, canonicalBlock->bbNum);
Expand Down
13 changes: 7 additions & 6 deletions src/coreclr/jit/fgopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3703,17 +3703,17 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock*
fgInsertStmtAtEnd(block, cloneStmt);
}

// add an unconditional block after this block to jump to the target block's fallthrough block
//
assert(!target->IsLast());
BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget());

// Fix up block's flow
//
block->SetCond(target->GetTrueTarget());
fgAddRefPred(block->GetTrueTarget(), block);
fgRemoveRefPred(target, block);

// add an unconditional block after this block to jump to the target block's fallthrough block
//
assert(!target->IsLast());
BasicBlock* next = fgNewBBafter(BBJ_ALWAYS, block, true, target->GetFalseTarget());

// The new block 'next' will inherit its weight from 'block'
//
next->inheritWeight(block);
Expand Down Expand Up @@ -4118,7 +4118,6 @@ bool Compiler::fgOptimizeBranch(BasicBlock* bJump)
bJump->CopyFlags(bDest, BBF_COPY_PROPAGATE);

bJump->SetCond(bDestNormalTarget);
bJump->SetFalseTarget(bJump->Next());

/* Update bbRefs and bbPreds */

Expand Down Expand Up @@ -6199,6 +6198,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase)
if (bDest->KindIs(BBJ_COND))
{
BasicBlock* const bFixup = fgNewBBafter(BBJ_ALWAYS, bDest, true, bDestNext);
bDest->SetFalseTarget(bFixup);
bFixup->inheritWeight(bDestNext);

fgRemoveRefPred(bDestNext, bDest);
Expand Down Expand Up @@ -6234,6 +6234,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication, bool isPhase)

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

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

Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,10 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)
BasicBlock* top = block;
unsigned char lpIndexFallThrough = BasicBlock::NOT_IN_LOOP;

if (top->KindIs(BBJ_COND))
BBKinds oldJumpKind = top->GetKind();
unsigned char lpIndex = top->bbNatLoopNum;

if (oldJumpKind == BBJ_COND)
{
lpIndexFallThrough = top->GetFalseTarget()->bbNatLoopNum;
}
Expand All @@ -283,9 +286,6 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block)

bottom->TransferTarget(top);

BBKinds oldJumpKind = top->GetKind();
unsigned char lpIndex = top->bbNatLoopNum;

// Update block flags
const BasicBlockFlags originalFlags = top->GetFlagsRaw() | BBF_GC_SAFE_POINT;

Expand Down
19 changes: 13 additions & 6 deletions src/coreclr/jit/helperexpansion.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
// Fallback basic block
GenTree* fallbackValueDef = gtNewStoreLclVarNode(rtLookupLcl->GetLclNum(), call);
BasicBlock* fallbackBb =
fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fallbackValueDef, debugInfo, nullcheckBb->GetFalseTarget(), true);
fgNewBBFromTreeAfter(BBJ_ALWAYS, nullcheckBb, fallbackValueDef, debugInfo, nullcheckBb->Next(), true);

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

// Set nullcheckBb's false jump target
nullcheckBb->SetFalseTarget(fastPathBb);

BasicBlock* sizeCheckBb = nullptr;
if (needsSizeCheck)
{
Expand Down Expand Up @@ -339,6 +342,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
GenTree* jtrue = gtNewOperNode(GT_JTRUE, TYP_VOID, sizeCheck);
// sizeCheckBb fails - jump to fallbackBb
sizeCheckBb = fgNewBBFromTreeAfter(BBJ_COND, prevBb, jtrue, debugInfo, fallbackBb);
sizeCheckBb->SetFalseTarget(nullcheckBb);
}

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

// Set maxThreadStaticBlocksCondBB's true jump target
// Set maxThreadStaticBlocksCondBB's jump targets
maxThreadStaticBlocksCondBB->SetTrueTarget(fallbackBb);
maxThreadStaticBlocksCondBB->SetFalseTarget(threadStaticBlockNullCondBB);

// Set threadStaticBlockNullCondBB's true jump target
// Set threadStaticBlockNullCondBB's jump targets
threadStaticBlockNullCondBB->SetTrueTarget(fastPathBb);
threadStaticBlockNullCondBB->SetFalseTarget(fallbackBb);

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

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

// Both fastPathBb and helperCallBb have a single common pred - isInitedBb
isInitedBb->SetFalseTarget(helperCallBb);
fgAddRefPred(helperCallBb, isInitedBb);

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

Expand Down Expand Up @@ -1514,6 +1520,7 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,
assert(prevBb->JumpsToNext());
fgAddRefPred(lengthCheckBb, prevBb);
// lengthCheckBb has two successors: block and fastpathBb
lengthCheckBb->SetFalseTarget(fastpathBb);
fgAddRefPred(fastpathBb, lengthCheckBb);
fgAddRefPred(block, lengthCheckBb);
// fastpathBb flows into block
Expand Down
Loading

0 comments on commit 25efee0

Please sign in to comment.