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

Morph ASG(ONE_FLD_STRUCT, CALL) using BITCAST #61049

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Oct 31, 2021

This changes the representation we have for the special case of assignments from calls that return a struct with one field
to a promoted local of that struct type, making it more normal and simplifying the model for SSA and value numbering.

In the past, the optimization phases treated this case specially and "skipped" the outer struct local in relevant cases. Instead, I propose a new form for this assignment: ASG(FLD, BITCAST(CALL)), that gets rid of this special-casing, and introduces the useful invariant that if a node has an SSA name, then it always represents a def or use of the corresponding SSA local.

After optimizations have done their work, lowering removes the new bitcasts as they do not have meaningful value in the backend.

There are some downsides to this representation. For example, if we decide to stop retyping helper calls in the importer, VN will start giving BITCAST VNs to locals assigned values produced by important helpers such as TypeHandleToRuntimeTypeHandle.

It makes the IR a bit larger, and, perhaps more importantly, less dense. I can measure a 0.003% increase in memory consumption when CG-ing x64 CoreLib with this change. There are some TP benefits from the reduced checks too, they may offset this regression (my PIN is too noisy to detect anything though :( ).

I posit that the benefits outweigh the drawbacks in this case.

Part of #58312.

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

ghost commented Oct 31, 2021

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

Issue Details

Let's see what the CI says about this change.

Part of #58312.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Nov 1, 2021

There are diffs with this change: win-x64, win-arm64. There are a few sources of them.

  1. Detection of candidates for the EH write-thru optimization does not "see" through the struct assignments here:
    if (tree->gtFlags & GTF_VAR_DEF) // Is this is a def of our variable
    With this change it does, and this causes different register allocation (in all examples I've seen - minor size regressions) as the promoted field is marked "single def for the purposes of EH write-thru".
  2. VN-based copy propagation is enabled for the fields. This is a bit counterintuitive - what copy propagation if we see that it already "looks through" the structs and such (and the fields are given VNs)? The reason is that copy propagation checks for the VNs assigned to the LHS of an assignment. Before this change, no VN was assigned to the struct local on the LHS as it was not tracked, thus copy propagation "missed" some defs for the field. One will notice: if the assignment does not update any VNs, then how come the SSA def for the field gets assigned a VN after all? The answer is in the code here, which does "see through" the struct local, and assigns the per-SSA-def-VN in question. I believe this path can be deleted now (it just allocates unnecessary unique VNs that will be overwritten when numbering the assignment anyway). I did not do it in this PR though - not worth the risk.
  3. CSE and folding kicking in for analyzable results of helper calls. This one is pretty straightforward - if we can VN the call source for the bitcast, we can also properly VN the field - recall that before this change all such definitions were given unique VNs. However, it seems that we retype the helpers to primitives anyway, so this does not have as much of an impact (a few cases here and there involving CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE).
  4. Loop cloning misses definitions for fields of promoted structs #61040 - we now (correctly) see the promoted field as not invariant in the loop and give up on eliminating the bounds check, either statically (size regressions) or via cloning (size improvements). These are the "big" diffs in libraries - we have surprisingly many of them.

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Nov 1, 2021

Will leave this as a draft for now pending #61040 being fixed, to reduce the noise in the diffs.

Edit: now with #61040 fixed, it is perhaps time to finally get back to this..

@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from 54e2485 to bdf28d9 Compare November 6, 2021 13:35
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from bdf28d9 to e512f20 Compare November 20, 2021 22:07
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from e512f20 to c72c794 Compare December 1, 2021 12:19
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from c72c794 to 931f571 Compare December 10, 2021 18:02
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from 2f6c81a to 6efc97a Compare December 20, 2021 10:34
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from 6efc97a to 5fa597b Compare January 13, 2022 13:26
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from 530e6f2 to f84b512 Compare January 28, 2022 15:30
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from 5bfcaa8 to 3160e39 Compare February 12, 2022 10:11
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from 11366bb to 4cc9e1c Compare February 25, 2022 20:50
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from 4cc9e1c to f2b7c58 Compare March 7, 2022 21:46
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 3 times, most recently from 6701780 to 16c5773 Compare April 3, 2022 19:30
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from 16c5773 to 2c0cd3d Compare April 15, 2022 15:12
@ghost ghost added the no-recent-activity label Apr 29, 2022
@ghost
Copy link

ghost commented Apr 29, 2022

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from 2c0cd3d to ba64df5 Compare May 2, 2022 12:41
@ghost ghost removed the no-recent-activity label May 2, 2022
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from d4e9ad6 to b53f19f Compare May 10, 2022 15:31
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from b53f19f to 521071b Compare May 13, 2022 14:58
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from fc6e496 to dbf12e3 Compare May 28, 2022 11:54
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from dbf12e3 to 523a150 Compare June 4, 2022 18:15
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from 46889ce to e719865 Compare June 15, 2022 13:21
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch 2 times, most recently from 24a6eff to f202ce7 Compare July 2, 2022 14:08
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from f202ce7 to 0c46158 Compare July 7, 2022 20:03
This changes the representation we have for the special case
of assignments from calls that return a struct with one field
to a promoted local of that struct type, making it more normal
and simplifying the model for SSA and value numbering.

In the past, the optimization phases treated this case specially
and "skipped" the outer struct local in relevant cases. Instead,
I propose a new form for this assignment: ASG(FLD, BITCAST(CALL)),
that gets rid of this special-casing, and introduces the useful
invariant that if a node has an SSA name, then it always represents
a def or use of the corresponding SSA local.

After optimizations have done their work, lowering removes the new
bitcasts as they do not have meaningful value in the backend.

There are some downsides to this representation. For example, if
we decide to stop retyping helper calls in the importer, VN will
start giving BITCAST VNs to locals assigned values produced by
important helpers such as TypeHandleToRuntimeTypeHandle.

It makes the IR a bit larger, and, perhaps more importantly, more
sparse. I can measure a 0.003% increase in memory consumption when
CG-ing x64 CoreLib with this change. There are some TP benefits
from the reduced checks too, they may offset this regression.

It seems to me that the benefits outweigh the drawbacks in this case.
@SingleAccretion SingleAccretion force-pushed the Bitcasts-For-Promoted-Struct-Asgs-With-Call-Srcs branch from 0c46158 to 4974a67 Compare July 21, 2022 13:40
@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Aug 11, 2022
@JulieLeeMSFT
Copy link
Member

@SingleAccretion, I marked this PR as .NET 8. If you think it should be .NET 7, please let me know.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 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