Skip to content

NativeAOT: Partially expand static initialization - #83911

Merged
EgorBo merged 40 commits into
dotnet:mainfrom
EgorBo:expand-static-init
Apr 8, 2023
Merged

NativeAOT: Partially expand static initialization#83911
EgorBo merged 40 commits into
dotnet:mainfrom
EgorBo:expand-static-init

Conversation

@EgorBo

@EgorBoEgorBo commented Mar 24, 2023

Copy link
Copy Markdown
Member

Closes#80954 and contributes to #64242

In the JIT world we successfully get rid of static class initializations (either beforefieldinit or normal cctors) naturally or via tiered compilation (Tier1 means that we already initialized eveyrything we needed on hot path in the previous tier). However, it's not the case for AOT. The plan is to help NativeAOT to avoid hitting call overhead in this case by partially inline "is class already initialized?" check with a fast path, e.g.:

// non-GC staticstaticints_Int=int.Parse("42");intGetInt()=>s_Int;// GC staticstaticstrings_Str=(42).ToString();stringGetStr()=>s_Str;

Current codegen on NativeAOT:

; Method Prog:GetStr():System.String:thisG_M45136_IG01:  4883EC28 subrsp,40G_M45136_IG02:  E800000000 call CORINFO_HELP_READYTORUN_GCSTATIC_BASE 488B4008 movrax, gword ptr [rax+08H]G_M45136_IG03:  4883C428 addrsp,40 C3 ret; Total bytes of code: 18; Method Prog:GetInt():int:thisG_M29019_IG01:  4883EC28 subrsp,40G_M29019_IG02:  E800000000 call CORINFO_HELP_READYTORUN_NONGCSTATIC_BASE 8B00 moveax, dword ptr [rax]G_M29019_IG03:  4883C428 addrsp,40 C3 ret; Total bytes of code: 16

New codegen (NativeAOT):

; Method Prog:GetStr():System.String:this 4883EC28 subrsp,40 488D0500000000 learax,[(reloc 0x4000000000421128)] 8378F801 cmp dword ptr [rax-08H],17510jne SHORT G_M25482_IG05G_M25482_IG03:  488B0500000000 movrax, qword ptr [(reloc 0x4000000000421130)] 488B4008 movrax, gword ptr [rax+08H] 4883C428 addrsp,40 C3 retG_M25482_IG05:  E800000000 call CORINFO_HELP_READYTORUN_GCSTATIC_BASE EBE9 jmp SHORT G_M25482_IG03; Total bytes of code: 40; Method Prog:GetInt():int:this56pushrsi 4883EC20 subrsp,32 488D3500000000 learsi,[(reloc 0x4000000000421008)] 837EF801 cmp dword ptr [rsi-08H],17508jne SHORT G_M56961_IG05G_M56961_IG03:  8B06 moveax, dword ptr [rsi] 4883C420 addrsp,32 5E poprsi C3 retG_M56961_IG05:  E800000000 call CORINFO_HELP_READYTORUN_NONGCSTATIC_BASE EBF1 jmp SHORT G_M56961_IG03; Total bytes of code: 33

It also works for JIT (mainly, to test it, but could be useful for TC=0).

Benchmarks

I was using JIT with TieredCompilation=0

// case 1: normal cctor. JIT doesn't hoist such initializations// from loops (needs loop peeling)publicclassTestClassWithCctor{publicstaticintfield;staticTestClassWithCctor(){field=42;}}[Benchmark]publicintNonHoistableStaticInit(){intsum=0;for(inti=0;i<10000;i++){sum+=TestClassWithCctor.field;}returnsum;}// case 2: beforefieldinitpublicclassTestClassWithBeforefieldinit{publicstaticintfield=42;}[Benchmark]publicintSimpleStaticInit(){returnTestClassWithBeforefieldinit.field;}
MethodToolchainMean
NonHoistableStaticInit\runtime-base\corerun.exe11,123.0890 ns
NonHoistableStaticInit\runtime\corerun.exe1,893.7071 ns
SimpleStaticInit\runtime-base\corerun.exe0.9206 ns
SimpleStaticInit\runtime\corerun.exe0.0302 ns

codegen diff: https://www.diffchecker.com/87bWA1er/

Can't say yet the total size overhead for a hello world, in the worst case we can leave this optimization only for "prefer speed" mode (-Os)

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

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

Closes #80954 and contributes to #64242

In the JIT world we successfully get rid of static class initializations (either beforefieldinit or normal cctors) naturally or via tiered compilation (Tier1 means that we already initialized eveyrything we needed on hot path in the previous tier). However, it's not the case for AOT. The plan is to help NativeAOT to avoid hitting call overhead in this case by partially inline "is class already initialized?" check with a fast path, e.g.:

staticintfield=int.Parse("42");// mimic a complex cctorintTest(){returnfield;}

Current codegen on NativeAOT:

; Method Prog:Test():int:thissubrsp,40call CORINFO_HELP_READYTORUN_NONGCSTATIC_BASEmoveax, dword ptr [rax]addrsp,40ret

Expected codegen:

subrsp,40test byte ptr [(reloc)],1 ;; is already initialized?jne SHORT G_M3272_IG04call CORINFO_HELP_READYTORUN_NONGCSTATIC_BASE ;; it's not -- fallbackSHORT G_M3272_IG04:moveax, dword ptr [(reloc)] ;; just access field's valueaddrsp,40ret

The implementation in this PR only handles JIT for now just to test it, while I'm trying to figure out how to implement getIsClassInitedFieldAddress API on NativeAOT side, might need @MichalStrehovsky help

Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

Comment threadsrc/coreclr/jit/flowgraph.cpp Outdated
@jkotas

jkotas commented Mar 24, 2023

Copy link
Copy Markdown
Member

getIsClassInitedFieldAddress API on NativeAOT side,

factory.TypeNonGCStaticsSymbol(type) and subtract NonGCStaticsNode.GetClassConstructorContextSize(factory.Target) offset from it.

It should be compared with 1 (CoreCLR does bit test, native aot uses comparison).

@EgorBo

EgorBo commented Mar 25, 2023

Copy link
Copy Markdown
MemberAuthor

Managed to get NAOT working locally (not yet pushed), so for

staticintfield=int.Parse("42");// mimic a complex cctorintTest(){returnfield;}

Was:

; Method Prog:Test():int:thissubrsp,40call CORINFO_HELP_READYTORUN_NONGCSTATIC_BASEmoveax, dword ptr [rax]addrsp,40ret; Total bytes of code 16 

Now I get:

; Assembly listing for method Prog:Test():int:thissubrsp,40learax,[(reloc)]cmp dword ptr [rax-08H],0je SHORT G_M3272_IG05G_M3272_IG03:moveax, dword ptr [(reloc)]addrsp,40retG_M3272_IG05:call CORINFO_HELP_READYTORUN_NONGCSTATIC_BASEjmp SHORT G_M3272_IG03; Total bytes of code 35

(nongc statics)

@EgorBo

Copy link
Copy Markdown
MemberAuthor

Same but for JIT:

Was:

; Assembly listing for method Prog:Test():int:thisG_M3272_IG01:subrsp,40movrcx,0xD1FFAB1Emovedx,3call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASEmoveax, dword ptr [(reloc)]addrsp,40ret; Total bytes of code 35

New:

; Assembly listing for method Prog:Test():int:thissubrsp,40test byte ptr [(reloc)],1je SHORT G_M3272_IG05G_M3272_IG03:moveax, dword ptr [(reloc)]addrsp,40retG_M3272_IG05:movrcx,0xD1FFAB1Emovedx,3call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASEjmp SHORT G_M3272_IG03; Total bytes of code 46

Presumably, JIT version can be made 5 bytes smaller if we replace the cold part with "call INITCLASS(cls)".

@EgorBo

EgorBo commented Mar 25, 2023

Copy link
Copy Markdown
MemberAuthor

GC statics come with a bigger size increase on NAOT:

staticstringfield=(42).ToString();[MethodImpl(MethodImplOptions.NoInlining)]stringTest(){returnfield;}

Was:

; Method Prog:Test():System.String:thissubrsp,40call CORINFO_HELP_READYTORUN_GCSTATIC_BASEmovrax, gword ptr [rax+08H]addrsp,40ret; Total bytes of code: 18

Now:

; Method Prog:Test():System.String:thissubrsp,40learax,[(reloc)] ;; NonGCStaticsBasecmp dword ptr [rax-08H],0je SHORT G_M48517_IG05G_M48517_IG03:movrax, qword ptr [(reloc)] ;; this is a different reloc (GCStaticBase)movrax, gword ptr [rax+08H]addrsp,40retG_M48517_IG05:call CORINFO_HELP_READYTORUN_GCSTATIC_BASEjmp SHORT G_M48517_IG03; Total bytes of code: 40

due to double-indirect for field access

@EgorBo

EgorBo commented Mar 25, 2023

Copy link
Copy Markdown
MemberAuthor

Benchmarks (JIT, TieredCompilation=0):

// case 1: normal cctor. JIT doesn't hoist such initializations// from loops (needs loop peeling)publicclassTestClassWithCctor{publicstaticintfield;staticTestClassWithCctor(){field=42;}}[Benchmark]publicintNonHoistableStaticInit(){intsum=0;for(inti=0;i<10000;i++){sum+=TestClassWithCctor.field;}returnsum;}// case 2: beforefieldinitpublicclassTestClassWithBeforefieldinit{publicstaticintfield=42;}[Benchmark]publicintSimpleStaticInit(){returnTestClassWithBeforefieldinit.field;}
MethodToolchainMean
NonHoistableStaticInit\runtime-base\corerun.exe11,123.0890 ns
NonHoistableStaticInit\runtime\corerun.exe1,893.7071 ns
SimpleStaticInit\runtime-base\corerun.exe0.9206 ns
SimpleStaticInit\runtime\corerun.exe0.0302 ns

codegen diff: https://www.diffchecker.com/87bWA1er/

@EgorBo

Copy link
Copy Markdown
MemberAuthor

/azp list

@azure-pipelines

This comment was marked as outdated.

@EgorBo

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop, runtime-extra-platforms, runtime-coreclr jitstress

@azure-pipelines

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

@EgorBoEgorBo mentioned this pull request Apr 6, 2023
@EgorBo

Copy link
Copy Markdown
MemberAuthor

@kunalspathak@AndyAyersMS@SingleAccretion I've addressed feedback, anything else?

Comment threadsrc/coreclr/jit/flowgraph.cpp Outdated
Comment threadsrc/coreclr/jit/flowgraph.cpp Outdated
Comment threadsrc/coreclr/jit/flowgraph.cpp Outdated
Comment threadsrc/coreclr/jit/importer.cpp Outdated
Comment threadsrc/coreclr/jit/importer.cpp Outdated
EgorBoand others added 2 commits April 6, 2023 18:44
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo

Copy link
Copy Markdown
MemberAuthor

@SingleAccretion Thanks!

@kunalspathak@AndyAyersMS waiting for a green approve now 🙂

Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/jit/flowgraph.cpp
Comment threadsrc/coreclr/jit/flowgraph.cpp

@AndyAyersMSAndyAyersMS 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.

Would it make sense to have some kind of "post phase" check to make sure each possible candidate was either expanded or intentionally not expanded?

Comment threadsrc/coreclr/jit/flowgraph.cpp
Comment threadsrc/coreclr/jit/flowgraph.cpp Outdated
@EgorBo

EgorBo commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

Would it make sense to have some kind of "post phase" check to make sure each possible candidate was either expanded or intentionally not expanded?

E.g. for runtime lookups I put an assert in Lower.cpp to make sure all of them are expanded (because it was required). In this case I didn't do that because it's just an optimization and we skip some type of static initializations on JIT already + we skip cold blocks so I didn't do that

@kunalspathakkunalspathak 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.

LGTM

@EgorBo
EgorBo merged commit 4ec7db0 into dotnet:mainApr 8, 2023
@EgorBo
EgorBo deleted the expand-static-init branch April 8, 2023 08:10
@ghostghost locked as resolved and limited conversation to collaborators May 10, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NativeAOT: Inline static initialization checks in codegen

7 participants

@EgorBo@jkotas@MichalStrehovsky@AndyAyersMS@kunalspathak@markples@SingleAccretion