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: Assert bbJumpKind in BasicBlock::HasJump #95152

Merged
merged 6 commits into from
Nov 27, 2023
Merged

JIT: Assert bbJumpKind in BasicBlock::HasJump #95152

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
71c3b2b
Assert bbJumpKind in BasicBlock::HasJump
amanasifkhalid Nov 23, 2023
68e982b
Rename to HasInitializedJumpDest
amanasifkhalid Nov 23, 2023
776b0e3
Fix merge conflict
amanasifkhalid Nov 23, 2023
ff9faad
Add HasJumpDest()
amanasifkhalid Nov 26, 2023
11c3366
Fix HasJumpDest
amanasifkhalid Nov 26, 2023
e037c3a
Fix merge conflict
amanasifkhalid Nov 26, 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
18 changes: 9 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 @@ -552,10 +552,10 @@ struct BasicBlock : private LIR::Range
void SetJumpKind(BBjumpKinds jumpKind)
{
// If this block's jump kind requires a target, ensure it is already set
assert(HasJump() || !KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE));
assert(!KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE) || HasInitializedJumpDest());
bbJumpKind = jumpKind;
// If new jump kind requires a target, ensure a target is already set
assert(HasJump() || !KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE));
assert(!KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE) || HasInitializedJumpDest());
}

BasicBlock* Prev() const
Expand Down Expand Up @@ -625,7 +625,7 @@ struct BasicBlock : private LIR::Range
BasicBlock* GetJumpDest() const
{
// If bbJumpKind indicates this block has a jump, bbJumpDest cannot be null
assert(HasJump() || !KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE));
assert(!KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE) || HasInitializedJumpDest());
return bbJumpDest;
}

Expand All @@ -634,7 +634,7 @@ struct BasicBlock : private LIR::Range
// SetJumpKindAndTarget() nulls jumpDest for non-jump kinds,
// so don't use SetJumpDest() to null bbJumpDest without updating bbJumpKind.
bbJumpDest = jumpDest;
assert(HasJump());
assert(!KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE) || HasInitializedJumpDest());
}

void SetJumpKindAndTarget(BBjumpKinds jumpKind, BasicBlock* jumpDest = nullptr)
Expand All @@ -643,24 +643,24 @@ struct BasicBlock : private LIR::Range
bbJumpDest = jumpDest;

// If bbJumpKind indicates this block has a jump, bbJumpDest cannot be null
assert(HasJump() || !KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE));
assert(!KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE) || HasInitializedJumpDest());
}

bool HasJump() const
bool HasInitializedJumpDest() const
{
assert(KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE));
return (bbJumpDest != nullptr);
}
Copy link
Member

@jakobbotsch jakobbotsch Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What is the point of HasJump()? It seems like a check for whether the block is in some half-constructed state, since I would expect the block kinds above always to have valid bbJumpDest. It does not really make sense for me to have this function with the assert that is added here.
An assert that would make more sense to me would be:

assert(KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE) == (bbJumpDest != nullptr))

or

assert(!KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE) || (bbJumpDest != nullptr));

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see that @BruceForstall's feedback in #94239 was about this being a union field.

I would like to understand when it is expected that these jump kinds have a null bbJumpDest. Maybe that happens only for a very short while when we're initializing the flow graph? In that case, consider renaming this function to HasInitializedJumpDest() or something of the sort, to make that clear. I think it is a bit confusing currently, as it makes it seem like an expected situation that some of these blocks do not have a valid bbJumpDest, which should not be possible.

Copy link
Member Author

@amanasifkhalid amanasifkhalid Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to understand when it is expected that these jump kinds have a null bbJumpDest. Maybe that happens only for a very short while when we're initializing the flow graph?

Yes, there are a few cases where we have to create a block that requires a jump target without knowing the target yet. For example, in Compiler::fgMakeBasicBlocks, we create blocks only knowing the offset of their jump targets, and set the actual target block later in Compiler::fgLinkBasicBlocks. The tougher cases are during importing, where we sometimes create a BBJ_ALWAYS or BBJ_CALLFINALLY without setting its jump target in a loop, and then create and set the jump target in a later iteration of the loop. There are a few other spots where we do this pattern, but I can't remember off the top of my head...

Your first suggested assert seems to best communicate what HasJump should do; for block kinds that should never read bbJumpDest, bbJumpDest should not be set, else bbJumpDest must be set (i.e. we shouldn't be in some half-constructed state) before performing any conditional logic based on whether the block has a jump or not.

Edit: Ah nevermind, I realize we check for this uninitialized block jump state in Compiler::impImportLeave in a few places, and I'd rather not touch the importer code in this PR to better fit the asserts in HasJump. Right now, we seem to only check HasJump for blocks that should have a jump target (eventually), so maybe renaming it to something like HasInitializedJumpDest makes more sense.


bool HasJumpTo(const BasicBlock* jumpDest) const
{
assert(HasJump());
assert(!KindIs(BBJ_SWITCH, BBJ_EHFINALLYRET));
assert(HasInitializedJumpDest());
return (bbJumpDest == jumpDest);
}

bool JumpsToNext() const
{
assert(HasJump());
assert(HasInitializedJumpDest());
return (bbJumpDest == bbNext);
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegenarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)

assert(!block->IsLast());
assert(block->Next()->KindIs(BBJ_ALWAYS));
assert(block->Next()->HasJump());
assert(block->Next()->HasInitializedJumpDest());
assert(block->Next()->GetJumpDest()->bbFlags & BBF_FINALLY_TARGET);

bbFinallyRet = block->Next()->GetJumpDest();
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2946,7 +2946,7 @@ void Compiler::fgLinkBasicBlocks()
{
BasicBlock* const jumpDest = fgLookupBB(curBBdesc->GetJumpOffs());
// Redundantly use SetJumpKindAndTarget() instead of SetJumpDest() just this once,
// so we don't break the HasJump() invariant of SetJumpDest().
// so we don't break the HasInitializedJumpDest() invariant of SetJumpDest().
curBBdesc->SetJumpKindAndTarget(curBBdesc->GetJumpKind(), jumpDest);
fgAddRefPred<initializingPreds>(jumpDest, curBBdesc, oldEdge);

Expand Down Expand Up @@ -5645,7 +5645,7 @@ BasicBlock* Compiler::fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst)
// (It's possible for bSrc's jump destination to not be set yet,
// so check with BasicBlock::HasJump before calling BasicBlock::JumpsToNext)
//
if (bSrc->KindIs(BBJ_ALWAYS) && !(bSrc->bbFlags & BBF_KEEP_BBJ_ALWAYS) && bSrc->HasJump() &&
if (bSrc->KindIs(BBJ_ALWAYS) && !(bSrc->bbFlags & BBF_KEEP_BBJ_ALWAYS) && bSrc->HasInitializedJumpDest() &&
bSrc->JumpsToNext())
{
bSrc->SetJumpKindAndTarget(BBJ_NONE);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/fgdiagnostic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3129,7 +3129,7 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef
// Blocks with these jump kinds must have non-null jump targets
if (block->KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE))
{
assert(block->HasJump());
assert(block->HasInitializedJumpDest());
}

// A branch or fall-through to a BBJ_CALLFINALLY block must come from the `try` region associated
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/fgehopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ PhaseStatus Compiler::fgRemoveEmptyTry()
// Time to optimize.
//
// (1) Convert the callfinally to a normal jump to the handler
assert(callFinally->HasJump());
assert(callFinally->HasInitializedJumpDest());
callFinally->SetJumpKind(BBJ_ALWAYS);

// Identify the leave block and the continuation
Expand Down Expand Up @@ -1081,7 +1081,7 @@ PhaseStatus Compiler::fgCloneFinally()
newBlock->clearHndIndex();

// Jump dests are set in a post-pass; make sure CloneBlockState hasn't tried to set them.
assert(!newBlock->HasJump());
assert(newBlock->KindIs(BBJ_NONE));
}

if (!clonedOk)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/fgprofile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3800,7 +3800,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf
{
assert(nSucc == 1);
assert(block == pseudoEdge->m_sourceBlock);
assert(block->HasJump());
assert(block->HasInitializedJumpDest());
FlowEdge* const flowEdge = m_comp->fgGetPredForBlock(block->GetJumpDest(), block);
assert(flowEdge != nullptr);
flowEdge->setLikelihood(1.0);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/ifconversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ bool OptIfConversionDsc::optIfConvert()

// Update the flow from the original block.
m_comp->fgRemoveAllRefPreds(m_startBlock->Next(), m_startBlock);
assert(m_startBlock->HasJump());
assert(m_startBlock->HasInitializedJumpDest());
m_startBlock->SetJumpKind(BBJ_ALWAYS);

#ifdef DEBUG
Expand Down
23 changes: 9 additions & 14 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4323,7 +4323,7 @@ void Compiler::impImportLeave(BasicBlock* block)
assert(step == DUMMY_INIT(NULL));
callBlock = block;

assert(callBlock->HasJump());
assert(callBlock->HasInitializedJumpDest());
fgRemoveRefPred(callBlock->GetJumpDest(), callBlock);

// callBlock will call the finally handler. Convert the BBJ_LEAVE to BBJ_CALLFINALLY
Expand Down Expand Up @@ -4353,7 +4353,7 @@ void Compiler::impImportLeave(BasicBlock* block)
callBlock = fgNewBBinRegion(BBJ_CALLFINALLY, XTnum + 1, 0, step, HBtab->ebdHndBeg);

// step's jump target shouldn't be set yet
assert(!step->HasJump());
assert(!step->HasInitializedJumpDest());

// the previous call to a finally returns to this call (to the next finally in the chain)
step->SetJumpDest(callBlock);
Expand Down Expand Up @@ -4453,7 +4453,7 @@ void Compiler::impImportLeave(BasicBlock* block)
finalStep->bbFlags |= BBF_KEEP_BBJ_ALWAYS;

// step's jump target shouldn't be set yet
assert(!step->HasJump());
assert(!step->HasInitializedJumpDest());

step->SetJumpDest(finalStep);
fgAddRefPred(finalStep, step);
Expand Down Expand Up @@ -4598,7 +4598,7 @@ void Compiler::impImportLeave(BasicBlock* block)
BasicBlock* exitBlock = fgNewBBinRegion(BBJ_EHCATCHRET, 0, XTnum + 1, step);

assert(step->KindIs(BBJ_ALWAYS, BBJ_EHCATCHRET));
assert((step == block) || !step->HasJump());
assert((step == block) || !step->HasInitializedJumpDest());
if (step == block)
{
fgRemoveRefPred(step->GetJumpDest(), step);
Expand Down Expand Up @@ -4678,7 +4678,7 @@ void Compiler::impImportLeave(BasicBlock* block)

callBlock = block;

assert(callBlock->HasJump());
assert(callBlock->HasInitializedJumpDest());
fgRemoveRefPred(callBlock->GetJumpDest(), callBlock);

// callBlock will call the finally handler. Convert the BBJ_LEAVE to BBJ_CALLFINALLY
Expand Down Expand Up @@ -4715,7 +4715,7 @@ void Compiler::impImportLeave(BasicBlock* block)
// stack walks.)

assert(step->KindIs(BBJ_ALWAYS, BBJ_EHCATCHRET));
assert((step == block) || !step->HasJump());
assert((step == block) || !step->HasInitializedJumpDest());

#if FEATURE_EH_CALLFINALLY_THUNKS
if (step->KindIs(BBJ_EHCATCHRET))
Expand Down Expand Up @@ -4758,7 +4758,7 @@ void Compiler::impImportLeave(BasicBlock* block)
#endif // !FEATURE_EH_CALLFINALLY_THUNKS

assert(step->KindIs(BBJ_ALWAYS, BBJ_EHCATCHRET));
assert((step == block) || !step->HasJump());
assert((step == block) || !step->HasInitializedJumpDest());

// callBlock will call the finally handler
callBlock =
Expand Down Expand Up @@ -4859,7 +4859,7 @@ void Compiler::impImportLeave(BasicBlock* block)
if ((stepType == ST_FinallyReturn) || (stepType == ST_Catch))
{
assert(step);
assert((step == block) || !step->HasJump());
assert((step == block) || !step->HasInitializedJumpDest());

if (stepType == ST_FinallyReturn)
{
Expand Down Expand Up @@ -4935,7 +4935,7 @@ void Compiler::impImportLeave(BasicBlock* block)
}
else
{
assert((step == block) || !step->HasJump());
assert((step == block) || !step->HasInitializedJumpDest());

if (step == block)
{
Expand Down Expand Up @@ -7565,11 +7565,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
assertImp((genActualType(op1) == genActualType(op2)) || (varTypeIsI(op1) && varTypeIsI(op2)) ||
(varTypeIsFloating(op1) && varTypeIsFloating(op2)));

if (block->KindIs(BBJ_NONE))
{
assert(!block->HasJump());
}

if (opts.OptimizationEnabled() && (block->KindIs(BBJ_NONE) || block->JumpsToNext()))
{
// We may have already modified `block`'s jump kind, if this is a re-importation.
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2404,7 +2404,7 @@ class LoopSearch
case BBJ_CALLFINALLY:
case BBJ_ALWAYS:
case BBJ_EHCATCHRET:
assert(block->HasJump());
assert(block->HasInitializedJumpDest());
exitPoint = block->GetJumpDest();

if (!loopBlocks.IsMember(exitPoint->bbNum))
Expand Down Expand Up @@ -4417,7 +4417,7 @@ PhaseStatus Compiler::optUnrollLoops()
newBlock->scaleBBWeight(1.0 / BB_LOOP_WEIGHT_SCALE);

// Jump dests are set in a post-pass; make sure CloneBlockState hasn't tried to set them.
assert(!newBlock->HasJump());
assert(newBlock->KindIs(BBJ_NONE));

if (block == bottom)
{
Expand Down