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

JIT: better support for patchpoints in try regions #59784

Merged

Conversation

AndyAyersMS
Copy link
Member

This change adds control flow to ensure that an OSR method for a patchpoint
nested in try regions enters those regions try from the first block of each
try rather than mid-try.

This lets these OSR methods conform to the data flow expectations that the
only way control flow can enter a try is via its first block.

See #33658 for more details on the approach taken here.

Fixes #35687.

This change adds control flow to ensure that an OSR method for a patchpoint
nested in try regions enters those regions try from the first block of each
try rather than mid-try.

This lets these OSR methods conform to the data flow expectations that the
only way control flow can enter a try is via its first block.

See dotnet#33658 for more details on the approach taken here.

Fixes dotnet#35687.
@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 Sep 29, 2021
@ghost
Copy link

ghost commented Sep 29, 2021

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

Issue Details

This change adds control flow to ensure that an OSR method for a patchpoint
nested in try regions enters those regions try from the first block of each
try rather than mid-try.

This lets these OSR methods conform to the data flow expectations that the
only way control flow can enter a try is via its first block.

See #33658 for more details on the approach taken here.

Fixes #35687.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

It might be possible to merge this new logic with the try region trimming done in fgRemoveEmptyBlocks but for now I'm keeping it separate.

No diffs outside of OSR (which we currently can't easily diff; seems like I should do an SPMI collection here perhaps).

cc @dotnet/jit-contrib

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

  1. I think you should add some of the comment from On Stack Replacement Next Steps #33658 (comment) into your new code, especially the pseudo-code of the flow you are creating.
  2. It's pretty weird that a function named fgRemoveEmptyBlocks can create a bunch of blocks and flow and a new temp.
  3. Re (2), it looks like fgRemoveEmptyBlocks gets called on inlinees, and then later in morph init. I guess that ends up not creating multiple flow because we don't inline functions with EH, but it maybe creates an impediment if we ever do (and the inlinee is an OSR method? So, maybe not a problem?)

@AndyAyersMS
Copy link
Member Author

Going to wait on this until #59789 lands so (hopefully) I can get clean OSR runs.

@AndyAyersMS
Copy link
Member Author

Yeah, fgRemoveEmptyBlocks should probably be renamed, maybe fgUpdateFlowAfterImportation ...? Similar in nature to fgAddInternal too, though that runs a bit later on.

As for running it on inlinees, right, only the first bit (removing un-imported blocks) will matter today.

We'll never inline OSR methods, they never get called in a normal fashion.

@AndyAyersMS
Copy link
Member Author

Merged in the other OSR fix, so will try running the experimental pipeline here. Don't expect it to be green as there is a latent crossgen2 issue.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Looks like a number of novel failures, so I'll have to revise this PR.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Still have one overly aggressive assert (mutual protect case)... the OSR entry can be in a nested try and still not need step blocks.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

jit-experimental now has just the expected set of failures.

@AndyAyersMS
Copy link
Member Author

@BruceForstall feel free to take another look; think I addressed your comments.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding the detailed comments.

@AndyAyersMS AndyAyersMS merged commit 26954f5 into dotnet:main Oct 6, 2021
@AndyAyersMS AndyAyersMS deleted the FixMidTryPatchpointPostImporter branch October 6, 2021 01:40
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2021
@AndyAyersMS AndyAyersMS restored the FixMidTryPatchpointPostImporter branch October 11, 2024 21:23
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.

JIT\opt\OSR\tailrecursetry\tailrecursetry.cmd fails with COMPlus_FastTailCalls=0
2 participants