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

Switches with intrinsic const aren't getting optimized #78356

Closed
stephentoub opened this issue Nov 15, 2022 · 8 comments
Closed

Switches with intrinsic const aren't getting optimized #78356

stephentoub opened this issue Nov 15, 2022 · 8 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Priority:3 Work that is nice to have
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Nov 15, 2022

Consider:

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Test<TEnum>()
{
    TypeCode code = Type.GetTypeCode(typeof(TEnum));
    switch (code)
    {
        case TypeCode.Int32:
            return 100;
        case TypeCode.Int64:
            return 200;
        default:
            return 300;
    }
}

called with Test<DayOfWeek>. The optimized code for this ends up being:

; Assembly listing for method ConsoleApp4.Program:Test[int]():int
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; Tier-1 compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 0 single block inlinees; 2 inlinees without PGO data

G_M000_IG01:                ;; offset=0000H

G_M000_IG02:                ;; offset=0000H
       B809000000           mov      eax, 9
       83F809               cmp      eax, 9
       7407                 je       SHORT G_M000_IG04

G_M000_IG03:                ;; offset=000AH
       83F80B               cmp      eax, 11
       7408                 je       SHORT G_M000_IG06
       EB0C                 jmp      SHORT G_M000_IG08

G_M000_IG04:                ;; offset=0011H
       B864000000           mov      eax, 100

G_M000_IG05:                ;; offset=0016H
       C3                   ret

G_M000_IG06:                ;; offset=0017H
       B8C8000000           mov      eax, 200

G_M000_IG07:                ;; offset=001CH
       C3                   ret

G_M000_IG08:                ;; offset=001DH
       B82C010000           mov      eax, 300

G_M000_IG09:                ;; offset=0022H
       C3                   ret

; Total bytes of code 35

so even though the Type.GetTypeCode(typeof(TEnum)) is turned into mov eax, 9, the whole switch is still implemented (that mov eax, 9 followed by a cmp eax, 9 is particularly fun). If I instead replace the Type.GetTypeCode(typeof(TEnum)) with TypeCode.Int32, then I do get the expected:

; Assembly listing for method ConsoleApp4.Program:Test[int]():int
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; Tier-1 compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data

G_M000_IG01:                ;; offset=0000H

G_M000_IG02:                ;; offset=0000H
       B864000000           mov      eax, 100

G_M000_IG03:                ;; offset=0005H
       C3                   ret

; Total bytes of code 6
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 15, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 15, 2022
@ghost
Copy link

ghost commented Nov 15, 2022

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

Issue Details

Consider:

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Test<TEnum>()
{
    TypeCode code = Type.GetTypeCode(typeof(TEnum));
    switch (code)
    {
        case TypeCode.Int32:
            return 100;
        case TypeCode.Int64:
            return 200;
        default:
            return 300;
    }
}

called with Test<DayOfWeek>. The optimized code for this ends up being:

; Assembly listing for method ConsoleApp4.Program:Test[int]():int
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; Tier-1 compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 0 single block inlinees; 2 inlinees without PGO data

G_M000_IG01:                ;; offset=0000H

G_M000_IG02:                ;; offset=0000H
       B809000000           mov      eax, 9
       83F809               cmp      eax, 9
       7407                 je       SHORT G_M000_IG04

G_M000_IG03:                ;; offset=000AH
       83F80B               cmp      eax, 11
       7408                 je       SHORT G_M000_IG06
       EB0C                 jmp      SHORT G_M000_IG08

G_M000_IG04:                ;; offset=0011H
       B864000000           mov      eax, 100

G_M000_IG05:                ;; offset=0016H
       C3                   ret

G_M000_IG06:                ;; offset=0017H
       B8C8000000           mov      eax, 200

G_M000_IG07:                ;; offset=001CH
       C3                   ret

G_M000_IG08:                ;; offset=001DH
       B82C010000           mov      eax, 300

G_M000_IG09:                ;; offset=0022H
       C3                   ret

; Total bytes of code 35

so even though the Type.GetTypeCode(typeof(TEnum)) is turned into mov eax, 9, the whole switch is still implemented. If I instead replace the Type.GetTypeCode(typeof(TEnum)) with TypeCode.Int32, then I do get the expected:

; Assembly listing for method ConsoleApp4.Program:Test[int]():int
; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
; Tier-1 compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data

G_M000_IG01:                ;; offset=0000H

G_M000_IG02:                ;; offset=0000H
       B864000000           mov      eax, 100

G_M000_IG03:                ;; offset=0005H
       C3                   ret

; Total bytes of code 6
Author: stephentoub
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jkotas
Copy link
Member

jkotas commented Nov 15, 2022

You should see it getting optimized if you switch on GetEnumUnderlyingType:

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Test<TEnum>()
{
    Type type = typeof(TEnum).GetEnumUnderlyingType();
    if (type == typeof(int))
        return 100;
    if (type == typeof(long))
        return 200;
    return 300;
}

@stephentoub
Copy link
Member Author

You should see it getting optimized if you switch on GetEnumUnderlyingType

I do, which is my local workaround.

@jkotas
Copy link
Member

jkotas commented Nov 15, 2022

I think that switching on GetEnumUnderlyingType is the right forward-looking way to write this. We have been saying that IConvertible and TypeCode are legacy concepts.

@stephentoub
Copy link
Member Author

Ok.

Though I still think outputting the code:

       B809000000           mov      eax, 9
       83F809               cmp      eax, 9

is suspect :)

@jkotas
Copy link
Member

jkotas commented Nov 15, 2022

Yes, it would be nice to optimize this better. The problem is not specific to intrinsics. Any switch over constant expression that is complex-enough will hit this problem. For example:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
static int Map(int x, int y, bool firstOrSecond)
{
    int t = firstOrSecond ? x : y;
    if (t == 50) return 1;
    if (t == 100) return 2;
    return 3;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Test()
{
    switch (Map(50, 100, false))
    {
    case 0:
        return 100;
    case 1:
        return 200;
    default:
        return 0;
    }
}

generates

...
mov      edx, 2
cmp      edx, 1
...

@EgorBo
Copy link
Member

EgorBo commented Nov 15, 2022

Interesting case, thanks, we end-up with this when we hit rationalization:

image

@EgorBo EgorBo added this to the 8.0.0 milestone Nov 15, 2022
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Nov 15, 2022
@JulieLeeMSFT JulieLeeMSFT added the Priority:3 Work that is nice to have label Apr 13, 2023
@EgorBo EgorBo modified the milestones: 8.0.0, Future Jun 9, 2023
@EgorBo
Copy link
Member

EgorBo commented Jan 8, 2024

Both Stephen's and Jan's examples are now folded to return const; in Main.
I bet it was improved by @AndyAyersMS's #94689

@EgorBo EgorBo closed this as completed Jan 8, 2024
@EgorBo EgorBo assigned AndyAyersMS and unassigned EgorBo Jan 8, 2024
@EgorBo EgorBo modified the milestones: Future, 9.0.0 Jan 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 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 Priority:3 Work that is nice to have
Projects
None yet
Development

No branches or pull requests

5 participants