Skip to content

Inefficient codegen for casts between same size types. #11413

Closed
Closed
Inefficient codegen for casts between same size types.#11413
Assignees
sandreenko
Labels
area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIoptimization
Milestone
Future

Description

@tannergooding
tannergooding
opened on Nov 5, 2018

As per dotnet/coreclr#20788 (comment), using BitConverter.SingleToInt32Bits, BitConverter.Int32BitsToSingle, BitConverter.DoubleToInt64Bits, and BitConverter.Int64BitsToDouble generates some "inefficient" code.

Currently BitConverter.SingleToInt32Bits is generating:

vmovss   dword ptr [rsp+14H], xmm0
mov      eax, dword ptr [rsp+14H]

When it could generate:

vmovd eax, xmm0

Currently BitConverter.Int32BitsToSingle is generating:

mov      dword ptr [rsp+0CH], eax
vmovss   xmm0, dword ptr [rsp+0CH]

When it could generate:

vmovd xmm0, eax

The same logic applies to double <-> long, but using the rax register and vmovq.

  • For x86, it can use the movq xmm, [m64] or movq [m64], xmm encoding

category:cq
theme:casts
skill-level:intermediate
cost:medium

Activity

  1. msftgits
    transferred this issue fromdotnet/coreclron Jan 31, 2020
  2. msftgits
    added this to the Future milestone on Jan 31, 2020

  3. GrabYourPitchforks commented on Jan 31, 2020

    @GrabYourPitchforks
    GrabYourPitchforks
    on Jan 31, 2020
    Member

    Related PR: #6864, which makes use of this API

  4. tannergooding commented on Mar 20, 2020

    @tannergooding
    tannergooding
    on Mar 20, 2020
    MemberAuthor

    #33476 introduced a fix which works around the issue, but we are leaving this issue open to track the JIT getting a more general codegen improvement that isn't specific to BitConverter.

  5. sandreenko
    self-assigned this
    on May 27, 2020
  6. sandreenko
    removed
    arch-x64
    arch-x86
    on May 27, 2020
  7. sandreenko
    changed the title [-]Inefficient codegen for `BitConverter.SingleToInt32Bits` and `BitConverter.DoubleToInt64Bits`[/-] [+]Inefficient codegen for casts between same size types.[/+] on May 27, 2020

  8. Kein commented on Sep 24, 2020

    @Kein
    Kein
    on Sep 24, 2020

    I guess this is partially related to aforementioned issue - is there any reason float.isFinite() is safe while the rest of similar extensions using similar tools are unsafe (like IsInfinity)?

  9. GrabYourPitchforks commented on Sep 24, 2020

    @GrabYourPitchforks
    GrabYourPitchforks
    on Sep 24, 2020
    Member

    @Kein Looks like we just forgot to remove the unsafe modifier from the Single.IsInfinity method. It doesn't affect callers; they're not required to be unsafe. But it does provide some further evidence that we should consider stripping the unsafe modifier from types and functions that no longer need it, just as an overall code hygiene cleanup.

  10. tannergooding commented on Sep 24, 2020

    @tannergooding
    tannergooding
    on Sep 24, 2020
    MemberAuthor

    But it does provide some further evidence that we should consider stripping the unsafe modifier from types and functions that no longer need it, just as an overall code hygiene cleanup.

    This seems like one of the simpler analyzers we could write, is there an issue suggesting it yet?

  11. GrabYourPitchforks commented on Sep 24, 2020

    @GrabYourPitchforks
    GrabYourPitchforks
    on Sep 24, 2020
    Member

    There's some limited discussion around it as part of #31354. Basically: if we have an analyzer that says "you're not using pointers, remove unsafe" alongside a language feature that enforces "this API doesn't use pointers but you need to wrap call sites within an unsafe block", we need to figure out how these two things interact together.

  12. BruceForstall
    added
    JitUntriagedCLR JIT issues needing additional triage
    on Oct 28, 2020
  13. BruceForstall
    removed
    JitUntriagedCLR JIT issues needing additional triage
    on Nov 25, 2020

  14. deeprobin commented on Aug 16, 2022

    @deeprobin
    deeprobin
    on Aug 16, 2022
    Contributor

    In .NET 7 it generates

    https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHuGAHKAJYA3bBhgMBAOwwMAGgAoAZgBsIohooCUHLjz3EA7AwBCAjCwiShMKGKgA6AMpSA5spgAVCAElpdUqYYuEqaANy63AC+NJFAA=

        L0000: vzeroupper
    L0003: vmovd eax, xmm1

  15. JulieLeeMSFT commented on Aug 20, 2024

    @JulieLeeMSFT
    JulieLeeMSFT
    on Aug 20, 2024
    Member

    With .NET 9 Preview 7,
    BitConverter.SingleToInt32Bits(float) generates

    vmovd    eax, xmm0

    BitConverter.Int32BitsToSingle(int) generates

    vmovd    xmm0, ecx

    BitConverter.DoubleToInt64Bits(double) generates

     vmovd    rax, xmm0

    BitConverter.Int64BitsToDouble(long) generates

    vmovd    xmm0, rcx

  16. JulieLeeMSFT commented on Aug 21, 2024

    @JulieLeeMSFT
    JulieLeeMSFT
    on Aug 21, 2024
    Member

    @tannergooding, please check the generated encoding with .NET 9 above (#11413 (comment)).

    • float <-> int is generating the expected code.
    • double <-> long is generating vmovd, not vmovq. Do we need further optimization?

      The same logic applies to double <-> long, but using the rax register and vmovq.

    cc @dotnet/jit-contrib

  17. tannergooding commented on Aug 21, 2024

    @tannergooding
    tannergooding
    on Aug 21, 2024
    MemberAuthor

    This got resolved some time back (#71567, as well as other commits by other JIT team members)

    The movd for double <-> long is actually just a disassembly bug/quirk. movd and movq are the same instruction, it's just that movq is the preferred disassembly when operating on 64-bit registers. Looks like we're not accounting for that when we do the disasm dumping

  18. tannergooding
    closed this as completedon Aug 21, 2024

  19. JulieLeeMSFT commented on Aug 21, 2024

    @JulieLeeMSFT
    JulieLeeMSFT
    on Aug 21, 2024
    Member

    Very good.

    This got resolved some time back (#71567, as well as other commits by other JIT team members)

    Also checked SingleToInt32Bits(float) on x86

    mov      eax, dword ptr [ebp+0x08]

  20. JulieLeeMSFT commented on Aug 21, 2024

    @JulieLeeMSFT
    JulieLeeMSFT
    on Aug 21, 2024
    Member

    on x86,
    Int32BitsToSingle(int):float

           vmovd    xmm0, ecx
       vmovss   dword ptr [ebp-0x04], xmm0
       fld      dword ptr [ebp-0x04]

    DoubleToInt64Bits(double):long

           vmovsd   xmm0, qword ptr [ebp+0x08]
       sub      esp, 8
       vmovsd   qword ptr [esp], xmm0
       call     [System.BitConverter:DoubleToInt64Bits(double):long]

    Int64BitsToDouble(long):double

           push     dword ptr [ebp+0x0C]
       push     dword ptr [ebp+0x08]
       call     [System.BitConverter:Int64BitsToDouble(long):double]
       fstp     qword ptr [ebp-0x08]
       vmovsd   xmm0, qword ptr [ebp-0x08]
       vmovsd   qword ptr [ebp-0x08], xmm0
       fld      qword ptr [ebp-0x08]

  21. tannergooding commented on Aug 21, 2024

    @tannergooding
    tannergooding
    on Aug 21, 2024
    MemberAuthor

    👍

    x86 is expected to be less efficient for DoubleToInt64Bits and the inverse here. There's no singular instruction available since the register size is 32-bits and so we have to do handle the lower and upper halves through memory.

    There is notably a way to load a double directly to/from memory using a specialized memory only version of movq on 32-bit, but that's a non-trivial amount of work and it hasn't bubbled up in priority yet. That work is tracked instead by #11626

  22. github-actions
    locked and limited conversation to collaborators on Sep 20, 2024
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 SuperPMIoptimization

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions