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

Jump to bottom

JIT: Add explicit block fallthrough successor #93772

Closed
wants to merge 3 commits into from
Closed

JIT: Add explicit block fallthrough successor #93772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f841d28
Add explicit block fallthrough successor
amanasifkhalid Oct 20, 2023
9acad86
Fix assert
amanasifkhalid Oct 20, 2023
a1ce47e
[skip ci] Update SetFallThroughSucc comment
amanasifkhalid Oct 20, 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/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5297,7 +5297,7 @@ class AssertionPropFlowCallback
{
// Scenario where next block and conditional block, both point to the same block.
// In such case, intersect the assertions present on both the out edges of predBlock.
assert(predBlock->NextIs(block));
assert(predBlock->FallsInto(block));
BitVecOps::IntersectionD(apTraits, pAssertionOut, predBlock->bbAssertionOut);

if (VerboseDataflow())
Expand Down
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 @@ -1278,7 +1278,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i, Compiler* comp)
return bbJumpDest;

case BBJ_NONE:
return bbNext;
return GetFallThroughSucc();
Comment on lines 1280 to +1281
Copy link
Contributor

@SingleAccretion SingleAccretion Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here and in a number of other places in this change, the pattern seems to be to change: BBJ_NONE -> bbNext / BBJ_COND -> bbNext+bbJumpDest to BBJ_NONE -> GetFallThroughSucc() / BBJ_COND -> GetFallThroughSucc()+bbJumpDest. The BBJ_COND part makes sense, but the BBJ_NONE changes confuse me.

If GetFallThroughSucc() is the 'normal' successor of BBJ_COND, why touch BBJ_NONE?

Side note: I think you will also want to update VisitAllSuccs/VisitRegularSuccs.

Copy link
Member Author

@amanasifkhalid amanasifkhalid Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what @AndyAyersMS's plan is here, but a "lazy" approach to reordering the block layout might be moving a block without considering it might be a BBJ_NONE and its successor is its old bbNext, and just converting the BBJ_NONE to BBJ_ALWAYS after the reorder. If we do something like this, I think it would be nice to always use GetFallThroughSucc for BBJ_NONE blocks so we can assert the fall-through successor matches bbNext; if we mess something up during block reordering, hopefully this will help us catch it.

Side note: I think you will also want to update VisitAllSuccs/VisitRegularSuccs.

Thanks for catching that. I'll fix those.

Copy link
Contributor

@SingleAccretion SingleAccretion Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel there is some overloading of meanings for "the fall-through successor" here.

The change at large is needed because in the refactored flowgraph, the "false" successor of BBJ_COND (and possibly the BBJ_ALWAYS part of callfinally pairs) will be distinct from bbNext. So, overall, the imported blocks will change as follows:

  1. BBJ_NONE -> bbNext -> BBJ_ALWAYS -> bbJumpDest (can be reordered arbitrarily later on).
  2. BBJ_COND -> bbNext, bbJumpDest -> BBJ_COND -> bbTheNewFieldBeingAdded, bbJumpDest (same).

In other words, "fall through" will not exist as an explicit flowgraph concept like it does today (perhaps until after a very late layout stage, where BBJ_NONE may appear as an optional optimization).

In that light, I would expect changes to basically replace case BBJ_COND: if (<false branch>) return bbNext; with case BBJ_COND: if (<false branch>) return bbTheNewFieldBeingAdded;, and indeed they do in some places like switch recognition, but not in others.

Copy link
Member

Choose a reason for hiding this comment

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

My initial plan was to disallow BBJ_NONE until we'd set block layout. I think we'll have to chip away at this as we have a lot of code that needs to work both before and after we do layout.

So a plausible starting point is to allow BBJ_NONE but treat it somewhat like BBJ_ALWAYS, meaning it has to explicitly name its flow successor, and find some way to start incrementally removing the early BBJ_NONE cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if simply not producing BBJ_NONE (anywhere), instead producing BBJ_ALWAYS, and adding a small peephole to codegen to not emit "branch to next block" would be a faster/easier way to go about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if simply not producing BBJ_NONE (anywhere), instead producing BBJ_ALWAYS, and adding a small peephole to codegen to not emit "branch to next block" would be a faster/easier way to go about this.

With this approach, maybe we can get rid of bbFallThroughSucc and instead add a new member specific to BBJ_COND for its false branch target.


case BBJ_COND:
if (i == 0)
Expand Down
39 changes: 37 additions & 2 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,8 @@ struct BasicBlock : private LIR::Range
BBehfDesc* bbJumpEhf; // BBJ_EHFINALLYRET descriptor
};

BasicBlock* bbFallThroughSucc;

public:
#ifdef DEBUG
// When creating a block with a jump, we require its jump kind and target be initialized simultaneously.
Expand Down Expand Up @@ -581,7 +583,8 @@ struct BasicBlock : private LIR::Range

void SetNext(BasicBlock* next)
{
bbNext = next;
bbNext = next;
bbFallThroughSucc = bbNext;
if (next)
{
next->bbPrev = this;
Expand Down Expand Up @@ -666,6 +669,38 @@ struct BasicBlock : private LIR::Range
SetJumpKindAndTarget(jumpKind, jumpDest DEBUG_ARG(compiler));
}

BasicBlock* GetFallThroughSucc() const
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a confusing name. The fall-through successor is always going to be the next block.

What may not be is the "false" branch of a BBJ_COND block / JTRUE node. Is this meant to model that, or...?

Copy link
Member Author

@amanasifkhalid amanasifkhalid Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, in the case of BBJ_COND blocks, this would be the destination of the false branch. I wanted this name to capture the idea that, in the case of BBJ_COND blocks, bbFallThroughSucc will be taken if bbJumpDest isn't, and that this fall-through successor isn't necessarily the next block (which means the control flow doesn't actually "fall through"). Do you have any suggestions for the name @SingleAccretion? I don't want it to sound specific to BBJ_COND since we might decide to allow BBJ_CALLFINALLY/BBJ_ALWAYS pairs to be non-contiguous later, thus making bbFallThroughSucc relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest GetNormalJumpDest() for symmetry with GetJumpDest(). It matches the expected codegen for BBJ_COND:

jcc <cond jump dest>
jmp <normal jump dest>

I wouldn't worry too much about the callfinally cases. Callfinally cases are inherently complex, one has to look up how they work specifically. Even if you make the BBJ_ALWAYS part of the pair non-contiguous with the callfinally, you'll still not have it as a "successor" in the flowgraph sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks for the suggestion! I'll change it.

{
assert(bbFallsThrough());
// For now, bbFallThroughSucc cannot diverge from bbNext
assert(FallsIntoNext());
return bbFallThroughSucc;
}

void SetFallThroughSucc(BasicBlock* fallThroughSucc)
{
// TODO: Once we allow bbFallThroughSucc to diverge from bbNext, ensure we only set bbFallThroughSucc
// to something other than bbNext when this block is a BBJ_COND.
// For BBJ_CALLFINALLY blocks part of call-always pairs, bbFallThroughSucc points to the BBJ_ALWAYS block.
// For now, call-always pairs are always contiguous, so bbFallThroughSucc cannot diverge from bbNext.
assert(KindIs(BBJ_COND));
bbFallThroughSucc = fallThroughSucc;
// For now, bbFallThroughSucc cannot diverge from bbNext
assert(FallsIntoNext());
}

bool FallsInto(const BasicBlock* dest) const
{
assert(bbFallsThrough());
return (bbFallThroughSucc == dest);
}

bool FallsIntoNext() const
{
assert(bbFallsThrough());
return (bbFallThroughSucc == bbNext);
}

bool HasJump() const
{
return (bbJumpDest != nullptr);
Expand All @@ -681,7 +716,7 @@ struct BasicBlock : private LIR::Range
bool JumpsToNext() const
{
assert(HasJump());
return (bbJumpDest == bbNext);
return (bbJumpDest == bbFallThroughSucc);
}

BBswtDesc* GetJumpSwt() const
Expand Down
15 changes: 7 additions & 8 deletions src/coreclr/jit/codegenarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2176,6 +2176,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}
else
{
assert(block->isBBCallAlwaysPair());

// Because of the way the flowgraph is connected, the liveness info for this one instruction
// after the call is not (can not be) correct in cases where a variable has a last use in the
// handler. So turn off GC reporting for this single instruction.
Expand All @@ -2198,17 +2200,14 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}

GetEmitter()->emitEnableGC();
}

// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
if (!(block->bbFlags & BBF_RETLESS_CALL))
{
assert(block->isBBCallAlwaysPair());
// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
block = nextBlock;
}

return block;
}

Expand Down
15 changes: 7 additions & 8 deletions src/coreclr/jit/codegenloongarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,6 +1536,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}
else
{
assert(block->isBBCallAlwaysPair());

// Because of the way the flowgraph is connected, the liveness info for this one instruction
// after the call is not (can not be) correct in cases where a variable has a last use in the
// handler. So turn off GC reporting for this single instruction.
Expand All @@ -1558,17 +1560,14 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}

GetEmitter()->emitEnableGC();
}

// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
if (!(block->bbFlags & BBF_RETLESS_CALL))
{
assert(block->isBBCallAlwaysPair());
// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
block = nextBlock;
}

return block;
}

Expand Down
15 changes: 7 additions & 8 deletions src/coreclr/jit/codegenriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1174,6 +1174,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}
else
{
assert(block->isBBCallAlwaysPair());

// Because of the way the flowgraph is connected, the liveness info for this one instruction
// after the call is not (can not be) correct in cases where a variable has a last use in the
// handler. So turn off GC reporting for this single instruction.
Expand All @@ -1196,17 +1198,14 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}

GetEmitter()->emitEnableGC();
}

// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
if (!(block->bbFlags & BBF_RETLESS_CALL))
{
assert(block->isBBCallAlwaysPair());
// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
block = nextBlock;
}

return block;
}

Expand Down
40 changes: 20 additions & 20 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2958,7 +2958,7 @@ void Compiler::fgLinkBasicBlocks()
FALLTHROUGH;

case BBJ_NONE:
fgAddRefPred<initializingPreds>(curBBdesc->Next(), curBBdesc, oldEdge);
fgAddRefPred<initializingPreds>(curBBdesc->GetFallThroughSucc(), curBBdesc, oldEdge);
break;

case BBJ_EHFILTERRET:
Expand Down Expand Up @@ -4180,7 +4180,7 @@ void Compiler::fgFixEntryFlowForOSR()
//
fgEnsureFirstBBisScratch();
assert(fgFirstBB->KindIs(BBJ_NONE));
fgRemoveRefPred(fgFirstBB->Next(), fgFirstBB);
fgRemoveRefPred(fgFirstBB->GetFallThroughSucc(), fgFirstBB);
fgFirstBB->SetJumpKindAndTarget(BBJ_ALWAYS, fgOSREntryBB DEBUG_ARG(this));
FlowEdge* const edge = fgAddRefPred(fgOSREntryBB, fgFirstBB);
edge->setLikelihood(1.0);
Expand Down Expand Up @@ -4224,7 +4224,7 @@ void Compiler::fgCheckBasicBlockControlFlow()
{
case BBJ_NONE: // block flows into the next one (no jump)

fgControlFlowPermitted(blk, blk->Next());
fgControlFlowPermitted(blk, blk->GetFallThroughSucc());

break;

Expand All @@ -4236,7 +4236,7 @@ void Compiler::fgCheckBasicBlockControlFlow()

case BBJ_COND: // block conditionally jumps to the target

fgControlFlowPermitted(blk, blk->Next());
fgControlFlowPermitted(blk, blk->GetFallThroughSucc());

fgControlFlowPermitted(blk, blk->GetJumpDest());

Expand Down Expand Up @@ -5042,6 +5042,10 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)
curr->SetJumpDest(newBlock);
}
fgAddRefPred(newBlock, curr);

// This isn't accurate, but it is complex to compute a reasonable number so just assume that we take the
// branch 50% of the time.
newBlock->inheritWeightPercentage(curr, 50);
}
else if (curr->KindIs(BBJ_SWITCH))
{
Expand All @@ -5050,6 +5054,10 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)

// And 'succ' has 'newBlock' as a new predecessor.
fgAddRefPred(succ, newBlock);

// This isn't accurate, but it is complex to compute a reasonable number so just assume that we take the
// branch 50% of the time.
newBlock->inheritWeightPercentage(curr, 50);
}
else
{
Expand All @@ -5063,14 +5071,6 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)
fgFixFinallyTargetFlags(curr, succ, newBlock);
#endif

// This isn't accurate, but it is complex to compute a reasonable number so just assume that we take the
// branch 50% of the time.
//
if (!curr->KindIs(BBJ_ALWAYS))
{
newBlock->inheritWeightPercentage(curr, 50);
}

// The bbLiveIn and bbLiveOut are both equal to the bbLiveIn of 'succ'
if (fgLocalVarLivenessDone)
{
Expand Down Expand Up @@ -5218,7 +5218,7 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
NO_WAY("No retless call finally blocks; need unwind target instead");
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
}
else if (bPrev->KindIs(BBJ_ALWAYS) && block->NextIs(bPrev->GetJumpDest()) &&
else if (bPrev->KindIs(BBJ_ALWAYS) && bPrev->HasJumpTo(block->Next()) &&
!(bPrev->bbFlags & BBF_KEEP_BBJ_ALWAYS) && !block->IsFirstColdBlock(this) &&
!block->IsLastHotBlock(this))
{
Expand Down Expand Up @@ -5246,7 +5246,7 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
*/
if (block->isBBCallAlwaysPair())
{
BasicBlock* leaveBlk = block->Next();
BasicBlock* leaveBlk = block->GetFallThroughSucc();
noway_assert(leaveBlk->KindIs(BBJ_ALWAYS));

leaveBlk->bbFlags &= ~BBF_DONT_REMOVE;
Expand Down Expand Up @@ -5325,7 +5325,7 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
}
else
{
succBlock = block->Next();
succBlock = block->GetFallThroughSucc();
}

bool skipUnmarkLoop = false;
Expand Down Expand Up @@ -5437,7 +5437,7 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
}

/* Check if both side of the BBJ_COND now jump to the same block */
if (predBlock->NextIs(succBlock))
if (predBlock->FallsInto(succBlock))
{
// Make sure we are replacing "block" with "succBlock" in predBlock->bbJumpDest.
noway_assert(predBlock->HasJumpTo(block));
Expand Down Expand Up @@ -5502,7 +5502,7 @@ void Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)

case BBJ_COND:
/* Check for branch to next block */
if (bPrev->JumpsToNext())
if (bPrev->HasJumpTo(bPrev->GetFallThroughSucc()))
{
fgRemoveConditionalJump(bPrev);
}
Expand Down Expand Up @@ -5538,7 +5538,7 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* 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))
if (bSrc->bbFallsThrough() && !bSrc->FallsInto(bDst))
{
switch (bSrc->GetJumpKind())
{
Expand Down Expand Up @@ -5622,7 +5622,7 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst)
bSrc->SetJumpKindAndTarget(BBJ_NONE DEBUG_ARG(this));
JITDUMP("Changed an unconditional jump from " FMT_BB " to the next block " FMT_BB
" into a BBJ_NONE block\n",
bSrc->bbNum, bSrc->Next()->bbNum);
bSrc->bbNum, bSrc->GetFallThroughSucc()->bbNum);
}
}
}
Expand Down Expand Up @@ -6413,7 +6413,7 @@ bool Compiler::fgIsBetterFallThrough(BasicBlock* bCur, BasicBlock* bAlt)
}

// Currently bNext is the fall through for bCur
BasicBlock* bNext = bCur->Next();
BasicBlock* bNext = bCur->GetFallThroughSucc();
noway_assert(bNext != nullptr);

// We will set result to true if bAlt is a better fall through than bCur
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/jit/fgdiagnostic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2697,11 +2697,12 @@ bool BBPredsChecker::CheckJump(BasicBlock* blockPred, BasicBlock* block)
switch (blockPred->GetJumpKind())
{
case BBJ_COND:
assert(blockPred->NextIs(block) || blockPred->HasJumpTo(block));
assert(blockPred->FallsInto(block) || blockPred->HasJumpTo(block));
return true;

case BBJ_NONE:
assert(blockPred->NextIs(block));
assert(blockPred->FallsInto(block));
assert(blockPred->FallsIntoNext());
return true;

case BBJ_CALLFINALLY:
Expand Down Expand Up @@ -4871,7 +4872,7 @@ void Compiler::fgDebugCheckLoopTable()
else
{
assert(h->KindIs(BBJ_NONE));
assert(h->NextIs(e));
assert(h->FallsInto(e));
assert(loop.lpTop == e);
assert(loop.lpIsTopEntry());
}
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/jit/fgehopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ PhaseStatus Compiler::fgCloneFinally()

if (block->KindIs(BBJ_NONE) && (block == lastTryBlock))
{
jumpDest = block->Next();
jumpDest = block->GetFallThroughSucc();
}
else if (block->KindIs(BBJ_ALWAYS))
{
Expand Down Expand Up @@ -1032,7 +1032,7 @@ PhaseStatus Compiler::fgCloneFinally()

// If the clone ends up just after the finally, adjust
// the stopping point for finally traversal.
if (newBlock->NextIs(nextBlock))
if (newBlock->FallsInto(nextBlock))
{
assert(newBlock->PrevIs(lastBlock));
nextBlock = newBlock;
Expand Down Expand Up @@ -2178,7 +2178,7 @@ PhaseStatus Compiler::fgTailMergeThrows()
case BBJ_COND:
{
// Flow to non canonical block could be via fall through or jump or both.
if (predBlock->NextIs(nonCanonicalBlock))
if (predBlock->FallsInto(nonCanonicalBlock))
{
fgTailMergeThrowsFallThroughHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge);
}
Expand Down Expand Up @@ -2254,7 +2254,8 @@ void Compiler::fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock,
BasicBlock* canonicalBlock,
FlowEdge* predEdge)
{
assert(predBlock->NextIs(nonCanonicalBlock));
assert(predBlock->bbFallsThrough());
assert(predBlock->FallsInto(nonCanonicalBlock));

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

Expand Down
Loading