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

Replace default(T) != null with typeof(T).IsValueType #32098

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

GrabYourPitchforks
Copy link
Member

Builds on top of the optimizations done at #1157.

Replaces default(T) != null with typeof(T).IsValueType when: (a) the code targets netcoreapp5.0+; and (b) the caller intended to use this pattern as a cheap "is value type?" check and isn't intending to special-case nullable types.

@stephentoub
Copy link
Member

stephentoub commented Feb 11, 2020

Does this have any negative impact on tier 0 code that might make start-up slower? (Presumably R2R shared framework images will employ the optimization and so are fine.)

And I assume the new asm in these cases is as good / better than what it was before the change?

@GrabYourPitchforks
Copy link
Member Author

Does this have any negative impact on tier 0 code that might make start-up slower?

I'm not really qualified to answer this. @dotnet/jit-contrib, thoughts?

@GrabYourPitchforks
Copy link
Member Author

And I assume the new asm in these cases is as good / better than what it was before the change?

As far as I can tell, yeah.

[Benchmark]
public int TestNullableInt() => Array.Empty<int?>().AsSpan().Length;
; before
00007ffd`54b7a210 4883ec28        sub     rsp,28h
00007ffd`54b7a214 33c0            xor     eax,eax
00007ffd`54b7a216 4889442420      mov     qword ptr [rsp+20h],rax
00007ffd`54b7a21b 48b8782d009019020000 mov rax,21990002D78h
00007ffd`54b7a225 488b00          mov     rax,qword ptr [rax]
00007ffd`54b7a228 4885c0          test    rax,rax
00007ffd`54b7a22b 7504            jne     ConsoleAppBenchmark!ConsoleAppBenchmark.DefaultOfTRunner.Test()+0x1d01 (00007ffd`54b7a231)
00007ffd`54b7a22d 33c0            xor     eax,eax
00007ffd`54b7a22f eb20            jmp     ConsoleAppBenchmark!ConsoleAppBenchmark.DefaultOfTRunner.Test()+0x1d21 (00007ffd`54b7a251)
00007ffd`54b7a231 33d2            xor     edx,edx
00007ffd`54b7a233 4889542420      mov     qword ptr [rsp+20h],rdx
00007ffd`54b7a238 807c242000      cmp     byte ptr [rsp+20h],0
00007ffd`54b7a23d 750f            jne     ConsoleAppBenchmark!ConsoleAppBenchmark.DefaultOfTRunner.Test()+0x1d1e (00007ffd`54b7a24e)
00007ffd`54b7a23f 48bac8aec054fd7f0000 mov rdx,7FFD54C0AEC8h
00007ffd`54b7a249 483910          cmp     qword ptr [rax],rdx
00007ffd`54b7a24c 7508            jne     ConsoleAppBenchmark!ConsoleAppBenchmark.DefaultOfTRunner.Test()+0x1d26 (00007ffd`54b7a256)
00007ffd`54b7a24e 8b4008          mov     eax,dword ptr [rax+8]
00007ffd`54b7a251 4883c428        add     rsp,28h
00007ffd`54b7a255 c3              ret

; after
00007ffd`517ba2e0 48b8782ddb1594010000 mov rax,19415DB2D78h
00007ffd`517ba2ea 488b00          mov     rax,qword ptr [rax]
00007ffd`517ba2ed 4885c0          test    rax,rax
00007ffd`517ba2f0 7504            jne     ConsoleAppBenchmark!ConsoleAppBenchmark.DefaultOfTRunner.Test()+0x20e6 (00007ffd`517ba2f6)
00007ffd`517ba2f2 33c0            xor     eax,eax
00007ffd`517ba2f4 eb03            jmp     ConsoleAppBenchmark!ConsoleAppBenchmark.DefaultOfTRunner.Test()+0x20e9 (00007ffd`517ba2f9)
00007ffd`517ba2f6 8b4008          mov     eax,dword ptr [rax+8]
00007ffd`517ba2f9 c3              ret

@CarolEidt
Copy link
Contributor

Does this have any negative impact on tier 0 code that might make start-up slower?

I assume that the question pertains to the quality of the generated code, which can be seen by compiling with COMPlus_JitMinOpts=1.

@stephentoub
Copy link
Member

I assume that the question pertains to the quality of the generated code

Effectively, yes, whether typeof(T).IsValueType is going to be treated as an intrinsic and substituted for even in min opts, or whether by adding a bunch of these calls we're actually going to be doing more reflection and invoking IsValueTypeImpl on a bunch more code paths. If it's the former, great.

@GrabYourPitchforks
Copy link
Member Author

I believe that since Type.IsValueType is marked [Intrinsic], the importer code always performs this substitution and burns the constant into the codegen, even in Tier 0. @EgorBo can you confirm?

@jkotas
Copy link
Member

jkotas commented Feb 12, 2020

the importer code always performs this substitution and burns the constant into the codegen, even in Tier 0

That is not correct. The intrinsics are recognized only with optimizations on by default.

@AndyAyersMS
Copy link
Member

typeof(T).IsValueType is going to be treated as an intrinsic and substituted for even in min opts

(as Jan's just commented) Intrinsics are generally not expanded in Tier0 compilation (aka min opts).

default(T) == null for non-nullable T is fully optimized via the box pattern match which does happen at Tier0.

So for some of the cases in this PR, this change would cause regressions in Tier0 code.

@jkotas
Copy link
Member

jkotas commented Feb 12, 2020

The Tier0 code will be bigger and slower with this, but I do not think that it is a problem. And it is not going to allocate repeatedly, so it is not going to show up on the radar for folks who look at GC allocations.

@GrabYourPitchforks
Copy link
Member Author

CI failures seem to be known issue #32126.

@GrabYourPitchforks GrabYourPitchforks merged commit fff8f5a into dotnet:master Feb 12, 2020
@GrabYourPitchforks GrabYourPitchforks deleted the default_t branch February 12, 2020 02:35
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants