Skip to content

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.
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 17, 2024
@dotnet-policy-service
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

@azure-pipelines
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
@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.

@jakobbotsch jakobbotsch requested a review from kunalspathak June 19, 2024 13:43
Copy link
Contributor

@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.

@jakobbotsch jakobbotsch merged commit a39fed2 into dotnet:main Jun 19, 2024
@jakobbotsch jakobbotsch deleted the lsra-cleanup-kill-register-assignment branch June 19, 2024 18:17
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

2 participants