Import isinst/castclass Nullable as underlying type by huoyaoyuan · Pull Request #87241 · dotnet/runtime

huoyaoyuan · 2023-06-08T01:49:14Z

According to ECMA-335 III.4.6:

If typeTok is a non-nullable value type or a generic parameter type it is interpreted
as “boxed” typeTok. If typeTok is a nullable type, Nullable<T>, it is interpreted as
“boxed” T.

In my understanding, it means that isinst V? is the same as isinst V.

Code gen:

boolTestNullable(objecto)=>oisint?;intTestCoalesce(objecto)=>(oasint?)??1234;

Before:

; Method CSPlayground.Program:TestNullable(System.Object):bool:thisG_M50267_IG01: ;; offset=0000Hsubrsp,40 ;; size=4 bbWeight=1 PerfScore 0.25G_M50267_IG02: ;; offset=0004Hmovrcx,0x7FF8A0D4A9B8 ; System.Nullable`1[int]call[CORINFO_HELP_ISINSTANCEOFANY]testrax,rax setne almovzxrax,al ;; size=25 bbWeight=1 PerfScore 4.75G_M50267_IG03: ;; offset=001DHaddrsp,40ret ;; size=5 bbWeight=1 PerfScore 1.25; Total bytes of code: 34; Method CSPlayground.Program:TestCoalesce(System.Object):int:thisG_M24774_IG01: ;; offset=0000Hsubrsp,40xoreax,eaxmov qword ptr [rsp+20H],rax ;; size=11 bbWeight=1 PerfScore 1.50G_M24774_IG02: ;; offset=000BHmovrcx,0x7FF8A0D1A9B8 ; System.Nullable`1[int]call[CORINFO_HELP_ISINSTANCEOFANY]movr8,raxlearcx,[rsp+20H]movrdx,0x7FF8A0D1A9B8 ; System.Nullable`1[int]call CORINFO_HELP_UNBOX_NULLABLEmoveax,0x4D2cmp byte ptr [rsp+20H],0 cmovne eax, dword ptr [rsp+24H] ;; size=54 bbWeight=1 PerfScore 9.50G_M24774_IG03: ;; offset=0041Haddrsp,40ret ;; size=5 bbWeight=1 PerfScore 1.25; Total bytes of code: 70

After:

; Method CSPlayground.Program:TestNullable(System.Object):bool:thisG_M50267_IG01: ;; offset=0000H ;; size=0 bbWeight=1 PerfScore 0.00G_M50267_IG02: ;; offset=0000Htestrdx,rdxje SHORT G_M50267_IG04 ;; size=5 bbWeight=1 PerfScore 1.25G_M50267_IG03: ;; offset=0005Hmovrax,0x7FF89421E7E8 ; System.Int32xorrcx,rcxcmp qword ptr [rdx],rax cmovne rdx,rcx ;; size=19 bbWeight=0.25 PerfScore 0.94G_M50267_IG04: ;; offset=0018Hxoreax,eaxtestrdx,rdx setne al ;; size=8 bbWeight=1 PerfScore 1.50G_M50267_IG05: ;; offset=0020Hret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code: 33; Method CSPlayground.Program:TestCoalesce(System.Object):int:thisG_M24774_IG01: ;; offset=0000Hsubrsp,40xoreax,eaxmov qword ptr [rsp+20H],rax ;; size=11 bbWeight=1 PerfScore 1.50G_M24774_IG02: ;; offset=000BHmovr8,rdxtestr8,r8je SHORT G_M24774_IG04 ;; size=8 bbWeight=1 PerfScore 1.50G_M24774_IG03: ;; offset=0013Hmovrcx,0x7FF89420E7E8 ; System.Int32xorrdx,rdxcmp qword ptr [r8],rcx cmovne r8,rdx ;; size=19 bbWeight=0.25 PerfScore 0.94G_M24774_IG04: ;; offset=0026Hlearcx,[rsp+20H]movrdx,0x7FF89451A9B8 ; System.Nullable`1[int]call CORINFO_HELP_UNBOX_NULLABLEmoveax,0x4D2cmp byte ptr [rsp+20H],0 cmovne eax, dword ptr [rsp+24H] ;; size=35 bbWeight=1 PerfScore 6.00G_M24774_IG05: ;; offset=0049Haddrsp,40ret ;; size=5 bbWeight=1 PerfScore 1.25; Total bytes of code: 78

Still not ideal (#47920 (comment)), but reduces a helper call.

ghost · 2023-06-08T01:49:25Z

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

Issue Details

According to ECMA-335 III.4.6:

If typeTok is a non-nullable value type or a generic parameter type it is interpreted
as “boxed” typeTok. If typeTok is a nullable type, Nullable<T>, it is interpreted as
“boxed” T.

In my understanding, it means that isinst V? is the same as isinst V.

Code gen:

boolTestNullable(objecto)=>oisint?;intTestCoalesce(objecto)=>(oasint?)??1234;

Before:

; Method CSPlayground.Program:TestNullable(System.Object):bool:thisG_M50267_IG01: ;; offset=0000Hsubrsp,40 ;; size=4 bbWeight=1 PerfScore 0.25G_M50267_IG02: ;; offset=0004Hmovrcx,0x7FF8A0D4A9B8 ; System.Nullable`1[int]call[CORINFO_HELP_ISINSTANCEOFANY]testrax,rax setne almovzxrax,al ;; size=25 bbWeight=1 PerfScore 4.75G_M50267_IG03: ;; offset=001DHaddrsp,40ret ;; size=5 bbWeight=1 PerfScore 1.25; Total bytes of code: 34; Method CSPlayground.Program:TestCoalesce(System.Object):int:thisG_M24774_IG01: ;; offset=0000Hsubrsp,40xoreax,eaxmov qword ptr [rsp+20H],rax ;; size=11 bbWeight=1 PerfScore 1.50G_M24774_IG02: ;; offset=000BHmovrcx,0x7FF8A0D1A9B8 ; System.Nullable`1[int]call[CORINFO_HELP_ISINSTANCEOFANY]movr8,raxlearcx,[rsp+20H]movrdx,0x7FF8A0D1A9B8 ; System.Nullable`1[int]call CORINFO_HELP_UNBOX_NULLABLEmoveax,0x4D2cmp byte ptr [rsp+20H],0 cmovne eax, dword ptr [rsp+24H] ;; size=54 bbWeight=1 PerfScore 9.50G_M24774_IG03: ;; offset=0041Haddrsp,40ret ;; size=5 bbWeight=1 PerfScore 1.25; Total bytes of code: 70

After:

; Method CSPlayground.Program:TestNullable(System.Object):bool:thisG_M50267_IG01: ;; offset=0000H ;; size=0 bbWeight=1 PerfScore 0.00G_M50267_IG02: ;; offset=0000Htestrdx,rdxje SHORT G_M50267_IG04 ;; size=5 bbWeight=1 PerfScore 1.25G_M50267_IG03: ;; offset=0005Hmovrax,0x7FF89421E7E8 ; System.Int32xorrcx,rcxcmp qword ptr [rdx],rax cmovne rdx,rcx ;; size=19 bbWeight=0.25 PerfScore 0.94G_M50267_IG04: ;; offset=0018Hxoreax,eaxtestrdx,rdx setne al ;; size=8 bbWeight=1 PerfScore 1.50G_M50267_IG05: ;; offset=0020Hret ;; size=1 bbWeight=1 PerfScore 1.00; Total bytes of code: 33; Method CSPlayground.Program:TestCoalesce(System.Object):int:thisG_M24774_IG01: ;; offset=0000Hsubrsp,40xoreax,eaxmov qword ptr [rsp+20H],rax ;; size=11 bbWeight=1 PerfScore 1.50G_M24774_IG02: ;; offset=000BHmovr8,rdxtestr8,r8je SHORT G_M24774_IG04 ;; size=8 bbWeight=1 PerfScore 1.50G_M24774_IG03: ;; offset=0013Hmovrcx,0x7FF89420E7E8 ; System.Int32xorrdx,rdxcmp qword ptr [r8],rcx cmovne r8,rdx ;; size=19 bbWeight=0.25 PerfScore 0.94G_M24774_IG04: ;; offset=0026Hlearcx,[rsp+20H]movrdx,0x7FF89451A9B8 ; System.Nullable`1[int]call CORINFO_HELP_UNBOX_NULLABLEmoveax,0x4D2cmp byte ptr [rsp+20H],0 cmovne eax, dword ptr [rsp+24H] ;; size=35 bbWeight=1 PerfScore 6.00G_M24774_IG05: ;; offset=0049Haddrsp,40ret ;; size=5 bbWeight=1 PerfScore 1.25; Total bytes of code: 78

Still not ideal (#47920 (comment)), but reduces a helper call.

Author:	huoyaoyuan
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

huoyaoyuan · 2023-06-08T01:49:58Z

+ if (!clsHnd.IsTypeDesc() && !(Nullable::IsNullableType(clsHnd) && fThrowing))
 {
 // If it is a non-variant class, use the fast class helper
+ // Also use fast helper for isinst Nullable, which is equal to isinst of underlying type


I'm not sure where to handle this. JIT is passing the token of original nullable type. Is there any way to get the token of underlying type?

Is there any way to get the token of underlying type?
Do you mean to get int from Nullable<int> ? in that case it should be clsHnd->AsMethodTable()->GetInstantiation()[0]

I mean to get it in JIT instead of VM.
I didn't update jitinterface for native aot (if exists). If done in JIT, there's no need to update in two places.

um.. getTypeForBox that you already using?

It returns a VM handle, but the JIT interface requires a metadata handle, if I'm correct.
Or we can skip the call to JIT interface and hard code to exact type helper when nullable case detected. Is there any risk?

Presumably, you need to check that you don't deal with Nullable<MyValueType<_Canon>> (or emit a runtime type lookup)

publicclassProgram{publicstaticvoidMain(){Test<string>(newMyStruct<string>());}[MethodImpl(MethodImplOptions.NoInlining)]staticboolTest<T>(objecto)=>oisMyStruct<T>?;}publicstructMyStruct<T>{publicTval;}

Solutions I can see:
Call getCastingHelperStatic again after getCastingHelper. Is the call to classMustBeLoadedBeforeCodeIsRun reliable to mark for underlying type too?
Find a way to convert CORINFO_RESOLVED_TOKEN to underlying type
Update JIT interface and tell getCastingHelper to use underlying type.

Since the implementation of getCastingHelper are all converting it to vm handle immediately, I think it's better to update jit interface to accept CORINFO_CLASS_HANDLE.

huoyaoyuan · 2023-06-08T01:52:12Z

~~Oops, forgot the null check difference. Converting to draft until I handle it.~~

Null check difference is for as unboxing. null is int? is false.

EgorBo · 2023-06-08T09:07:45Z

CI failures look related?

EgorBo · 2023-06-08T09:20:44Z

The codegen for as looks a bit weird to me. Basically, it is expected to be:

if(obj!=null&&obj->pMT==int32)returnNullable<int>{hasValue=true,Value=unboxedValu}
else
fallback

huoyaoyuan · 2023-06-08T15:04:59Z

 assert(lclDsc->lvSingleDef == 0);
 lclDsc->lvSingleDef = 1;
 JITDUMP("Marked V%02u as a single def temp\n", tmp);
- lvaSetClass(tmp, pResolvedToken->hClass);


Is there any observable effect for changing the return type of the expression?

jkotas · 2023-06-08T15:40:53Z

 {
 assert(op1->TypeGet() == TYP_REF);

+ // Import isinst Nullable<V> as isinst V


If all you want to do is to convert Nullable<T> into T for casing helpers, the best place to do it is resolveToken on the VM side. resolveToken on VM side does similar conversion for NewArr here: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/jitinterface.cpp#L1170. You can do similar conversion for tokens that come from isinst/castclass.

The conversion has to be also taken into account when embedding the handle (look for other places that check for CORINFO_TOKENKIND_Newarr in the VM). I think your current change is missing this part.

There's null check difference for castclass, so the nullable type should be visible to JIT if we want to do optimization for castclass.

There's null check difference for castclass
Could you please elaborate? Both isinst on null and castclass on null return null.

I misunderstood. NRE for casting to value type is thrown by unbox.

jkotas · 2023-06-09T04:33:01Z

 break;

+ case CORINFO_TOKENKIND_Casting:
+ // Disallow ELEMENT_TYPE_BYREF and ELEMENT_TYPE_VOID


This is duplicated for 3rd time here. We should move it in front of the switch under if (tokenType != CORINFO_TOKENKIND_Ldtoken)

huoyaoyuan · 2023-06-09T10:42:26Z

It seems that roslyn doesn't emit castclass for value type. The following C++/CLI emits castclass:

int^ Test(Object^ o)
{
return safe_cast<int^>(o);
}

.method public hidebysig instance class [mscorlib]System.ValueType modopt([mscorlib]System.Int32) modopt([mscorlib]System.Runtime.CompilerServices.IsBoxed) Test (
object o
) cil managed {
// Method begins at RVA 0x10e5c
// Header size: 12
// Code size: 7 (0x7)
.maxstack 1
IL_0000: ldarg.1
IL_0001: castclass [mscorlib]System.Int32
IL_0006: ret
} // end of method Class1::Test

It's quite complicated to verify it since /clr:netcore is not working for me. Do we have IL coverage in jit test?

EgorBo · 2023-06-09T13:04:41Z

Do we have IL coverage in jit test?

yes, look for *.ilproj in the tests folder

huoyaoyuan · 2023-06-09T13:17:17Z

yes, look for *.ilproj in the tests folder

Yes I know that folder. It seems there's currently no cases covering castclass valuetype. Where could such tests be put into? Directed/Convert? IL_Conformance?

jkotas · 2023-06-10T05:04:17Z

- COMPlusThrow(kInvalidProgramException);
- break;
+ default:
+ // No additional checks. Satisfy switch exhaustiveness check.


Suggested change
// No additional checks. Satisfy switch exhaustiveness check.
Nit. I do not think this comment is useful.

jkotas · 2023-06-10T05:06:32Z

Where could such tests be put into? Directed/Convert? IL_Conformance?

JIT\opt\Casts may be a good place.

jkotas · 2023-06-15T23:46:47Z

use pRuntimeLookup in impTokenToHandle to determine whether it's a generic parameter.

I think you would want to check CORINFO_FLG_SHAREDINST flag instead, before impTokenToHandle is called.

jkotas · 2023-06-16T00:19:22Z

Would this have other side effects?

I do not see problems with this, as long as you avoid using the original RESOLVED_TOKEN after updating the class handle.

jkotas · 2023-06-16T23:13:12Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-06-16T23:13:28Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2023-06-16T23:14:02Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-06-16T23:14:21Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2023-06-17T03:25:21Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-06-17T03:25:32Z

Azure Pipelines successfully started running 1 pipeline(s).

huoyaoyuan · 2023-06-17T09:54:23Z

Test failure should be the known one.

jkotas · 2023-06-18T05:21:08Z

 // Optimistically assume the jit should expand this as an inline test
 bool shouldExpandInline = true;
 bool isClassExact = impIsClassExact(pResolvedToken->hClass);
+ bool isSharedInst = info.compCompHnd->getClassAttribs(pResolvedToken->hClass) & CORINFO_FLG_SHAREDINST;


We can avoid the JIT/EE interface call for !isClassExact case by inlining the JIT/EE interface into the condition below.

jkotas

LGTM otherwise. @dotnet/jit-contrib Any addition feedback?

EgorBo · 2023-06-19T21:35:11Z

+{
+ .method public hidebysig static int32 Main() cil managed
+ {
+ .custom instance void [xunit.core]Xunit.FactAttribute::.ctor() = (


I assume the [Fact] attribute is not needed here (or then .entrypoint is not needed if it's part of the merged runtime tests)?
I'm not asking to remove it, just curious

[Fact] is needed as JIT/opt is a merged test group. The .entrypoint will have no impact on merged test group execution (this test is a library that is referenced by the merged test group, so this .entrypoint isn't the overall entry).
We've been keeping the .entrypoint annotations in ilproj tests. I'm not entirely sure of the details, but my guess is that the basic wrappers (for BuildAsStandalone or RequiresProcessIsolation) have C#-specific aspects to them.

@markples I see, thanks, I thought that .entrypoint will confuse a merged test group

EgorBo · 2023-06-19T21:40:46Z

LGTM assuming the newly added outerloop test passed during manual azp run outerloop checks

I've restarted the failing CI job

huoyaoyuan · 2023-06-21T03:14:04Z

@jkotas anything remaining?

jkotas · 2023-06-21T04:19:15Z

Thank you!

Import isinst Nullable as underlying type
168f833

ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Jun 8, 2023

huoyaoyuan commented Jun 8, 2023
View reviewed changes

huoyaoyuan marked this pull request as draft June 8, 2023 01:52

huoyaoyuan marked this pull request as ready for review June 8, 2023 01:53

build-analysisBot mentioned this pull request Jun 8, 2023
[mono] Tests failing with Failed to find compatible device: arm64-v8a#87267
Closed

Update JIT interface for getCastingHelper
e0cd0e2

huoyaoyuan requested a review from MichalStrehovsky as a code owner June 8, 2023 15:00

huoyaoyuan commented Jun 8, 2023
View reviewed changes

jkotas reviewed Jun 8, 2023
View reviewed changes

build-analysisBot mentioned this pull request Jun 8, 2023
Tracking issue for CI build timeouts #76454
Closed

huoyaoyuan added 2 commits June 9, 2023 11:03

Revert
8522424

Resolve nullable as underlying type when CORINFO_TOKENKIND_Casting
c722484

jkotas reviewed Jun 9, 2023
View reviewed changes

This was referenced Jun 9, 2023
NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723
Open
Official runtime builds for .NET 7 and 8 frequently failing during artifact upload dotnet/arcade#12936
Closed

huoyaoyuan added 2 commits June 9, 2023 18:47

Simplify token validation
8e5392a

Switch exhaustiveness
3ddd0ea

build-analysisBot mentioned this pull request Jun 9, 2023
Could not load file or assembly 'Microsoft.CodeAnalysis.NetAnalyzers #84995
Closed

jkotas reviewed Jun 10, 2023
View reviewed changes

IL test coverage for castclass
3e58229

huoyaoyuan added 2 commits June 16, 2023 18:40

Revert implementation
85ba6f2

Implement at JIT side
2b10425

huoyaoyuan force-pushed the isinst-nullable branch from a15e668 to 2b10425Compare June 16, 2023 13:00

Fix IL test
3855226

jkotas reviewed Jun 18, 2023
View reviewed changes

jkotas approved these changes Jun 18, 2023
View reviewed changes

Avoid unnecessary JIT to EE call
e71042b

BruceForstall requested a review from EgorBo June 19, 2023 21:31

EgorBo reviewed Jun 19, 2023
View reviewed changes

EgorBo approved these changes Jun 19, 2023
View reviewed changes

jkotas merged commit 74196a0 into dotnet:mainJun 21, 2023

huoyaoyuan deleted the isinst-nullable branch June 21, 2023 04:40

JulieLeeMSFT mentioned this pull request Jul 5, 2023
What's new in .NET 8 Preview 6 [WIP] dotnet/core#8437
Closed
3 tasks

ghost locked as resolved and limited conversation to collaborators Jul 21, 2023

Uh oh!

Conversation

huoyaoyuan commented Jun 8, 2023

Uh oh!

ghost commented Jun 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBoJun 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Jun 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Jun 8, 2023

Uh oh!

EgorBo commented Jun 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Jun 9, 2023

Uh oh!

EgorBo commented Jun 9, 2023

Uh oh!

huoyaoyuan commented Jun 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jun 10, 2023

Uh oh!

jkotas commented Jun 15, 2023

Uh oh!

jkotas commented Jun 16, 2023

Uh oh!

jkotas commented Jun 16, 2023

Uh oh!

azure-pipelinesBot commented Jun 16, 2023

Uh oh!

jkotas commented Jun 16, 2023

Uh oh!

azure-pipelinesBot commented Jun 16, 2023

Uh oh!

jkotas commented Jun 17, 2023

Uh oh!

azure-pipelinesBot commented Jun 17, 2023

Uh oh!

huoyaoyuan commented Jun 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

EgorBoJun 8, 2023•
edited
Loading

huoyaoyuan commented Jun 8, 2023•
edited
Loading

EgorBo commented Jun 8, 2023•
edited
Loading

EgorBo commented Jun 19, 2023•
edited
Loading