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

NativeAOT: avoid helper calls for static fields #79709

Merged
merged 14 commits into from
Jan 21, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Dec 15, 2022

Based on #63620 for non-gc fields for now (looking into gc now)

static int s;

static int GetS() => s;

Was:

       4883EC28             sub      rsp, 40
       E800000000           call     CORINFO_HELP_READYTORUN_NONGCSTATIC_BASE
       8B00                 mov      eax, dword ptr [rax]
       4883C428             add      rsp, 40
       C3                   ret
; Total bytes of code 16

Now:

       8B0500000000         mov      eax, dword ptr [(reloc 0x4000000000425290)]
       C3                   ret
; Total bytes of code 7

@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 Dec 15, 2022
@ghost ghost assigned EgorBo Dec 15, 2022
@ghost
Copy link

ghost commented Dec 15, 2022

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

Issue Details

Based on #63620 for non-gc fields for now (looking into gc now)

static int s;

static int GetS() => s;

Was:

       4883EC28             sub      rsp, 40
       E800000000           call     CORINFO_HELP_READYTORUN_NONGCSTATIC_BASE
       8B00                 mov      eax, dword ptr [rax]
       4883C428             add      rsp, 40
       C3                   ret
; Total bytes of code 16

Now:

       8B0500000000         mov      eax, dword ptr [(reloc 0x4000000000425290)]
       C3                   ret
; Total bytes of code 7
Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Dec 15, 2022

/azp list

@azure-pipelines

This comment was marked as outdated.

@EgorBo
Copy link
Member Author

EgorBo commented Dec 15, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@am11
Copy link
Member

am11 commented Dec 15, 2022

Nice! Does it help property initializers in R2R as well: https://godbolt.org/z/EvTjxcG9a (get_Bar())?

@EgorBo
Copy link
Member Author

EgorBo commented Dec 15, 2022

Nice! Does it help property initializers in R2R as well: https://godbolt.org/z/EvTjxcG9a (get_Bar())?

This is optimized to just 42 in NativeAOT now (in .NET 8.0) via some previous PR I merged 🙂

@EgorBo EgorBo marked this pull request as ready for review January 6, 2023 14:58
@EgorBo
Copy link
Member Author

EgorBo commented Jan 6, 2023

@jakobbotsch @dotnet/jit-contrib PTAL the JIT side, I'll handle statics of GC types separately due to additional complexity (double indirect + new type of const handle in jit for it).

@MichalStrehovsky PTAL NAOT side, it's copied from your PR 🙂

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.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/inc/corinfo.h Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
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.

The AOT side looks good!

I think we'd also be able to do the case when the bases are accessed from shared code if we changed pResult->fieldLookup from CORINFO_CONST_LOOKUP to CORINFO_LOOKUP.

The compiler side can compute the lookup with ComputeLookup(ref pResolvedToken, field.OwningType, ReadyToRunHelperId.GetNonGCStaticBase, ref pResult->fieldLookup).

It can also be done separately, just want to make sure the JIT side doesn't make assumptions that would make it more difficult.

@EgorBo
Copy link
Member Author

EgorBo commented Jan 9, 2023

@SingleAccretion thanks for the review, can you look again please?

src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/valuenum.cpp Outdated Show resolved Hide resolved
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo
Copy link
Member Author

EgorBo commented Jan 14, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Jan 15, 2023

@SingleAccretion can you please take another look?

Failures in runtime-extra-platforms don't look to be related as they fail in other PRs too - I'll file an issue if we don't have one already.

@EgorBo EgorBo merged commit e5dcd3c into dotnet:main Jan 21, 2023
@EgorBo EgorBo deleted the static-fields-nativeaot branch January 21, 2023 07:59
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Feb 20, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants