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

Stop generating CLS_VAR for 64 bit targets #66298

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Mar 7, 2022

This is the first change that fully disables CLS_VAR - on 64 bit platforms. The reason I am doing it this way is because there will be many more (positive) diffs on x86, and ARM likely requires some CSE heuristic changes and/or quirks to avoid some regressions.

We are not expecting diffs on ARM64. But expecting some limited amount of them on x64, caused by:

  1. "Hoisting" more constants (Do not hoist constants if they will not be CSEed #64039).
  2. Slightly different gtSetEvalOrder behavior: more aggressive reversal of ASG(..., IND(static addr)) (it didn't reverse for leaf CLS_VAR RHSs).
  3. Very subtle changes in register allocation due to more opportunities for reuse of constants.

At first, my goal was to make this a zero-diff change, by mititigating all the above ("and more") with other changes, but after some weeks of looking at the problems from different angles, I "gave up" and decided it would be more productive to accept the diffs.

Diffs.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 7, 2022
@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 Mar 7, 2022
@ghost
Copy link

ghost commented Mar 7, 2022

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

Issue Details

This is the first change that fully disables CLS_VAR - on 64 bit platforms. The reason I am doing it this way is because there will be many more (positive) diffs on x86, and ARM likely requires yet one more change to CSE heuristics and/or quirks to avoid some regressions.

We are not expecting diffs on ARM64, and expecting some limited diffs on x64, due to the fact we will now be "hoisting" more constants (#64039).

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review March 7, 2022 18:36
@SingleAccretion
Copy link
Contributor Author

Actually fewer diffs than I initially feared, very nice (and green CI!).

@dotnet/jit-contrib - the simple part of deleting CLS_VAR.

@jakobbotsch
Copy link
Member

Nice! Do you plan to delete CLS_VAR in another change?

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Mar 7, 2022

Yes, there will be at least the x86 change and the ARM change (probably the latter one will actually delete the code).

@jakobbotsch jakobbotsch merged commit cbc856d into dotnet:main Mar 14, 2022
@SingleAccretion SingleAccretion deleted the Delete-ClsVar-64-Bit branch March 14, 2022 10:29
@ghost ghost locked as resolved and limited conversation to collaborators Apr 13, 2022
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants