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: Clean up RefPosition "killed registers" storage #103560

Merged

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Jun 17, 2024

Avoid some ifdefs and rename the field to reflect more specifically what it represents.

Also avoid calling getWeight entirely for RefTypeKill ref positions, even for dumping. Dumping this info does not make much sense since there was a requirement that it was never called in non-dump paths anyway.

Avoid some ifdefs and fix a crash during JITDUMP in LSRA.

Also avoid calling `getWeight` entirely for `RefTypeKill` ref positions,
even for dumping. Dumping this info does not make much sense anyway
since there was a requirement that it was never called in non-dump paths
anyway.
@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 Jun 17, 2024
Copy link
Contributor

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

@jakobbotsch

This comment was marked as outdated.

@jakobbotsch
Copy link
Member Author

No asm diffs. Some large Linux-hosted JIT TP regressions that I need to look into. The Windows-hosted JIT has negligible TP regressions.

@jakobbotsch
Copy link
Member Author

Looks like #103573 will fix the JITDUMP crash as a separate change.

@jakobbotsch jakobbotsch changed the title JIT: Use killRegisterAssignment unconditionally in LSRA JIT: Clean up RefPosition "killed registers" storage Jun 17, 2024
@build-analysis build-analysis bot mentioned this pull request Jun 17, 2024
@jakobbotsch
Copy link
Member Author

The linux-hosted JIT regressions are just the LTO mismatch when PGO is disabled that is being discussed in #103058:

Base: 50016445039, Diff: 50115771931, +0.1986%

84928137 : +53461.67% : 74.74% : +0.1698% : _ZNK9regMaskTP14IsRegNumInMaskE15_regNumber_enum               
20940077 : +8.07%     : 18.43% : +0.0419% : _ZN10LinearScan12processKillsEP11RefPosition                   
309229   : +1.64%     : 0.27%  : +0.0006% : _ZN10LinearScan24allocateRegistersMinimalEv                    
297458   : +0.33%     : 0.26%  : +0.0006% : _ZN10LinearScan25buildKillPositionsForNodeEP7GenTreej9regMaskTP
-7146243 : -0.65%     : 6.29%  : -0.0143% : _ZN10LinearScan17allocateRegistersEv                           

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr superpmi-diffs

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch jakobbotsch marked this pull request as ready for review June 19, 2024 13:39
src/coreclr/jit/lsra.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @kunalspathak

No asm diffs. linux hosted regressions due to what I mentioned above. Some minor MinOpts TP regressions (up to 0.04% in the contexts with many MinOpts contexts). I think it's worth it to avoid the ifdefs and avoid bugs like the one fixed by #103573.

Copy link
Member

@kunalspathak kunalspathak 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 cleaning it up. The new name killedRegisters is much better.

src/coreclr/jit/lsra.cpp Outdated Show resolved Hide resolved
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
@jakobbotsch jakobbotsch merged commit a39fed2 into dotnet:main Jun 19, 2024
107 checks passed
@jakobbotsch jakobbotsch deleted the lsra-cleanup-kill-register-assignment branch June 19, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

2 participants