NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

Expand unboxing for Nullable<> in JIT#105073

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

EgorBo merged 6 commits intodotnet:mainfromEgorBo:nullable-expand

Jul 19, 2024

Merged

Expand unboxing for Nullable<> in JIT#105073

EgorBo merged 6 commits intodotnet:mainfromEgorBo:nullable-expand

Jul 19, 2024

Conversation

Copy link

Member

EgorBo commentedJul 18, 2024•
edited
Loading

Closes#104954

Currently, unboxing for Nullable<> always go through a helper call, while we can inline it in JIT.

int?Test(objecto)=>(int?)o;

Current codegen:

; Method Proga:Test(System.Object):System.Nullable`1[int]:this (FullOpts)G_M11066_IG01:subrsp,40G_M11066_IG02:learcx,[rsp+0x20]movr8,rdxmovrdx,0x7FF7E261B750      ; System.Nullable`1[int]call     CORINFO_HELP_UNBOX_NULLABLEmovrax, qword ptr[rsp+0x20]G_M11066_IG03:addrsp,40ret; Total bytes of code: 37

New codegen:

; Method Proga:Test(System.Object):System.Nullable`1[int]:this (FullOpts)G_M11066_IG01:subrsp,40G_M11066_IG02:testrdx,rdx ;; is null?je       SHORT G_M11066_IG04G_M11066_IG03:movrax,0x7FF7E23674D0      ; System.Int32cmp      qword ptr[rdx],rax ;; is object's type int32?je       SHORT G_M11066_IG05jmp      SHORT G_M11066_IG08G_M11066_IG04:xoredx,edxmov      qword ptr[rsp+0x20],rdxjmp      SHORT G_M11066_IG06G_M11066_IG05:mov      byte  ptr[rsp+0x20],1moveax, dword ptr[rdx+0x08]mov      dword ptr[rsp+0x24],eaxG_M11066_IG06:movrax, qword ptr[rsp+0x20]G_M11066_IG07:addrsp,40ret; cold pathG_M11066_IG08:learcx,[rsp+0x20]movr8,rdxmovrdx,0x7FF7E263B750      ; System.Nullable`1[int]call     CORINFO_HELP_UNBOX_NULLABLEjmp      SHORT G_M11066_IG06; Total bytes of code: 82

Benchmark:

usingBenchmarkDotNet.Attributes;publicclassMyBench{[Benchmark][Arguments(42)][Arguments(null)]publicint?Unbox(objecto)=>(int?)o;}

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)Unknown processor (which is "Ampere Altra")  Job-MMNLPR : .NET 9.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD  Job-QXOQTU : .NET 9.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD

Method	Toolchain	o	Mean	Error	Ratio
Unbox	Main	?	7.1195 ns	0.0098 ns	1.00
Unbox	PR	?	0.6698 ns	0.0072 ns	0.09

Unbox	Main	42	10.9406 ns	0.0175 ns	1.00
Unbox	PR	42	2.7002 ns	0.0001 ns	0.25

EgorBo added2 commits

July 18, 2024 11:18

Expand nullable unbox in JIT

99fb623

reorder blocks

49aca95

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

Jul 18, 2024

dotnet-policy-servicebot assignedEgorBo

Jul 18, 2024

Copy link

MemberAuthor

EgorBo commentedJul 18, 2024

@EgorBot -intel -arm64

usingBenchmarkDotNet.Attributes;publicclassMyBench{[Benchmark][Arguments(42)][Arguments(null)]publicint?Unbox(objecto)=>(int?)o;}

Copy link

Member

huoyaoyuan commentedJul 18, 2024

Does it worth to recognizeRuntimeHelpers.Unbox_Nullable and do the same expansion?

Or, can we write the method in C# and make helpers inlinable? This will help more around nullables.

Copy link

MemberAuthor

EgorBo commentedJul 18, 2024•
edited
Loading

Or, can we write the method in C# and make helpers inlinable? This will help more around nullables.

Yes, that is an option, requires some effort to make helpers inlineable, they're currently not. I decided to do it in JIT since the actual change is like 30 LOC, the rest are comments. If we move it to an inlineable helper we can delete this I guess..
I'll take a look at that next week probably.

Copy link

EgorBot commentedJul 18, 2024

Benchmark results on Intel

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 8 logical and 4 physical cores  Job-XHSURK : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI  Job-ZDCIDK : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Toolchain	o	Mean	Error	Ratio
Unbox	Main	?	4.4659 ns	0.0093 ns	1.00
Unbox	PR	?	0.5740 ns	0.0003 ns	0.13

Unbox	Main	42	7.1867 ns	0.0395 ns	1.00
Unbox	PR	42	3.9114 ns	0.0016 ns	0.54

BDN_Artifacts.zip

MihuBot mentioned this pull request

Jul 18, 2024

[JitDiff X64] [EgorBo] Expand unboxing for Nullable<> in JITMihuBot/runtime-utils#540

Open

Copy link

EgorBot commentedJul 18, 2024

Benchmark results on Arm64

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)Unknown processor  Job-MMNLPR : .NET 9.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD  Job-QXOQTU : .NET 9.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD

Method	Toolchain	o	Mean	Error	Ratio
Unbox	Main	?	7.1195 ns	0.0098 ns	1.00
Unbox	PR	?	0.6698 ns	0.0072 ns	0.09

Unbox	Main	42	10.9406 ns	0.0175 ns	1.00
Unbox	PR	42	2.7002 ns	0.0001 ns	0.25

BDN_Artifacts.zip

Only with optimizations and in hot blocks

9ef585b

Copy link

MemberAuthor

EgorBo commentedJul 18, 2024

@MihuBot

MihuBot mentioned this pull request

Jul 18, 2024

[JitDiff X64] [EgorBo] Expand unboxing for Nullable<> in JITMihuBot/runtime-utils#541

Open

EgorBo marked this pull request as ready for review

July 18, 2024 10:32

Copy link

MemberAuthor

EgorBo commentedJul 18, 2024

@jakobbotsch @AndyAyersMS cc @dotnet/jit-contrib PTAL, the actual change is 40 LOC, benchmarks attached. Jit-diff:MihuBot/runtime-utils#541 obviously, a size regression, but there were also a few cases where jit can now fold theif we emit here, e.g. previously we never tried to foldCORINFO_HELP_UNBOX_NULLABLE(null) etc.

EgorBo requested review fromAndyAyersMS andjakobbotsch

July 18, 2024 12:28

EgorBo commented

Jul 18, 2024

View reviewed changes

src/coreclr/jit/importer.cpp

		// Unbox the object to the result local:
		//
		// result._hasValue = true;
		// result._value = MethodTableLookup(obj);

Copy link

MemberAuthor

EgorBoJul 18, 2024

Choose a reason for hiding this comment

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

oops, wrong comment, it'sobj->RawData

build-analysisbot mentioned this pull request

Jul 18, 2024

Assertion failed(pThread->m_StateNC & Thread::TSNC_OwnsSpinLock) == 0#104449

Closed

AndyAyersMS approved these changes

Jul 18, 2024

View reviewed changes

Copy link

Member

AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Would it be worth limiting this to relatively small types, since will create both a zeroing and a copy?

Copy link

MemberAuthor

EgorBo commentedJul 18, 2024•
edited
Loading

Might be a good idea to limit it, yeah

@EgorBot -intel -arm64

usingSystem.Runtime.CompilerServices;usingBenchmarkDotNet.Attributes;publicclassMyBench2{[InlineArray(100)]publicstructLargeStruct{publiclongFld;}[Benchmark][Arguments(42)][Arguments(null)]publicLargeStruct?Unbox(objecto)=>(LargeStruct?)o;}

dotnet deleted a comment fromEgorBot

Jul 18, 2024

dotnet deleted a comment fromEgorBot

Jul 18, 2024

Copy link

EgorBot commentedJul 18, 2024

Benchmark results on Intel

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 8 logical and 4 physical cores  Job-BUSFSY : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI  Job-NIMINM : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Toolchain	o	Mean	Error	Ratio
Unbox	Main	?	27.91 ns	0.013 ns	1.00
Unbox	PR	?	20.32 ns	0.010 ns	0.73

Unbox	Main	42	NA	NA	?
Unbox	PR	42	NA	NA	?
Benchmarks with issues:
MyBench2.Unbox: Job-BUSFSY(Toolchain=Main) [o=42]
MyBench2.Unbox: Job-NIMINM(Toolchain=PR) [o=42]

BDN_Artifacts.zip

Copy link

MemberAuthor

EgorBo commentedJul 18, 2024

@EgorBot -intel -arm64

usingSystem.Runtime.CompilerServices;usingBenchmarkDotNet.Attributes;publicclassMyBench2{[InlineArray(100)]publicstructLargeStruct{publiclongFld;}publicIEnumerable<object>TestData(){yieldreturnnull;yieldreturnnewLargeStruct();}[Benchmark][ArgumentsSource(nameof(TestData))]publicLargeStruct?Unbox(objecto)=>(LargeStruct?)o;}

Copy link

EgorBot commentedJul 18, 2024

Benchmark results on Intel

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 8 logical and 4 physical cores  Job-GNOQYR : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI  Job-LOMSQK : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Toolchain	o	Mean	Error	Ratio
Unbox	Main	?	31.62 ns	0.038 ns	1.00
Unbox	PR	?	21.39 ns	0.013 ns	0.68

Unbox	Main	MyBench2+LargeStruct	29.85 ns	0.010 ns	1.00
Unbox	PR	MyBench2+LargeStruct	26.19 ns	0.005 ns	0.88

BDN_Artifacts.zip

Copy link

EgorBot commentedJul 18, 2024

Benchmark results on Arm64

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)Unknown processor  Job-WWWDAN : .NET 9.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD  Job-YJHBTM : .NET 9.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD

Method	Toolchain	o	Mean	Error	Ratio
Unbox	Main	?	89.28 ns	0.041 ns	1.00
Unbox	PR	?	39.46 ns	0.023 ns	0.44

Unbox	Main	MyBench2+LargeStruct	48.70 ns	0.052 ns	1.00
Unbox	PR	MyBench2+LargeStruct	41.85 ns	0.209 ns	0.86