Optimize GetType with known type

[MichalPetryka]

Should help with the codegen in this simple method with C being a sealed type:

; Assembly listing for method Program:A(C):System.Type
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0000H
       sub      rsp, 40
 
G_M000_IG02:                ;; offset=0004H
       call     System.Object:GetType():System.Type:this
       nop      
 
G_M000_IG03:                ;; offset=000AH
       add      rsp, 40
       ret      
 
; Total bytes of code 15

[ghost]

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

Issue Details

---

Should help with the codegen in this simple method with C being a sealed type:

; Assembly listing for method Program:A(C):System.Type
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0000H
       sub      rsp, 40
 
G_M000_IG02:                ;; offset=0004H
       call     System.Object:GetType():System.Type:this
       nop      
 
G_M000_IG03:                ;; offset=000AH
       add      rsp, 40
       ret      
 
; Total bytes of code 15

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

[EgorBo]

How is this different from #87101 except doing the work earlier?

[MichalPetryka]

How is this different from #87101 except doing the work earlier?

I missed that change when I originally opened this PR (I only checked if the latest preview did this optimization and it didn't) and when @SingleAccretion told me about it, I've decided to let the jobs run to see if SPMI shows any meaningful diffs cause of running earlier which allows folding of stuff like IsKnownConstant and since this also handles isNonNull being false unlike your change, which then has surfaced the issue of gtGetClassHandle returning types that are CORINFO_FLG_GENERIC_TYPE_VARIABLE which should be debugged anyway (and also that second NAOT only failure that should be checked too).

[EgorBo]

since this also handles isNonNull being false unlike your change

That can be handled in VN in my change by adding a nullref exceptionset I guess, I just didn't bother because the diffs were not too promising

[MichalStrehovsky]

@MichalPetryka my guess is that the NativeAOT failures are because this brings into existence types that shouldn't exist. We have an optimization that optimizes MethodTables for things that shouldn't exist into a smaller data structure. Is it possible that for this code:

Console.WriteLine(Holder.TheX.GetType());

sealed class X { }
sealed class Holder
{
    public static X TheX = null;
}

Codegen will replace this with this?

Console.WriteLine(typeof(X));

The problem is that when we're doing optimized code generation, we need to compute the full set of types that had a MethodTable in the whole program. For the above program, we would not include MT for X because it was not needed.

The failure you're hitting looks like a situation where we didn't expect the MT to be needed so we generated a gimped "MT" that doesn't actually have much in it (and we can't construct a System.Type for it, among other things).

This should also be asserting the compiler.

Related: dotnet/runtimelab#1128

[MichalPetryka]

Codegen will replace this with this?

Console.WriteLine(typeof(X));

It'd actually be like this:

Nullcheck(Holder.TheX);
Console.WriteLine(typeof(X));

Which would prevent the code from getting to the typeof (and if possible I'd assume the compiler would fold the nullcheck to always true since it knows it can remove the metadata).
But the failing R2R test looks like this instead:

new MyType().GetType().ToString();

Could the unused new removal cause NAOT to omit metadata after the transformation?

[MichalStrehovsky]

Got a local repro. The codegen for:

public string GenericFooFunc3<M>() { return this.GetType() + "::GenericFooFunc3 called on " + typeof(T) + "::" + typeof(M); }

Is:

00007FF65F7A8CD9 48 8D 0D 90 B3 01 00 lea         rcx,[DynamicGenerics_MakeGenMethod_Bar_1<System___Canon>::`vftable' (07FF65F7C4070h)]  
00007FF65F7A8CE0 E8 8B 8C F1 FF       call        S_P_CoreLib_Internal_Runtime_CompilerHelpers_LdTokenHelpers__GetRuntimeType (07FF65F6C1970h)  

Notice we're grabbing the MethodTable for Bar<__Canon> directly instead of doing a runtime lookup for whatever the current T is. The reflection stack doesn't know what a Bar<__Canon> method table is since these should never show up in reflection or user code. This is bad codegen; there should be a runtime lookup.

[MichalPetryka]

Notice we're grabbing the MethodTable for Bar<__Canon> directly instead of doing a runtime lookup for whatever the current T is. The reflection stack doesn't know what a Bar<__Canon> method table is since these should never show up in reflection or user code. This is bad codegen; there should be a runtime lookup.

I've accidentally removed the shared generic check, readded it now, the issue is that we're getting invalid handled even with that.

[MichalStrehovsky]

Notice we're grabbing the MethodTable for Bar<__Canon> directly instead of doing a runtime lookup for whatever the current T is. The reflection stack doesn't know what a Bar<__Canon> method table is since these should never show up in reflection or user code. This is bad codegen; there should be a runtime lookup.

I've accidentally removed the shared generic check, readded it now, the issue is that we're getting invalid handled even with that.

I'm still seeing the same bad codegen with your latest commit.

[MichalPetryka]

I'm still seeing the same bad codegen with your latest commit.

Yeah that's the issue here, gtGetClassHandle somehow returns some invalid handles here.

[MichalPetryka]

@MihuBot -dependsOn 88163

[MichalPetryka]

Diffs seem to mostly be regressions, closing this then.