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.IsPrimitive a JIT intrinsic (and JIT time constant) #95929

Closed
Sergio0694 opened this issue Dec 12, 2023 · 5 comments · Fixed by #96226
Closed

Make Type.IsPrimitive a JIT intrinsic (and JIT time constant) #95929

Sergio0694 opened this issue Dec 12, 2023 · 5 comments · Fixed by #96226
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@Sergio0694
Copy link
Contributor

Overview

We're currently working on AOT support for CsWinRT, and as part of that we're also looking into ways to reduce the binary size footprint, which is currently very large even for very simple applications. One issue we've noticed in particular is the static constructor in the Marshaler<T> type which is quite large for value types (we've started doing some work in that area here, but there's plenty more to do). This is because the linker is not able to properly handle all branches, so it just preserves everything, which then ends up compiling a whole bunch of useless code, as can be seen when opening the MSTAT files with sizoscope.

It would be beneficial to use Type.IsPrimitive as a JIT constant, as we could use that to at least skip the (currently reflection based) path checking for ABI types, which is just completely breaking the flow analysis of the linker. We could then at least avoid that generic size explosion for all primitive types. In general, the more properties on Type were JIT time constants, the we could inform the linker in these cases, but I IsPrimitive seems like a good place to start, since there's plenty of (commonly used) value types there, and we already know none of them will ever have an ABI type anyway.

Codegen

[JitGeneric(typeof(int))]
static bool Test<T>() => typeof(T).IsPrimitive;

Current codegen on .NET 8 x64 (sharplab):

Program.<<Main>$>g__Test|0_0[[System.Int32, System.Private.CoreLib]]()
    L0000: sub rsp, 0x28
    L0004: mov rcx, 0x158e6ad3660
    L000e: call 0x00007ffb5ea7e7a0
    L0013: mov ecx, 1
    L0018: shlx eax, ecx, eax
    L001d: test eax, 0x3003ffc
    L0022: setne al
    L0025: movzx eax, al
    L0028: add rsp, 0x28
    L002c: ret

Expected codegen on .NET 8 x64:

Program.<<Main>$>g__Test|0_0[[System.Int32, System.Private.CoreLib]]()
    L0000: mov eax, 1
    L0005: ret

cc. @jkoritzinsky @MichalStrehovsky @EgorBo

@Sergio0694 Sergio0694 added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI area-Tools-ILLink .NET linker development as well as trimming analyzers labels Dec 12, 2023
@ghost
Copy link

ghost commented Dec 12, 2023

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

Issue Details

Overview

We're currently working on AOT support for CsWinRT, and as part of that we're also looking into ways to reduce the binary size footprint, which is currently very large even for very simple applications. One issue we've noticed in particular is the static constructor in the Marshaler<T> type which is quite large for value types (we've started doing some work in that area here, but there's plenty more to do). This is because the linker is not able to properly handle all branches, so it just preserves everything, which then ends up compiling a whole bunch of useless code, as can be seen when opening the MSTAT files with sizoscope.

It would be beneficial to use Type.IsPrimitive as a JIT constant, as we could use that to at least skip the (currently reflection based) path checking for ABI types, which is just completely breaking the flow analysis of the linker. We could then at least avoid that generic size explosion for all primitive types. In general, the more properties on Type were JIT time constants, the we could inform the linker in these cases, but I IsPrimitive seems like a good place to start, since there's plenty of (commonly used) value types there, and we already know none of them will ever have an ABI type anyway.

Codegen

[JitGeneric(typeof(int))]
static bool Test<T>() => typeof(T).IsPrimitive;

Current codegen on .NET 8 x64 (sharplab):

Program.<<Main>$>g__Test|0_0[[System.Int32, System.Private.CoreLib]]()
    L0000: sub rsp, 0x28
    L0004: mov rcx, 0x158e6ad3660
    L000e: call 0x00007ffb5ea7e7a0
    L0013: mov ecx, 1
    L0018: shlx eax, ecx, eax
    L001d: test eax, 0x3003ffc
    L0022: setne al
    L0025: movzx eax, al
    L0028: add rsp, 0x28
    L002c: ret

Expected codegen on .NET 8 x64:

Program.<<Main>$>g__Test|0_0[[System.Int32, System.Private.CoreLib]]()
    L0000: mov eax, 1
    L0005: ret

cc. @jkoritzinsky @MichalStrehovsky @EgorBo

Author: Sergio0694
Assignees: -
Labels:

area-CodeGen-coreclr, area-Tools-ILLink

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 12, 2023
@jkotas
Copy link
Member

jkotas commented Dec 12, 2023

Where is the IsPrimitive check in CsWinRT that you expect this optimization to help with?

@Sergio0694
Copy link
Contributor Author

We're not using it yet, but @jkoritzinsky suggested we could add a branch checking that here so that all primitive value types would be covered, and wouldn't cause the linker to preserve and generate code for everything because it can't statically resolve what the result of that helper type logic is doing. We're basically looking for a way of statically being able to tell if an unconstrained T type argument is blittable or not.

@jkotas
Copy link
Member

jkotas commented Dec 12, 2023

linker

Do you mean IL linker? IL linker does not analyze instantiated generics.

If you mean AOT compiler, you can replace the IsPrimitive check with inlined check for all primitive types typeof(T) == typeof(byte) || typeof(T) == typeof(sbyte) || ... (types checks like this are treated as intrinsics) and see whether it is going to produce the expected results.

@jkotas jkotas removed the area-Tools-ILLink .NET linker development as well as trimming analyzers label Dec 12, 2023
@MichalPetryka
Copy link
Contributor

We're basically looking for a way of statically being able to tell if an unconstrained T type argument is blittable or not.

Wouldn't an intrinsic like this:

public class Assembly
{
    public bool IsBlittable(Type type);
}

make more sense for this? It being on Assembly would let it respect DisableRuntimeMarshalling and such.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Dec 20, 2023
@EgorBo EgorBo self-assigned this Dec 21, 2023
@EgorBo EgorBo added this to the 9.0.0 milestone Dec 21, 2023
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Dec 21, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 21, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 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
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants