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

System.Half doesn't generate correct ordered comparison results when both sides are negative #39770

Closed
Gnbrkm41 opened this issue Jul 22, 2020 · 4 comments · Fixed by #39773
Closed

Comments

@Gnbrkm41
Copy link
Contributor

Description

The comparison methods / operators do not return the correct comparison result when both numbers are negative. For example, -1 <= NegativeInfinity returns true, when it is obvious that -1 is greater than NegativeInfinity.

Configuration

The code likely isn't specific to a certain H/W or OS. There's only software implementations of those comparison methods.

Regression?

This bug existed from the initial prototype code in dotnet/corefxlab.

Other information

The current implementation, return (short)(left._value) <= (short)(right._value); is incorrect when both sides are negative.

Consider the following:

Actual value Raw bits Signed raw bit comparison Unsigned raw bit comparison Half comparison
-1 1 01111 0000000000 Smaller (-17408) Smaller (48128) Larger
-∞ 1 11111 0000000000 Larger (-1024) Larger (64512) Smaller
------------ -------------------- --------------------------- ------------------------- ---------------
1 0 01111 0000000000 Smaller (15360) Smaller (15360) Smaller
0 11111 0000000000 Larger (31744) Larger (31744) Larger

This implementation is correct for cases where both sides are positive, but when both sides are negative we get an inverted result. This likely is the same in all 4 ordered comparison operators and CompareTo method.

The solution would be to invert the result when leftIsNegative is true. (XOR with leftIsNegative?)

public static bool operator <=(Half left, Half right)
{
if (IsNaN(left) || IsNaN(right))
{
// IEEE defines that NaN is unordered with respect to everything, including itself.
return false;
}
bool leftIsNegative = IsNegative(left);
if (leftIsNegative != IsNegative(right))
{
// When the signs of left and right differ, we know that left is less than right if it is
// the negative value. The exception to this is if both values are zero, in which case IEEE
// says they should be equal, even if the signs differ.
return leftIsNegative || AreZero(left, right);
}
return (short)(left._value) <= (short)(right._value);
}

P.S. Apparently we don't have any test that compares two negative values....

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 22, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Gnbrkm41
Copy link
Contributor Author

cc @tannergooding

@Gnbrkm41 Gnbrkm41 changed the title System.Half doesn't generate correct ordered comparison when both sides are negative System.Half doesn't generate correct ordered comparison results when both sides are negative Jul 22, 2020
@tannergooding tannergooding added this to the 5.0.0 milestone Jul 22, 2020
@tannergooding tannergooding added area-System.Numerics bug and removed untriaged New issue has not been triaged by the area owner labels Jul 22, 2020
@ghost
Copy link

ghost commented Jul 22, 2020

Tagging subscribers to this area: @tannergooding, @pgovind
Notify danmosemsft if you want to be subscribed.

@tannergooding
Copy link
Member

CC. @jeffhandley, @pgovind

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants