-
Notifications
You must be signed in to change notification settings - Fork 5k
Adding additional 8 eGPR. #113988
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
Adding additional 8 eGPR. #113988
Conversation
657a419
to
50153a7
Compare
5e5548c
to
85c9ef8
Compare
3a45505
to
963c341
Compare
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.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (11)
- src/coreclr/jit/compiler.cpp: Language not supported
- src/coreclr/jit/emit.h: Language not supported
- src/coreclr/jit/emitxarch.cpp: Language not supported
- src/coreclr/jit/emitxarch.h: Language not supported
- src/coreclr/jit/lsra.cpp: Language not supported
- src/coreclr/jit/lsra.h: Language not supported
- src/coreclr/jit/lsrabuild.cpp: Language not supported
- src/coreclr/jit/register.h: Language not supported
- src/coreclr/jit/target.h: Language not supported
- src/coreclr/jit/targetamd64.h: Language not supported
- src/coreclr/jit/unwindamd64.cpp: Language not supported
@BruceForstall @jakobbotsch @kunalspathak This PR is ready for review |
@DeepakRajendrakumaran - can you double check why TP of arm64 got affected? |
This commit affects all targets but should be negligible for non x64 - commit This is to skip over eGPR for non APX x64 machines when iterating over RegisterRecord. This can be extended to arm64 if there are any registers needed to be skipped over with/without features for arm64. Will need to add something similar - Change for x86 |
This reverts commit 963c341.
/azp run runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs |
Azure Pipelines successfully started running 2 pipeline(s). |
@DeepakRajendrakumaran - I know you have done lot of work to improve TP. I am wondering did you perform analysis of the remaining regressions and can share where the cost is coming from? |
/azp run runtime-coreclr libraries-jitstress2-jitstressregs |
Azure Pipelines successfully started running 1 pipeline(s). |
I ran this for The tpdiff for Overall (+2.12%)
MinOpts (+4.46%)
FullOpts (+1.58%)
I did try some changes with separating out registers to free in |
Failures are timeout and #114638 |
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. Thanks!
@DeepakRajendrakumaran Thanks for the hard work and persistence to get this in. |
Thanks to you, @kunalspathak and others involved in the discussions for helping me make the necessary tweaks to get this in. :) |
Overview
This PR does the following 2 things
on x64 without APX for instance, REG16-REG_31 are unused. We iterate through this processing them currently. This allows us to just skip these
TPDIFF impact
From CI for this PR after last few PRs to improve tpdiff(See here for numbers before)
windows x64
Overall (+1.67% to +2.54%)
MinOpts (+3.14% to +5.71%)
FullOpts (+1.63% to +2.18%)
Alternate test numbers from running superpmi directly and using the 'Total Time':
% Regression = ((Diff Mean - Base Mean)/ Base Mean) * 100)