-
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
ARM/ARM64: Optimize virtual call stub for R2R and JIT #36817
Conversation
6952980
to
dc58cae
Compare
@dotnet/jit-contrib , @jeffschwMSFT , @richlander , @davidwrighton , @jkotas |
@dotnet/runtime-infrastructure - I started the runs few days back and they are still showing as running. Is there a known issue or do I need to cancel/rerun the jobs? |
Yeah, that is a GH bug that is already reported. If you go into Azure DevOps, the build shows as finished and there are two failing legs: https://dev.azure.com/dnceng/public/_build/results?buildId=657536&view=results |
@BruceForstall , @CarolEidt - can you please take a preliminary look? I will see why Linux arm64 Release failed. The windows leg passed. |
@kunalspathak I believe you unfortunately ran the rolling build on a state where it was broken. The linux_musl_arm64 issue you're seeing was fixed and such as the browser_wasm issue. So I would recommend rebasing on top of master and queueing another build. |
This reverts commit 441d866609b406c57ed586a79f5ed27e99c31194.
This reverts commit 0183e76e83d5ba1057b95ba37204cc525e295bf5.
63cd1f9
to
c82c277
Compare
Thanks @safern for saving my day. :) |
@kunalspathak FYI... we will soon have linux_arm, linux_arm64 and linux_musl_arm64 libraries test runs using a checked coreclr when coreclr is changed: #36910 |
Does it impact my existing runs? Or are you suggesting that I might have to do another run when #36910 is merged? |
No it doesn't impact any of your runs. It was just an FYI for future PRs, that you'll get this sort of testing on your PR by default if you changed anything under coreclr. |
Ah, so I won't have to manually trigger |
IIRC, not yet. I'll double check. |
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'd just like to see a description of what's being done (the why and how) for clarity.
Otherwise LGTM.
@BruceForstall - As pointed out in #35108 (comment) there were 615732 redundant adrp/add pairs through out framework libraries. With work done in #35675 and in this PR, I just verified that there are no redundant adrp/add pairs anymore. |
Optimize the calls to virtual stub by eliminating the redundant load of address. This is similar to #35675 but optimizes virtual call stubs which impacts both crossgen/JIT.
Fixes: #36700
CrossGen improvements:
JIT improvements: