-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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: Assert bbJumpKind in BasicBlock::HasJump #95152
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBased on feedback in #94239, before reading a block's
|
cc @dotnet/jit-contrib, @BruceForstall PTAL |
@@ -662,13 +662,13 @@ struct BasicBlock : private LIR::Range | |||
|
|||
bool HasJump() const | |||
{ | |||
assert(KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE)); | |||
return (bbJumpDest != nullptr); | |||
} |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Might want to wait for @BruceForstall too since this was his feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
You now have six copies of KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE)
I had suggested helper functions:
HasJumpDest() => KindIs(BBJ_ALWAYS,<others>) // bbJumpDest is valid
HasJumpSwt() => KindIs(BBJ_SWITCH) // bbJumpSwt is valid
HasJumpEhf() => KindIs(BBJ_EHFINALLYRET) // bbJumpEhf is valid
Also, it seems unfortunate that you're introducing two new BBJ_NONE cases that you'll have to remove with #94239
Ah sorry, I had misinterpreted those suggestions as replacements for
Yeah, I was planning on doing some cleanup work to fix all the new merge conflicts in #94239 from these follow-up PRs anyway, so I'm not too worried about having to fix those new |
@BruceForstall PTAL. I replaced all the |
src/coreclr/jit/block.h
Outdated
bool HasJumpDest() const | ||
{ | ||
// These block types should always have bbJumpDest set | ||
return KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing BBJ_EHFILTERRET
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that; BBJ_EHFILTERRET
was missing from the original asserts.
Based on feedback in #94239, before reading a block's
bbJumpDest
inBasicBlock::HasJump
, we should assert it has a valid jump kind. In other words, callingHasJump
on a block that is never expected to have a jump destination should be considered invalid behavior.