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

Add unsigned variants for BitConverter float bit APIs #53784

Merged
merged 4 commits into from Jun 8, 2021

Conversation

MichalPetryka
Copy link
Contributor

Adds DoubleToUInt64Bits, UInt64BitsToDouble, SingleToUInt32Bits,
UInt32BitsToSingle, HalfToUInt16Bits and UInt16BitsToHalf.

Implementations were based on existing signed integer variants.

Convert usages of existing APIs to unsigned variants when appropriate.

Fix #36469

The changes build on my machine, but I was unable to run the unit tests, running all of them made the test process hang for 4 hours, so I'm opening this to see what happens on the CI.

Adds DoubleToUInt64Bits, UInt64BitsToDouble, SingleToUInt32Bits,
UInt32BitsToSingle, HalfToUInt16Bits and UInt16BitsToHalf.

Implementations were based on existing signed integer variants.

Convert usages of existing APIs to unsigned variants when appropriate.

Fix dotnet#36469
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@msftbot
Copy link
Contributor

msftbot bot commented Jun 6, 2021

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

Issue Details

Adds DoubleToUInt64Bits, UInt64BitsToDouble, SingleToUInt32Bits,
UInt32BitsToSingle, HalfToUInt16Bits and UInt16BitsToHalf.

Implementations were based on existing signed integer variants.

Convert usages of existing APIs to unsigned variants when appropriate.

Fix #36469

The changes build on my machine, but I was unable to run the unit tests, running all of them made the test process hang for 4 hours, so I'm opening this to see what happens on the CI.

Author: MichalPetryka
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@MichalPetryka
Copy link
Contributor Author

I'm not quite sure what's going on with the tests failing on linux arm and musl arm & arm64, the values returned by the API seem to be way above the ushort range. Could somebody take a look?

@EgorBo
Copy link
Member

EgorBo commented Jun 6, 2021

Btw, on x64 (thus, unrelated to your issue)

        public static unsafe ulong DoubleToUInt64Bits1(double value)
        {
            Vector128<ulong> vec = Vector128.CreateScalarUnsafe(value).AsUInt64();
            return Sse2.X64.ConvertToUInt64(vec);
        }

Emits:

G_M65487_IG01:              ;; offset=0000H
       C5F877               vzeroupper 
						;; bbWeight=1    PerfScore 1.00

G_M65487_IG02:              ;; offset=0003H
       C4E1F97EC0           vmovd    rax, xmm0
						;; bbWeight=1    PerfScore 2.00

G_M65487_IG03:              ;; offset=0008H
       C3                   ret      
						;; bbWeight=1    PerfScore 1.00
; Total bytes of code: 9

@tannergooding any idea why it's movd instead of movq ? .NET 5.0 seems to be fine: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAJI8ovLrl5hcAbho1iAZiakGAYQYBvedQZ79epU3JIG3XNgBmMMwBsIXAOYMAIhA7BbMACoQAqiIYqABCvBi45AAUACbunjYAbti2HDAAlDQGBtq6WXkAajBgGNDkpAAcADwc9k4AfAwJRQwAvAyFxaUVLGqw2BgwAMpgydhQfhJWMJFJKeksAIK4ATyokWlyuXkGxADsxuzcfALCouKS0iwAGuVILIO4MKTXqD0OTVAYvitBKDNFG0y2wAvjRgUA==

@EgorBo
Copy link
Member

EgorBo commented Jun 6, 2021

cc @sandreenko (#47843)

@tannergooding
Copy link
Member

any idea why it's movd instead of movq

Looks to be a disassembly issue, its emitted an 8-byte move

@@ -328,7 +328,7 @@ CVType switch
CV_U4 => (uint)_data,
CV_I8 => _data,
CV_U8 => (ulong)_data,
CV_R4 => BitConverter.Int32BitsToSingle((int)_data),
CV_R4 => BitConverter.UInt32BitsToSingle((uint)_data),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it so that both places in Variant use the same pair of APIs, the unsigned ones.

Reverted a comment change made in the existing code by mistake.
Use the existing signed methods with a cast, less code with the same codegen.
Corrected the type used in a cast.
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

IIRC, there were a few remaining cases that didn't immediately cast, but which should also be switched to the unsigned ones. We can get them in a follow up PR, however.

@MichalPetryka MichalPetryka marked this pull request as ready for review June 7, 2021 17:59
Copy link
Contributor

@pgovind pgovind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, LGTM!

@tannergooding tannergooding merged commit 36a7e76 into dotnet:main Jun 8, 2021
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Jul 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding SingleToUInt32Bits(float value) to BitConverter
5 participants