BitCast<TYP_REF>(TypeHandleToRuntimeTypeHandle(clsHandle)) => nongc obj

[EgorBo]

Improves codegen for #97949, new codegen for GetIntType:

; Assembly listing for method LdtokenRepro:Test():System.Type (FullOpts)
G_M57559_IG01:
       sub      rsp, 40
G_M57559_IG02:
       mov      rax, 0x1FA00005580      ; 'System.Int32'
G_M57559_IG03:
       add      rsp, 40
       ret
; Total bytes of code 19

[ghost]

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

Issue Details

---

Improves codegen for #97949, new codegen for GetIntType:

; Assembly listing for method LdtokenRepro:Test():System.Type (FullOpts)
G_M57559_IG01:
       sub      rsp, 40
G_M57559_IG02:
       mov      rax, 0x1FA00005580      ; 'System.Int32'
G_M57559_IG03:
       add      rsp, 40
       ret
; Total bytes of code 19

Author:	EgorBo
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@dotnet/jit-contrib PTAL, simple change. Suprisingly, there were some diffs.

Basically, it optimizes:

RuntimeTypeHandle struct = CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE(constClassHandle);
RuntimeType type = struct.m_handle;

into:

<frozen object handle representing that RuntimeType>

[jkotas]

Does the same optimization kick in for native AOT that uses different representation of RuntimeTypeHandle?

[EgorBo]

Does the same optimization kick in for native AOT that uses different representation of RuntimeTypeHandle?

Oh, I forgot that RuntimeType objects are already frozen in NAOT. Currently the function in question emits this on NAOT:

; Assembly listing for method LdtokenRepro:Test():System.Type (FullOpts)
G_M57559_IG01:
       sub      rsp, 40
G_M57559_IG02:
       lea      rcx, [(reloc 0x40000000004224d0)]      ; System.Int32
       call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE
       mov      rcx, rax
       call     System.Type:GetTypeFromHandle(System.RuntimeTypeHandle):System.Type
       nop      
G_M57559_IG03:
       add      rsp, 40
       ret      
; Total bytes of code 30

since GetTypeFromHandle is already marked as jit intrinsic I presume it shouldn't be too hard to handle this.

[EgorBo]

Ah, nvm, @SingleAccretion pointed to an issue on my side

[EgorBo]

@SingleAccretion can you take a look?
NAOT made this PR significantly bigger so at this point I am not sure the diffs worth the efforts, although, we fixed a few potential asserts/bugs along the way. E.g. optVNConstantPropOnTree could replace a tree with a non-embeddable GT_CNS_INT on NAOT

[EgorBo]

@SingleAccretion I've addressed your feedback, can you give it another look? thanks

[SingleAccretion]

Code changes LGTM!

[jakobbotsch]

Unrelated, but any idea why this needs a large node for LATE_DISASM?

[SingleAccretion]

Pretty sure it's dead code.

[jakobbotsch]

We revived LATE_DISASM for use in SVE emitter unit tests in #96286, so it's not dead anymore.

[SingleAccretion]

Yes, but I don't think this large opcode ifdefing is still needed. It seems in the past gtCompileTimeHandle was part of the requirement for a "large" node:

runtime/src/coreclr/jit/morph.cpp

Lines 12663 to 12669 in 24d149c

	#if defined(LATE_DISASM)
	// GT_CNS_INT is considered small, so ReplaceWith() won't copy all fields
	if (tree->IsIconHandle())
	{
	copy->AsIntCon()->gtCompileTimeHandle = tree->AsIntCon()->gtCompileTimeHandle;
	}
	#endif

[EgorBo]

CI failure is #97049

[BruceForstall]

@EgorBo This PR seems to lead to an assert during JitDump:

Assert failure(PID 33704 [0x000083a8], Thread: 21932 [0x55ac]): Assertion failed '&typeid(T) == &typeid(size_t)' in 'C:Test(System.Object)' during 'Do value numbering' (IL size 53; hash 0x1f0881f5; FullOpts)

    File: C:\gh\runtime3\src\coreclr\jit\valuenum.h:1102
    Image: C:\gh\runtime3\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\corerun.exe

Here's the test case I'm using:

using System;
using System.Runtime.CompilerServices;

public class C
{

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static void Test(object o)
    {
        if (o is string)
        {
            if (o is string)
            {
                Console.WriteLine("yes");
            }
        }
        if (o is string)
        {
            if (o is string)
            {
                Console.WriteLine("yes2");
            }
        }
    }

    public static void Main(string[] args)
    {
        Test("hello");
        Test(2);
    }

}

[EgorBo]

@BruceForstall yeah 🙁 I've fixed it in #98337 (ready for final review)

[EgorBo]

Ah, wait, it should be fixed in main after https://github.com/dotnet/runtime/pull/98273/files already. I just added ssize_t to that assert since there should be no difference for us what we use to get a handle - we cast it to a CORINFO_OBJECT_HANDLE anyway usually.