Allow inlining for ldsfld + value-type #78736
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIt turns out that JIT used to always give up on inlining methods with static DateTime Foo { get; } = DateTime.Now;
static DateTime Test() => Foo;Current Tier1 codegen for ; Assembly listing for method Prog:Test():System.DateTime
; Tier-1 compilation
4883EC28 sub rsp, 40
FF159E210E00 call [Prog:get_Foo():System.DateTime]
90 nop
4883C428 add rsp, 40
C3 ret
; Total bytes of code 16New codegen: ; Assembly listing for method Prog:Test():System.DateTime
; Tier-1 compilation
48B8D6105720E4CCDA88 mov rax, 0x88DACCE4205710D6
C3 ret
; Total bytes of code 11
|
|
It triggered too many inlinings some of them do not look profitable 😞 will see if I can narrow it down to cases I was interested in |
ghost
closed this
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
EgorBo
force-pushed
the
allow-inlining-ldsfld-vt
branch
from
cf99073 to
c93afc3
Compare
|
@AndyAyersMS @jakobbotsch PTAL, this PR does two things: First, it slightly relaxes the limitation we had "if we start the actual inlining and meet a static field load/store/addr of a struct type --> reject due to possible helper calls" - I relaxed it with "if field is static readonly and initied - allow". Example: static DateTime Foo { get; } = DateTime.Now;
static DateTime Test() => Foo;
; Assembly listing for method Prog:Test():System.DateTime
; Tier-1 compilation
48B8D6105720E4CCDA88 mov rax, 0x88DACCE4205710D6
C3 ret
; Total bytes of code 11Second, always allow inlining for cases where we currently give up due to potential helper calls (e.g. TLS field) if we have PGO data and the callsite is hot. [ThreadStatic]
private static int TLS;
static int GetTLS() => TLS;
[MethodImpl(MethodImplOptions.NoInlining)]
static int Test() => GetTLS();
; Assembly listing for method Prog:Test():int
G_M6836_IG01:
sub rsp, 40
G_M6836_IG02:
mov rcx, 0xD1FFAB1E
mov edx, 3
call CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE_NOCTOR
mov eax, dword ptr [rax+1CH]
G_M6836_IG03:
add rsp, 40
ret
; Total bytes of code 32Example of a size improvement: https://www.diffchecker.com/XzMrjiah ( jit-diffs (--cctors) |
|
Can you run pmi over all the framework assemblies too ( |
This comment was marked as outdated.
This comment was marked as outdated.
5 tasks
|
@AndyAyersMS ok i've pushed a change and made it conservative:
https://www.diffchecker.com/VOLamS3D/ |
EgorBo
force-pushed
the
allow-inlining-ldsfld-vt
branch
from
58f79e4 to
378e801
Compare
|
The main motivation for this is that I need it for |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out that JIT used to always give up on inlining methods with
ldsfldinside for fields of struct type, e.g:Current Tier1 codegen for
Test:New codegen: