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

Do not number partial definitions and ARGPLACE nodes #64898

Merged
merged 3 commits into from
Feb 21, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Feb 7, 2022

Three changes in this set:

  1. Refactor the numbering of LCL_VAR "uses" - preserve the old behavior, reduce confusion on what should and should not be numbered, make the kinds of type mismatches we expect (and tolerate) explicit via asserts.
  2. Do not number partial definitions as uses. This is wasted work.
  3. Do not give unique VNs to ARGPLACE nodes - this, again, is simply wasting memory, as the nodes in question will have their VNs always reset by fgValueNumberCall anyway.

Overall, we are saving a little under 0.4% in memory consumption when CG-ing x64 Release CoreLib with these changes.

No diffs as expected.

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

ghost commented Feb 7, 2022

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

Issue Details

Three changes in this set:

  1. Refactor the numbering of LCL_VAR "uses" - preserve the old behavior, reduce confusion on what should and should not be numbered, make the kinds of type mismatches we expect (and tolerate) explicit via asserts.
  2. Do not number partial definitions as uses. This is wasted work.
  3. Do not give unique VNs to ARGPLACE nodes - this, again, is simply wasting memory, as the nodes in question will have their VNs always reset by fgValueNumberCall anyway.

Overall, we are saving a little under 0.4% in memory consumption when CG-ing x64 Release CoreLib with these changes.

No diffs are expected.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review February 7, 2022 16:30
const GenTree* nextNode = lcl->gtNext;
const LclVarDsc* varDsc = lvaGetDesc(lcl);

assert((nextNode->gtOper == GT_ADDR && nextNode->AsOp()->gtOp1 == lcl) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't replicate this assert as I don't find it useful - yes, we will see some locals under ADDRs here, but that's just how the IR is.

@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch jakobbotsch self-assigned this Feb 9, 2022
Do not number "partial definitions" - it is simply wasted work.
For ARGPLACE nodes, they will be overwritten when numbering the call.
For nodes that do not produce values, they serve no purpose, and just
waste memory.

Also, delete RET_EXPR/FTN_ADDR handling. They will never appear in VN.
src/coreclr/jit/valuenum.cpp Show resolved Hide resolved
@jakobbotsch jakobbotsch merged commit 41ab200 into dotnet:main Feb 21, 2022
@SingleAccretion SingleAccretion deleted the Misc-VN-Cleanup branch February 21, 2022 20:30
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 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.

3 participants