-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: Stop reordering arguments throwing distinct exceptions #70893
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
Conversation
Implement a precise scan for arguments that may throw exceptions which collects the exceptions they may throw. When we see two such interfering sets, make sure that the first argument is evaluated to a temp so that they do not get reordered by sort. Fix dotnet#63905
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsImplement a precise scan for arguments that may throw exceptions which Fix #63905 Also optimistically see if this is enough to relax some of the forward sub constraints...
|
This reverts commit 1536b67. Will do this separately in a follow-up change.
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Diffs. Some small regressions and a minor TP impact, as expected. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Ensure we do not set stack args as needing a temp.
|
jitstress runs hit a bunch of #71023 so will retry. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Does allowing this slight amount of reordering in the face of exceptions provide benefits over not allowing any reordering at all? I get that it would be very difficult for end users to figure out which arg tree is responsible, especially so if the tree can't have global effects, but I wonder what the difference is between this and just relying on |
Yes, this results in larger regressions (it was the first thing I tried): aspnet.run.windows.x64.checked.mch:Detail diffsbenchmarks.run.windows.x64.checked.mch:Detail diffscoreclr_tests.pmi.windows.x64.checked.mch:Detail diffslibraries.crossgen2.windows.x64.checked.mch:Detail diffslibraries.pmi.windows.x64.checked.mch:Detail diffslibraries_tests.pmi.windows.x64.checked.mch:Detail diffs |
AndyAyersMS
left a comment
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.
Changes LGTM.
Implement a precise scan for arguments that may throw exceptions which
collects the exceptions they may throw. When we see two such interfering
sets, make sure that the first argument is evaluated to a temp so that
they do not get reordered by sort.
Fix #63905
I think there is still low hanging fruit in optimizing the sort itself but I am focusing on fixing the correctness issues here.
I expect once this is merged we can loosen up some of the forward sub restrictions around call args.