KEMBAR78
NativeAOT: Optimize static fields of gc types by EgorBo · Pull Request #80969 · dotnet/runtime · GitHub
Skip to content

NativeAOT: Optimize static fields of gc types #80969

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 23, 2023
Merged

NativeAOT: Optimize static fields of gc types #80969

merged 7 commits into from
Jan 23, 2023

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 21, 2023

Follow up to #79709 but for gc fields now, e.g.:

static string field;

static string GetField() => field;

Was:

; Method ConsoleApp1.Program:GetField():System.String
       4883EC28             sub      rsp, 40
       E800000000           call     CORINFO_HELP_READYTORUN_GCSTATIC_BASE
       488B4008             mov      rax, gword ptr [rax+08H]
       4883C428             add      rsp, 40
       C3                   ret      
; Total bytes of code: 18

Now:

; Method ConsoleApp1.Program:GetField():System.String
       488B0500000000       mov      rax, bword ptr [(reloc 0x40000000004225c0)]
       488B4008             mov      rax, gword ptr [rax+08H]
       C3                   ret      
; Total bytes of code: 12

based on #63620 and @SingleAccretion's suggestions

@ghost ghost assigned EgorBo Jan 21, 2023
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 21, 2023
@ghost
Copy link

ghost commented Jan 21, 2023

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

Issue Details

Follow up to #79709 but for gc fields now, e.g.:

static string field;

static string GetField() => field;

Was:

; Method ConsoleApp1.Program:GetField():System.String
       4883EC28             sub      rsp, 40
       E800000000           call     CORINFO_HELP_READYTORUN_GCSTATIC_BASE
       488B4008             mov      rax, gword ptr [rax+08H]
       4883C428             add      rsp, 40
       C3                   ret      
; Total bytes of code: 18

Now:

; Method ConsoleApp1.Program:GetField():System.String
       488B0500000000       mov      rax, bword ptr [(reloc 0x40000000004225c0)]
       488B4008             mov      rax, gword ptr [rax+08H]
       C3                   ret      
; Total bytes of code: 12

based on #63620 and @SingleAccretion's suggestions

Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr
Milestone: -

@EgorBo EgorBo mentioned this pull request Jan 21, 2023
Closed
15 tasks
@EgorBo
Copy link
Member Author

EgorBo commented Jan 21, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo EgorBo marked this pull request as ready for review January 21, 2023 12:15
@EgorBo
Copy link
Member Author

EgorBo commented Jan 21, 2023

@SingleAccretion PTAL when you have time 🙂 CI looks good so far

@EgorBo
Copy link
Member Author

EgorBo commented Jan 21, 2023 •
edited
Loading

Failures are all Mono-related in runtime-extra-platforms: #80976 and likely dotnet/arcade#12266

SingleAccretion
SingleAccretion reviewed Jan 21, 2023
View reviewed changes
EgorBo and others added 2 commits January 21, 2023 18:23
@EgorBo @SingleAccretion
Apply suggestions from code review
e209ecf 
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo @SingleAccretion
Update src/coreclr/jit/gentree.h
e34e6fc 
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
SingleAccretion
SingleAccretion reviewed Jan 21, 2023
View reviewed changes
EgorBo and others added 2 commits January 21, 2023 18:29
@EgorBo
Update CorInfoImpl.RyuJit.cs
157f15d
@EgorBo @SingleAccretion
Update src/coreclr/jit/emit.cpp
a488079 
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
SingleAccretion
SingleAccretion approved these changes Jan 21, 2023
View reviewed changes
Copy link
Contributor

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

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

Jit part LGTM!

@EgorBo
Copy link
Member Author

EgorBo commented Jan 21, 2023 •
edited
Loading

@MichalStrehovsky PTAL NAOT side. runtime-extra-platforms NativeAOT tests all are green

@build-analysis build-analysis bot mentioned this pull request Jan 21, 2023
Closed
MichalStrehovsky
MichalStrehovsky approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

src/coreclr/jit/importer.cpp Outdated
Comment on lines 4251 to 4252
assert(pFieldInfo->fieldLookup.accessType == IAT_PVALUE);
op1 = gtNewIndOfIconHandleNode(TYP_BYREF, fldAddr, GTF_ICON_STATIC_ADDR_PTR, true);
Copy link
Member

Choose a reason for hiding this comment

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

Just to double check - typing this a TYP_BYREF and retyping it as TYP_I_IMPL when creating the GT_ADD below won't cause any issues?

Could we just type it as TYP_I_IMPL here? If I'm reading it right we're assuming it's pinned anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, TYP_I_IMPL works here to for the static base address (and as we discussed inhttps://github.com//pull/79709 it doesn't lead to constant folding) and won't generate unnecessary gc info. The root GT_IND node has TYP_REF for gc refs so everything should be fine.

@EgorBo EgorBo merged commit a39435f into dotnet:main Jan 23, 2023
@EgorBo EgorBo deleted the naot-static-gc-fields branch January 23, 2023 14:21
@runfoapp runfoapp bot mentioned this pull request Jan 23, 2023
Closed
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@EgorBo @SingleAccretion @mdh1418
NativeAOT: Optimize static fields of gc types (dotnet#80969)
f4426b9 
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Feb 8, 2023
Closed
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Feb 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

+1 more reviewer

@SingleAccretion SingleAccretion SingleAccretion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@EgorBo EgorBo

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@EgorBo @MichalStrehovsky @SingleAccretion