Skip to content

JIT doesn't fold typeof(_Canon) == typeof(vt) #79081

Open

Description

@EgorBo

@am11 noticed in #78874 (comment)

public static bool Test<T>()
{
    Type k = typeof(T);
    return k == typeof(sbyte) || k == typeof(byte);
}

codegen for shared T:

G_M37465_IG01:              ;; offset=0000H
       4883EC28             sub      rsp, 40
       48894C2420           mov      qword ptr [rsp+20H], rcx
G_M37465_IG02:              ;; offset=0009H
       488B4938             mov      rcx, qword ptr [rcx+38H]
       488B09               mov      rcx, qword ptr [rcx]
       E8AB49505F           call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
       48BA205359CFE3010000 mov      rdx, 0x1E3CF595320      ; 'System.SByte'
       483BC2               cmp      rax, rdx
       7418                 je       SHORT G_M37465_IG05
G_M37465_IG03:              ;; offset=0024H
       48BAE00059CFE3010000 mov      rdx, 0x1E3CF5900E0      ; 'System.Byte'
       483BC2               cmp      rax, rdx
       0F94C0               sete     al
       0FB6C0               movzx    rax, al
G_M37465_IG04:              ;; offset=0037H
       4883C428             add      rsp, 40
       C3                   ret      
G_M37465_IG05:              ;; offset=003CH
       B801000000           mov      eax, 1
G_M37465_IG06:              ;; offset=0041H
       4883C428             add      rsp, 40
       C3                   ret      
; Total bytes of code 70

Expected codegen:

return false;

Activity

  1. ghost added
    area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
    on Dec 1, 2022
  2. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Dec 1, 2022
  3. self-assigned this
    on Dec 1, 2022
  4. removed
    untriagedNew issue has not been triaged by the area owner
    on Dec 1, 2022
  5. added this to the 8.0.0 milestone on Dec 1, 2022
  6. am11 commented on Dec 1, 2022

    @am11
    Member

    Thanks. The non-generic version is also slightly inefficient: https://godbolt.org/z/cW47njasd.

  7. stephentoub commented on Dec 1, 2022

    @stephentoub
    Member

    I assume fixing this would enable this to be removed?

    else if (typeof(TResult).IsValueType) // help the JIT avoid the value type branches for ref types

  8. EgorBo commented on Dec 1, 2022

    @EgorBo
    MemberAuthor

    I assume fixing this would enable this to be removed?

    else if (typeof(TResult).IsValueType) // help the JIT avoid the value type branches for ref types

    Yep, I have a fix already but it requires another JIT-EE update so I can't use SPMI to get diffs and have to run it locally 😞

  9. EgorBo commented on Dec 1, 2022

    @EgorBo
    MemberAuthor

    @stephentoub hm.. actually your case should be foldable as is 🤔 e.g.:

    image

    This issue is mostly about a case where a local is introduced.

  10. stephentoub commented on Dec 1, 2022

    @stephentoub
    Member

    That line was added a decade ago (or, at least, it's evolved from one that was):
    https://referencesource.microsoft.com/#mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs,737

    If it's not helping with code gen and it's not helping to improve the JIT's throughput in any meaningful way or otherwise helping with anything, let's delete it :)

  11. EgorBo commented on Dec 1, 2022

    @EgorBo
    MemberAuthor

    That line was added a decade ago (or, at least, it's evolved from one that was): https://referencesource.microsoft.com/#mscorlib/system/runtime/compilerservices/AsyncMethodBuilder.cs,737

    If it's not helping with code gen and it's not helping to improve the JIT's throughput in any meaningful way or otherwise helping with anything, let's delete it :)

    Ah makes sense, it probably slightly improves JIT's throughput for this specific method but unlikely it's worth additional code in C# 🙂

  12. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on Dec 1, 2022
  13. EgorBo commented on Dec 1, 2022

    @EgorBo
    MemberAuthor

    Thanks. The non-generic version is also slightly inefficient: https://godbolt.org/z/cW47njasd.

    That one must be a different issue - jit pushes generic context for VM, we have an issue for that somewhere (quite old one)

  14. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Dec 2, 2022
  15. modified the milestones: 8.0.0, Future on Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions