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

Split assignment rationalization into a separate phase #85180

Merged
merged 9 commits into from
May 1, 2023

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Apr 21, 2023

And start adding the support required for store nodes to replace assignments in HIR.

This change is the proper beginning to the ASG removal project. The plan as of currently is to slowly move the newly introduced phase up the phase list, adding support to shared code (aka morph) where necessary and rewriting the rest as needed. In other words, there will be no intermediate states where both ASG and store nodes can appear in IR at once - the work will be split across phases, but not node types.

Initially, I was concerned about the TP cost of introducing a new phase, and considered more complex schemes to achieve incrementality, however, this ended up not being a problem - with some judicious use of PIN, we should be about even or slightly up on the TP metrics even with this change. Needless to say, once the end goal here has been achieved, we expect significant cumulative gains as the IR will shrink and special-casing from a number of places removed.

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

ghost commented Apr 21, 2023

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

Issue Details

And start adding the support required for store nodes to replace assignments in HIR.

This change is the proper beginning to the ASG removal project. The plan as of currently is to slowly move the newly introduced phase up the phase list, adding support to shared code (aka morph) where necessary and rewriting the rest as needed. In other words, there will be no intermediate states where both ASG and store nodes can appear in IR at once - the work will be split across the phases, but not node types.

Initially, I was concerned about the TP cost of introducing a new phase, and considered more complex schemes to achieve instrumentality, however, this ended up not being a problem - with some judicious use of PIN, we should be about even or slightly up on the TP metrics even with this change. Needless to say, once the end goal here has been achieved, we expect significant cumulative gains as the IR will shrink and special-casing from a number of places removed.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Apr 26, 2023

Diffs.

The TP situation (on 64 bit, 32 bit sees 0.2%-0.4% TP improvements across the board) has been improved, but some MinOpts regressions still remain. The larger ones look to be in collections with a lack of MinOpts MCs, and where such MCs are well-represented, the average is +0.05%. We are adding a new (temporary, by nature) phase here after all, so I am inclined to not spend more energy on mitigating this unless there are objections. As a point of reference, follow-up changes to remove ASGs from the optimization phases have shown > 1% TP improvements (in optimized code, naturally).

@dotnet/jit-contrib

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

And make "OperIsStore" means exactly these opers:
 GT_STORE_LCL_VAR
 GT_STORE_LCL_FLD
 GT_STOREIND
 GT_STORE_BLK

Notable: GT_STORE_DYN_BLK is excluded as it does
not follow conventions of all other stores (e. g.
is VOID-typed) and acts more like opaque atomic
operations instead.

Also fix an old bug with gtNodeHasSideEffects.
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Apr 28, 2023

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

Ooops, did not notice this... I've removed the stress bit, since it looks like moving rationalization earlier (thus, testing morph) will be easier than originally anticipated.

@jakobbotsch
Copy link
Member

Ooops, did not notice this... I've removed the stress bit, since it looks like moving rationalization earlier (thus, testing morph) will be easier than originally anticipated.

No worries, I'll rerun it just to be safe.

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM. Agree that it's fine to take the TP regressions for now.

@SingleAccretion
Copy link
Contributor Author

One unique failure in libraries stress, though hard to see it being related:

===========================================================================================================
/root/helix/work/workitem/e /root/helix/work/workitem/e
  Discovering: System.Net.Quic.Functional.Tests (method display = ClassAndMethod, method display options = None)
MsQuic supported and using 'libmsquic.so 2.1.8.348060 (c6931cb1eae6d885a8ec04c2e5558129b616730d)'.
  Discovered:  System.Net.Quic.Functional.Tests (found 115 of 124 test cases)
  Starting:    System.Net.Quic.Functional.Tests (parallel test collections = on, max threads = 2)
    System.Net.Quic.Tests.MsQuicPlatformDetectionTests.UnsupportedPlatforms_ThrowsPlatformNotSupportedException [SKIP]
      Condition(s) not met: "IsQuicUnsupported"
    System.Net.Quic.Tests.QuicListenerTests.AcceptConnectionAsync_ListenerDisposed_Throws [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.Security.Authentication.AuthenticationException)
      Actual:   typeof(System.Net.Quic.QuicException): The connection timed out from inactivity.
      ---- System.Net.Quic.QuicException : The connection timed out from inactivity.

@am11
Copy link
Member

am11 commented May 1, 2023

One unique failure in libraries stress, though hard to see it being related:

Stack trace in logs resembles the one reported in #85371.

@jakobbotsch jakobbotsch merged commit 9894fbf into dotnet:main May 1, 2023
@SingleAccretion SingleAccretion deleted the Op-Req-Asg branch May 1, 2023 15:14
@ghost ghost locked as resolved and limited conversation to collaborators May 31, 2023
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