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

Optimize "new DateTime(<const args>)" via improvements in JIT VN #81005

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 22, 2023

This PR is the last missing piece to fold new DateTime(2023, 1, 22) call into just a constant in JIT. While there is not much value in folding DateTime specifically, the PR(s) improves constant folding in JIT around RVA fields for a general case and makes it more robust. Just look at this screenshot (click to zoom):

image

All of that code is inlined and folded including "array" accesses to those ReadOnlySpan<> fields during JIT compilation into Test() method on the left-top.

Previous codegen for Test():

; Method ConsoleApp1.Program:Test():System.DateTime:this
G_M15270_IG01:              ;; offset=0000H
       4883EC28             sub      rsp, 40
G_M15270_IG02:              ;; offset=0004H
       48B8F82AE494FB7F0000 mov      rax, 0x7FFB94E42AF8      ; static handle
       8B5004               mov      edx, dword ptr [rax+04H]
       8B00                 mov      eax, dword ptr [rax]
       2BD0                 sub      edx, eax
       83FA16               cmp      edx, 22
       7218                 jb       SHORT G_M15270_IG04
       05ED440B00           add      eax, 0xB44ED
       48BA00C0692AC9000000 mov      rdx, 0xC92A69C000
       480FAFC2             imul     rax, rdx
G_M15270_IG03:              ;; offset=002DH
       4883C428             add      rsp, 40
       C3                   ret      
G_M15270_IG04:              ;; offset=0032H
       FF15B88E0B00         call     [System.ThrowHelper:ThrowArgumentOutOfRange_BadYearMonthDay()]
       CC                   int3     
; Total bytes of code: 57

What this PR does specifically is:

  1. Don't add implicit VN cast for nint -> byref operations on top of ICON handles (thanks to @SingleAccretion for idea). This leads to a slight size regression because JIT now recognizes more places it can propagate constants to
  2. Don't propagate certain types of handles to fix size regressions introduced by ^
  3. Add ability to obtain a FieldSeq* for given address via a side hashmap since we don't encode FieldSeq into a VN.

SPMI diffs are expected not to catch some improvements due to new VM calls (contexts) to fold RVAs

Just-for-fun retrospective of PRs helped to fold all of that 🙂:

@ghost ghost assigned EgorBo Jan 22, 2023
@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 Jan 22, 2023
@ghost
Copy link

ghost commented Jan 22, 2023

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

Issue Details

This PR is the last missing piece to fold new DateTime(2023, 1, 22) into just a constant in JIT (many other previous PRs helped with that as well). While there is not much value in this operation specifically, the PR(s) improves constant folding in JIT around RVA fields for a general case and makes it more robust. Just look at this screenshot (click to zoom):

image

All of that code is inlined and folded including "array" accesses to those ReadOnlySpan<> fields.

What this PR does specifically is:

  1. Don't add implicit VN cast for nint -> byref operations on top of ICON handles (thanks to @SingleAccretion for idea). This leads to a slight size regression because JIT now recognizes more places it can propagate constants too (and CSE decides not to touch them after) - overall it's something that should be fixed in CSE.
  2. Add ability to obtain a FieldSeq* for given address via a side hashmap.
Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as ready for review January 22, 2023 23:04
@EgorBo
Copy link
Member Author

EgorBo commented Jan 23, 2023

@jakobbotsch @SingleAccretion PTAL VN change

Diffs, but they don't represent actual changes when something new is folded in RVA, thus, the missing contexts in the Diff.

Some size regressions because once I dropped the implicit cast for nint->byref a lot of locals started to have the same VN as constants -> they were propagated. I tried to stop that by disabling propagation of these specific handles but it also had its own cost - e.g. constant aren't propagated to their uses accross calls, etc. Overall, it's something that we need to fix via proper constant re-materialization in RA (that we quite often mention)

@ShreyasJejurkar
Copy link
Contributor

This is Ultra Pro Max amazing! 🎉🎉

src/coreclr/jit/assertionprop.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
@EgorBo
Copy link
Member Author

EgorBo commented Jan 24, 2023

@SingleAccretion does it look good to you now?

src/coreclr/jit/assertionprop.cpp Outdated Show resolved Hide resolved
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
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, I assume the test failures are unrelated. Is there any significant memory impact from the new map?

@EgorBo
Copy link
Member Author

EgorBo commented Jan 24, 2023

LGTM, I assume the test failures are unrelated. Is there any significant memory impact from the new map?

I wasn't able to detect any significant impact, because initially wanted to make it lazy-allocated but gave up. As for its content then only methods with static fields contribute to that (and we rarely see method with a lot of static field operations inside them). As for JIT TP, I have a small improvement for VN's APIs to get -0.02% back but that will land separately

Failures are #80666

@EgorBo EgorBo merged commit 653cb87 into dotnet:main Jan 24, 2023
@EgorBo EgorBo deleted the vn-static-fld-improvements branch January 24, 2023 15:54
@ghost ghost locked as resolved and limited conversation to collaborators Feb 23, 2023
@jeffhandley jeffhandley added the blog-candidate Completed PRs that are candidate topics for blog post coverage label Mar 14, 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 blog-candidate Completed PRs that are candidate topics for blog post coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants