-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
JIT: import static readonly fields holding frozen objects as const handles #76112
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsJIT is able to fold static string MyStr { get; } = "Hello";
static readonly Type s_type = typeof(int);
static void Foo()
{
Console.WriteLine(MyStr);
Console.WriteLine(s_type);
} Tier1 codegen diff for ; Assembly listing for method Foo()
; Tier-1 compilation
G_M14074_IG01:
4883EC28 sub rsp, 40
G_M14074_IG02:
- 48B9C01EC0CB9E020000 mov rcx, 0x29ECBC01EC0 ; const ptr
- 488B09 mov rcx, gword ptr [rcx]
+ 48B9B017954DDA010000 mov rcx, 0x1DA4D9517B0 ; 'Hello'
FF15B9073800 call [System.Console:WriteLine(System.String)]
- 48B9C81EC0CB9E020000 mov rcx, 0x29ECBC01EC8 ; const ptr
- 488B09 mov rcx, gword ptr [rcx]
+ 48B9D017954DDA010000 mov rcx, 0x1DA4D9517D0 ; 'System.Int32'
FF158E073800 call [System.Console:WriteLine(System.Object)]
90 nop
G_M14074_IG03:
4883C428 add rsp, 40
C3 ret Motivation
Jit-diffPR is verbose due to JIT-EE update, the actual changes are fairly small.
|
BTW: Storing frozen objects does not require write barrier. For example:
Does this optimization happen already? If not, we just need to skip the write barrier for frozen icon handles, similar how we skip them for nulls. |
Good point! Do you mind if I do it separately, I also wanted to e.g. check if it's worth optimizing pinning against frozen objects, etc. Separate commits also help to triage perf improvements/regressions |
Yes, it is fine to do it separately. |
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. Thank you!
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
…pl.RyuJit.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Looks like |
Sounds reasonable. |
CI is green except SPMI jobs (JIT-EE change), going to merge |
JIT is able to fold
static readonly
fields to constants for primitive types. This PR extends that to handle such fields pointing to frozen objects, example:Tier1 codegen diff for
Foo
:UPD: Also, this PR helps NativeAOT:
see #76112 (comment) thread
Motivation
static readonly Type .* = typeof
pattern found 211 matches in dotnet/runtime (tests are not included)Example: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs#L15-L18
static readonly string .* = "\w+";
found 119 results in dotnet/runtime (tests are not included)Array.Empty<T>
as was suggested by @jkotasJit-diff (
-f --pmi --cctors
)Regressions seem to be CSE related e.g. https://www.diffchecker.com/E3PieJCB + changed inlining decisions due to changes in gtGetClassHandle
PR is verbose due to JIT-EE update, the actual changes are fairly small.