-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: Allow helper calls that always throw to be marked as no-return #100900
Merged
Merged
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not related to this change, but I was wondering - why do we do
(call->gtOper == GT_CALL)
?Is it possible at this point for a BBJ_THROW to end with anything other than a call?
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 am mostly concerned about a possibility that the throwing call is somewhere inside the block and followed by some unreachable junk. Then we either emit the junk unnecessarily, or, worse, emitter may skip/optimize it and we end up with the same state we are trying to prevent here.
So I wonder - is there
GT_SOMETHING
that is not a call, and yet it throws and can conclude aBBJ_THROW
?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 tested this out by asserting the last node is a call if the block has any IR, and the assert did hit. Here's some example IR it hit for:
That call is marked as not returning, but there's still a
GT_RETURN
after it; this seems to confirm your concerns. Perhaps we need a dead code elimination check somewhere that deletes any IR after no-return calls, though a quick-and-dirty fix might be to always emit the breakpoint after no-return calls (regardless of whether they're the last node or not) inCodeGen::genCall
, or somewhere similar.This method in particular expects an int to be return, so removing the
GT_RETURN
might trip up the JIT if we do it too early...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.
Morph has an exemption for tail calls that are no return, and will not prune the IR afterwards. So likely the method above is a tail call? Also there is this note:
runtime/src/coreclr/jit/morph.cpp
Lines 7825 to 7844 in 543b2cc
which the IR dump above seems to contradict, since it looks from the dump that the block kind is BBJ_THROW. I wonder if the comment is now stale?
Earlier on morph will reject implicit tail calls to no return methods, I wonder if we should just do the same for explicit tail calls, even though technically we'd be violating the intent of the
.tail
prefix. Then we would not have these special cases.There's also a concern that with profiler-driven rejitting a method that might always throw when the JIT looked at it could be updated to not always throw. I don't know if there is any good reason to support this sort of rejitting, but we should make sure there's an obvious failure if someone does it since the jitted code may depend on the old behavior. So I think in general we will want to put a breakpoint afterwards.
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.
In the above case, the call is an implicit tail call to a no-return method, so
fgMorphPotentialTailCall
rejected it, andfgMorphCall
setfgRemoveRestOfBlock = true
. TheGT_CALL
node is an operand of aGT_RETURN
node, which is in the last statement of the block, sofgRemoveRestOfBlock
didn't actually remove any IR, but it still converted the block into aBBJ_THROW
. Perhaps we could add a check that prevents converting the block into aBBJ_THROW
if it ends with aGT_RETURN
node, so that we can always expect the last node in aBBJ_THROW
to be aGT_CALL
? I'm not sure if we lose any benefits by doing this -- maybe we could still mark the block as rarely run?If we decide to make any changes to this, I'll open a follow-up PR.