Skip to content

NativeAOT: Optimize static fields of gc types - #80969

Merged
EgorBo merged 7 commits into
dotnet:mainfrom
EgorBo:naot-static-gc-fields
Jan 23, 2023
Merged

NativeAOT: Optimize static fields of gc types#80969
EgorBo merged 7 commits into
dotnet:mainfrom
EgorBo:naot-static-gc-fields

Conversation

@EgorBo

@EgorBoEgorBo commented Jan 21, 2023

Copy link
Copy Markdown
Member

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

staticstringfield;staticstringGetField()=>field;

Was:

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

Now:

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

based on #63620 and @SingleAccretion's suggestions

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

ghost commented Jan 21, 2023

Copy link
Copy Markdown

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.:

staticstringfield;staticstringGetField()=>field;

Was:

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

Now:

; Method ConsoleApp1.Program:GetField():System.String 488B0500000000 movrax, bword ptr [(reloc 0x40000000004225c0)] 488B4008 movrax, 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:-

@EgorBoEgorBo mentioned this pull request Jan 21, 2023
Closed
15 tasks
@EgorBo

EgorBo commented Jan 21, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Jan 21, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

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

EgorBo commented Jan 21, 2023

Copy link
Copy Markdown
MemberAuthor

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

@EgorBo

EgorBo commented Jan 21, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

SingleAccretion
SingleAccretion reviewed Jan 21, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/gentree.h Outdated
Comment threadsrc/coreclr/jit/gentree.h Outdated
Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/jit/importer.cpp Outdated
Comment threadsrc/coreclr/jit/importer.cpp Outdated
Comment threadsrc/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs Outdated
Comment threadsrc/coreclr/jit/gentree.h Outdated
EgorBoand 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
Comment threadsrc/coreclr/jit/emit.cpp Outdated
EgorBoand 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

@SingleAccretionSingleAccretion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Jit part LGTM!

@EgorBo

EgorBo commented Jan 21, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

@build-analysisbuild-analysisBot mentioned this pull request Jan 21, 2023
Closed
MichalStrehovsky
MichalStrehovsky approved these changes Jan 23, 2023
View reviewed changes

@MichalStrehovskyMichalStrehovsky left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome, thank you!

Comment threadsrc/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);

@MichalStrehovskyMichalStrehovskyJan 23, 2023

Copy link
Copy Markdown
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.

@EgorBoEgorBoJan 23, 2023

Copy link
Copy Markdown
MemberAuthor

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:mainJan 23, 2023
@EgorBo
EgorBo deleted the naot-static-gc-fields branch January 23, 2023 14:21
@runfoapprunfoappBot 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>
@JulieLeeMSFTJulieLeeMSFT mentioned this pull request Feb 8, 2023
Closed
3 tasks
@ghostghost locked as resolved and limited conversation to collaborators Feb 22, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky approved these changes
+1 more reviewer
@SingleAccretionSingleAccretionSingleAccretion approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR 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