Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Set bbFalseTarget upon BBJ_COND initialization/modification #96265

Merged
merged 8 commits into from
Jan 1, 2024
Merged

JIT: Set bbFalseTarget upon BBJ_COND initialization/modification #96265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
540ebac
Move bbFalseTarget update from SetNext to fgAddRefPred
amanasifkhalid Dec 21, 2023
bc3f3db
Remove SetFalseTarget logic in fgAddRefPred
amanasifkhalid Dec 21, 2023
4b282a4
Merge from main
amanasifkhalid Dec 22, 2023
98f2cb7
Fix assert on arm
amanasifkhalid Dec 22, 2023
77c9235
Small cleanup
amanasifkhalid Dec 22, 2023
6a755ff
Remove assert from fgAddRefPred
amanasifkhalid Dec 22, 2023
46894c9
Clean up fgConnectFallThrough
amanasifkhalid Dec 22, 2023
c0e065e
More fgConnectFallThrough cleanup
amanasifkhalid Dec 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/jit/block.cpp
View file
Open in desktop
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
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function take a falseTarget parameter that is used to set bbFalseTarget? Then, callers would be required to set it. Maybe later on when we remove bbNext as fall through?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me. I'll open a follow-up PR with that refactor; it might help others avoid hitting asserts around bbFalseTarget by forcing them to set it when initializing bbTrueTarget.

Thank you for the review!

}

// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading