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.

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

Stop tracking genReturnBB after global morph #97130

Merged

Conversation

BruceForstall
Copy link
Member

The genReturnBB pointer is set if a merged return block is created
during fgAddInternal (so, quite early during compilation). It needs
to be maintained (and the unreferenced block it points to needs to be
kept around) until global morph, which hooks up flow to this block.

After global morph, clear the pointer and don't maintain it; it is no
longer needed. Later code that was checking for it can instead check
for BBJ_RETURN blocks or GT_RETURN nodes. Also, remove the marking
of the genReturnBB block as BBF_DONT_REMOVE.

This leads to diffs, as unreachable return blocks get deleted. This can
happen, say, if all other exits are tail calls. If that happens and we're
in a case of needing a profiler hook, then the profile exit hook is also
not needed (it would be unreachable).

@ghost ghost assigned BruceForstall Jan 18, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 18, 2024
@ghost
Copy link

ghost commented Jan 18, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

The genReturnBB pointer is set if a merged return block is created
during fgAddInternal (so, quite early during compilation). It needs
to be maintained (and the unreferenced block it points to needs to be
kept around) until global morph, which hooks up flow to this block.

After global morph, clear the pointer and don't maintain it; it is no
longer needed. Later code that was checking for it can instead check
for BBJ_RETURN blocks or GT_RETURN nodes. Also, remove the marking
of the genReturnBB block as BBF_DONT_REMOVE.

This leads to diffs, as unreachable return blocks get deleted. This can
happen, say, if all other exits are tail calls. If that happens and we're
in a case of needing a profiler hook, then the profile exit hook is also
not needed (it would be unreachable).

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, but seems like win-x86 is unhappy about the removal of (even unreachable) monitor exits in synchronized methods? Presumably in those cases we can just report the entire method (assuming the info is used to exit the lock on exceptional flow by the VM)?

@BruceForstall BruceForstall force-pushed the StopTrackingGenReturnBBAfterMorph branch from 26427c7 to 04f5bcb Compare January 18, 2024 17:26
@BruceForstall
Copy link
Member Author

win-x86 handling of synchronized methods is unique: they aren't transformed to try/finally, but instead are reported to the VM via the GC information and the VM handles unlocking. I'll look into this...

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

The `genReturnBB` pointer is set if a merged return block is created
during `fgAddInternal` (so, quite early during compilation). It needs
to be maintained (and the unreferenced block it points to needs to be
kept around) until global morph, which hooks up flow to this block.

After global morph, clear the pointer and don't maintain it; it is no
longer needed. Later code that was checking for it can instead check
for `BBJ_RETURN` blocks or `GT_RETURN` nodes. Also, remove the marking
of the `genReturnBB` block as `BBF_DONT_REMOVE`.

This leads to diffs, as unreachable return blocks get deleted. This can
happen, say, if all other exits are tail calls. If that happens and we're
in a case of needing a profiler hook, then the profile exit hook is also
not needed (it would be unreachable).
The JIT still needs to report an end-of-synchronized-range offset
in the GC info for x86 synchronized methods, so just create a new
label at the end of all blocks. The VM will automatically exit
the synchronization on an exceptional method exit.
@BruceForstall BruceForstall force-pushed the StopTrackingGenReturnBBAfterMorph branch from 996b5b6 to fca4c50 Compare January 20, 2024 03:07
@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@BruceForstall
Copy link
Member Author

Diffs

Pretty significant size improvements.

@BruceForstall BruceForstall merged commit 27286ae into dotnet:main Jan 20, 2024
216 of 220 checks passed
@BruceForstall BruceForstall deleted the StopTrackingGenReturnBBAfterMorph branch January 20, 2024 18:06
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Stop tracking `genReturnBB` after global morph

The `genReturnBB` pointer is set if a merged return block is created
during `fgAddInternal` (so, quite early during compilation). It needs
to be maintained (and the unreferenced block it points to needs to be
kept around) until global morph, which hooks up flow to this block.

After global morph, clear the pointer and don't maintain it; it is no
longer needed. Later code that was checking for it can instead check
for `BBJ_RETURN` blocks or `GT_RETURN` nodes. Also, remove the marking
of the `genReturnBB` block as `BBF_DONT_REMOVE`.

This leads to diffs, as unreachable return blocks get deleted. This can
happen, say, if all other exits are tail calls. If that happens and we're
in a case of needing a profiler hook, then the profile exit hook is also
not needed (it would be unreachable).

* Allow x86 synchronized methods to have unreachable return blocks

The JIT still needs to report an end-of-synchronized-range offset
in the GC info for x86 synchronized methods, so just create a new
label at the end of all blocks. The VM will automatically exit
the synchronization on an exceptional method exit.

* Create final IG before clearing GC info
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants