-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Change LSRA to more efficiently initialize the availableRegs array #83479
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
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis was noticed in #82731 (comment) and should provide a minor improvement since it will just linearly initialize the data without needing to do any checks.
|
|
CC. @dotnet/jit-contrib, @kunalspathak Some nice TP wins: https://dev.azure.com/dnceng-public/public/_build/results?buildId=206256&view=ms.vss-build-web.run-extensions-tab Windows X64Windows Arm64Linux is similar numbers. |
|
LGTM...surprisingly it showed TP win given that this is only used in constructor. Also, may be not worth another commit, but I would change the name of variable from |
I've noticed that almost any change to LSRA can have significant impact on TP, more so than almost any other phase. My hypothesis is that even though this constructor is only called once, it's doing such a significant amount of "more work" than most other phases (there are many methods which are only 1-5 opcodes). It being required for every single method and that coupled with T0 being the default and so no other optimizations being typically done makes LSRA one of the most perf critical points. -- I've also noticed that a lot of the setup LSRA is doing each time is computing some information that is really "per process" and not "per LSRA instance". I think there is room to make some of this static or to split it out so that it doesn't have to be redone over and over, but instead is done once and copied into new instances.
I tried to go with some short name so as to not deviate from the other names in the macro and picked something that wasn't likely to conflict with any other local/field in the scope |
Are you referring to something along the lines of registers involved, etc.?
Makes sense. |
Right. We're doing some amount of work to determine things like which registers are available, initializing the "all registers" lists, etc. Some of this looks to be effectively immutable data ( |
We did see that this was hot already in the measurements at #76850 (comment). That's why I had similar suggestion to improve this loop there. |
This was noticed in #82731 (comment) and should provide a minor improvement since it will just linearly initialize the data without needing to do any checks.