Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/libraries/System.Private.CoreLib/src/System/Half.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ namespace System

// Well-defined and commonly used values

public static Half Epsilon => new Half(EpsilonBits); // 5.9604645E-08
public static Half Epsilon => new Half(EpsilonBits); // 5.9604645E-08

public static Half PositiveInfinity => new Half(PositiveInfinityBits); // 1.0 / 0.0;

Expand Down Expand Up @@ -151,12 +151,19 @@ private ushort Significand

public static bool operator ==(Half left, Half right)
{
return left.Equals(right);
if (IsNaN(left) || IsNaN(right))
{
// IEEE defines that NaN is not equal to anything, including itself.
return false;
}

// IEEE defines that positive and negative zero are equivalent.
return (left._value == right._value) || AreZero(left, right);
}

public static bool operator !=(Half left, Half right)
{
return !(left.Equals(right));
return !(left == right);
Comment thread
pgovind marked this conversation as resolved.
}

/// <summary>Determines whether the specified value is finite (zero, subnormal, or normal).</summary>
Expand Down Expand Up @@ -415,14 +422,11 @@ public override bool Equals(object? obj)
/// </summary>
public bool Equals(Half other)
{
if (IsNaN(this) || IsNaN(other))
if (this == other)

@jkotas jkotas Aug 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes the implementation slower than it needs to be - the IsNaN checks will be duplicated when the method returns false.

I think it would be better to inline the logic here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just copied the implementation from double, but didn't try to optimize it.
If there's more arithmetic operators for Half and corresponding benchmark, it will be better to track the optimization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

operator== is very cheap for double. It is a method call for Half.

I think it is ok to merge this change as is into release/5.0. I have opened #41329 on the optimization.

{
// IEEE defines that NaN is not equal to anything, including itself.
return false;
return true;
}

// IEEE defines that positive and negative zero are equivalent.
return (_value == other._value) || AreZero(this, other);
return IsNaN(this) && IsNaN(other);
}

/// <summary>
Expand Down
11 changes: 10 additions & 1 deletion src/libraries/System.Runtime/tests/System/HalfTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ public static IEnumerable<object[]> Equals_TestData()
yield return new object[] { Half.MaxValue, Half.MaxValue, true };
yield return new object[] { Half.MaxValue, Half.MinValue, false };
yield return new object[] { Half.MaxValue, UInt16BitsToHalf(0x0000), false };
yield return new object[] { Half.NaN, Half.NaN, false };
yield return new object[] { Half.NaN, Half.NaN, true };
yield return new object[] { Half.MaxValue, 789.0f, false };
yield return new object[] { Half.MaxValue, "789", false };
}
Expand Down Expand Up @@ -932,5 +932,14 @@ public static void ToSingle(Half half, float verify)
float f = (float)half;
Assert.Equal(f, verify, precision: 1);
}

[Fact]
public static void EqualityMethodAndOperator()
{
Assert.True(Half.NaN.Equals(Half.NaN));
Assert.False(Half.NaN == Half.NaN);
Assert.True(Half.NaN != Half.NaN);
Assert.Equal(Half.NaN, Half.NaN);
}
}
}