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

Make Type.IsGenericType and GetGenericTypeDefinition() JIT intrinsics (and JIT time constants) #96898

Closed
Sergio0694 opened this issue Jan 12, 2024 · 8 comments · Fixed by #103528
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@Sergio0694
Copy link
Contributor

Overview

This is one more codegen improvement we could leverage for CsWinRT, related to #95929. One scenario we have (here) is to check whether a given type T is some KeyValuePair<,> instantiation. This needs to be marshalled in a specific way to match the expectations of WinRT. Right now however, the only way we have to check for this case is to use IsGenericType and GetGenericTypeDefinition(), which are not JIT intrinsics. This makes the linker not see this special case, and therefore not correctly trim the code away (or just specialize it on the other hand). This is both marginally slower, but most importantly leaves some codegen size reduction opportunities on the table. We're seeing a bunch of KeyValuePair<__Canon, __Canon> instantiations in sizoscope even in sample WinRT components that never use KeyValuePair<,> at all anywhere. It would be nice if this could just be properly inlined as a constant instead.

Codegen

using SharpLab.Runtime;

[JitGeneric(typeof(System.Collections.Generic.KeyValuePair<int, int>))]
[JitGeneric(typeof(int))]
static bool IsKVP<T>()
{
    return typeof(T).IsGenericType && typeof(T).GetGenericTypeDefinition() == typeof(System.Collections.Generic.KeyValuePair<,>);
}

Current codegen on .NET 8 x64 (sharplab):

Program.<<Main>$>g__IsKVP|0_0[[System.Collections.Generic.KeyValuePair`2[[System.Int32, System.Private.CoreLib],[System.Int32, System.Private.CoreLib]], System.Private.CoreLib]]()
    L0000: sub rsp, 0x28
    L0004: mov rcx, 0x23e784e3260
    L000e: call qword ptr [0x7ffb1060a518]
    L0014: test eax, eax
    L0016: je short L0040
    L0018: mov rcx, 0x23e784e3260
    L0022: call qword ptr [0x7ffb1060a568]
    L0028: mov rcx, 0x23e783e2728
    L0032: cmp rax, rcx
    L0035: sete al
    L0038: movzx eax, al
    L003b: add rsp, 0x28
    L003f: ret
    L0040: xor eax, eax
    L0042: add rsp, 0x28
    L0046: ret

Program.<<Main>$>g__IsKVP|0_0[[System.Int32, System.Private.CoreLib]]()
    ; pretty much the same as above

Expected codegen:

Program.<<Main>$>g__IsKVP|0_0[[System.Collections.Generic.KeyValuePair`2[[System.Int32, System.Private.CoreLib],[System.Int32, System.Private.CoreLib]], System.Private.CoreLib]]()
    L0000: mov eax, 1
    L0005: ret

Program.<<Main>$>g__IsKVP|0_0[[System.Int32, System.Private.CoreLib]]()
    L0000: xor eax, eax
    L0002: ret

cc. @jkoritzinsky @MichalStrehovsky @EgorBo

@Sergio0694 Sergio0694 added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 12, 2024
@ghost
Copy link

ghost commented Jan 12, 2024

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

Issue Details

Overview

This is one more codegen improvement we could leverage for CsWinRT, related to #95929. One scenario we have (here) is to check whether a given type T is some KeyValuePair<,> instantiation. This needs to be marshalled in a specific way to match the expectations of WinRT. Right now however, the only way we have to check for this case is to use IsGenericType and GetGenericTypeDefinition(), which are not JIT intrinsics. This makes the linker not see this special case, and therefore not correctly trim the code away (or just specialize it on the other hand). This is both marginally slower, but most importantly leaves some codegen size reduction opportunities on the table. We're seeing a bunch of KeyValuePair<__Canon, __Canon> instantiations in sizoscope even in sample WinRT components that never use KeyValuePair<,> at all anywhere. It would be nice if this could just be properly inlined as a constant instead.

Codegen

using SharpLab.Runtime;

[JitGeneric(typeof(System.Collections.Generic.KeyValuePair<int, int>))]
[JitGeneric(typeof(int))]
static bool IsKVP<T>()
{
    return typeof(T).IsGenericType && typeof(T).GetGenericTypeDefinition() == typeof(System.Collections.Generic.KeyValuePair<,>);
}

Current codegen on .NET 8 x64 (sharplab):

Program.<<Main>$>g__IsKVP|0_0[[System.Collections.Generic.KeyValuePair`2[[System.Int32, System.Private.CoreLib],[System.Int32, System.Private.CoreLib]], System.Private.CoreLib]]()
    L0000: sub rsp, 0x28
    L0004: mov rcx, 0x23e784e3260
    L000e: call qword ptr [0x7ffb1060a518]
    L0014: test eax, eax
    L0016: je short L0040
    L0018: mov rcx, 0x23e784e3260
    L0022: call qword ptr [0x7ffb1060a568]
    L0028: mov rcx, 0x23e783e2728
    L0032: cmp rax, rcx
    L0035: sete al
    L0038: movzx eax, al
    L003b: add rsp, 0x28
    L003f: ret
    L0040: xor eax, eax
    L0042: add rsp, 0x28
    L0046: ret

Program.<<Main>$>g__IsKVP|0_0[[System.Int32, System.Private.CoreLib]]()
    ; pretty much the same as above

Expected codegen:

Program.<<Main>$>g__IsKVP|0_0[[System.Collections.Generic.KeyValuePair`2[[System.Int32, System.Private.CoreLib],[System.Int32, System.Private.CoreLib]], System.Private.CoreLib]]()
    L0000: mov eax, 1
    L0005: ret

Program.<<Main>$>g__IsKVP|0_0[[System.Int32, System.Private.CoreLib]]()
    L0000: xor eax, eax
    L0002: ret

cc. @jkoritzinsky @MichalStrehovsky @EgorBo

Author: Sergio0694
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 12, 2024
@Sergio0694
Copy link
Contributor Author

Sharing some notes from Discord:

  • @MichalPetryka mentioned that IsConstructedGenericType would be easier to make an intrinsic compared to IsGenericType, and it would still be sufficient for this scenario. Either of them would be completely fine, really.
  • @SingleAccretion mentioned that making GetGenericTypeDefinition() could be difficult to make an intrinsic, but it would be possible to just make direct comparisons over that return value be an intrinsic. That would also work in this scenario.

@Sergio0694
Copy link
Contributor Author

I agree there's not many hits, but this would be beneficial for CsWinRT. We're really trying to minimize the binary size in minimal applications and WinRT components, to make the adoption of C# easier to justify in place of C++ (and binary size is one of the main problems you immediately hit when doing so). We have lots of heavily generic code especially in our marshallers, and they all end up being instantiated over KeyValuePair<,> today because we can't tell the linker that branch is really not used. Having a linker-friendly way of doing that check would be nice to have, is all 🙂

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Jan 13, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 13, 2024
@teo-tsirpanis
Copy link
Contributor

It would also help with F#'s typedefof.

@hez2010
Copy link
Contributor

hez2010 commented Jun 17, 2024

Can we also intrinsify typeof(T).IsConstructedGenericType as well?

@Sergio0694
Copy link
Contributor Author

Thank you for volunteering! 😄

@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants