Skip to content

Commit

Permalink
Fixing the handling of Positive NaN in Math.Min for float/double (dot…
Browse files Browse the repository at this point in the history 
…net#70795)

* Adding tests validating Positive NaN for Max, MaxMagnitude, Min, and MinMagnitude

* Fixing the handling of Positive NaN in Math.Min for float/double

* Fixing the Max/Min code comments to use greater and lesser

* Adding a code comment clarifying the sign toggling behavior
  • Loading branch information
@tannergooding
tannergooding authored Jun 17, 2022
1 parent 1a02f0d commit 48adb84
Show file tree
Hide file tree
Showing 4 changed files with 251 additions and 21 deletions.
42 changes: 26 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Math.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -888,8 +888,8 @@ public static double Max(double val1, double val2)
// This matches the IEEE 754:2019 `maximum` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the larger of the inputs. It
// treats +0 as larger than -0 as per the specification.
// otherwise returns the greater of the inputs. It
// treats +0 as greater than -0 as per the specification.

if (val1 != val2)
{
Expand Down Expand Up @@ -946,8 +946,8 @@ public static float Max(float val1, float val2)
// This matches the IEEE 754:2019 `maximum` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the larger of the inputs. It
// treats +0 as larger than -0 as per the specification.
// otherwise returns the greater of the inputs. It
// treats +0 as greater than -0 as per the specification.

if (val1 != val2)
{
Expand Down Expand Up @@ -999,8 +999,8 @@ public static double MaxMagnitude(double x, double y)
// This matches the IEEE 754:2019 `maximumMagnitude` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the input with a larger magnitude.
// It treats +0 as larger than -0 as per the specification.
// otherwise returns the input with a greater magnitude.
// It treats +0 as greater than -0 as per the specification.

double ax = Abs(x);
double ay = Abs(y);
Expand Down Expand Up @@ -1037,12 +1037,17 @@ public static double Min(double val1, double val2)
// This matches the IEEE 754:2019 `minimum` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the larger of the inputs. It
// treats +0 as larger than -0 as per the specification.
// otherwise returns the lesser of the inputs. It
// treats +0 as lesser than -0 as per the specification.

if (val1 != val2 && !double.IsNaN(val1))
if (val1 != val2)
{
return val1 < val2 ? val1 : val2;
if (!double.IsNaN(val1))
{
return val1 < val2 ? val1 : val2;
}

return val1;
}

return double.IsNegative(val1) ? val1 : val2;
Expand Down Expand Up @@ -1090,12 +1095,17 @@ public static float Min(float val1, float val2)
// This matches the IEEE 754:2019 `minimum` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the larger of the inputs. It
// treats +0 as larger than -0 as per the specification.
// otherwise returns the lesser of the inputs. It
// treats +0 as lesser than -0 as per the specification.

if (val1 != val2 && !float.IsNaN(val1))
if (val1 != val2)
{
return val1 < val2 ? val1 : val2;
if (!float.IsNaN(val1))
{
return val1 < val2 ? val1 : val2;
}

return val1;
}

return float.IsNegative(val1) ? val1 : val2;
Expand Down Expand Up @@ -1138,8 +1148,8 @@ public static double MinMagnitude(double x, double y)
// This matches the IEEE 754:2019 `minimumMagnitude` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the input with a larger magnitude.
// It treats +0 as larger than -0 as per the specification.
// otherwise returns the input with a lesser magnitude.
// It treats +0 as lesser than -0 as per the specification.

double ax = Abs(x);
double ay = Abs(y);
Expand Down
8 changes: 4 additions & 4 deletions src/libraries/System.Private.CoreLib/src/System/MathF.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ public static float MaxMagnitude(float x, float y)
// This matches the IEEE 754:2019 `maximumMagnitude` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the input with a larger magnitude.
// It treats +0 as larger than -0 as per the specification.
// otherwise returns the input with a greater magnitude.
// It treats +0 as greater than -0 as per the specification.

float ax = Abs(x);
float ay = Abs(y);
Expand Down Expand Up @@ -282,8 +282,8 @@ public static float MinMagnitude(float x, float y)
// This matches the IEEE 754:2019 `minimumMagnitude` function
//
// It propagates NaN inputs back to the caller and
// otherwise returns the input with a larger magnitude.
// It treats +0 as larger than -0 as per the specification.
// otherwise returns the input with a lesser magnitude.
// It treats +0 as lesser than -0 as per the specification.

float ax = Abs(x);
float ay = Abs(y);
Expand Down
140 changes: 139 additions & 1 deletion src/libraries/System.Runtime.Extensions/tests/System/Math.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,29 @@ public static void Max_Decimal()
public static void Max_Double_NotNetFramework(double x, double y, double expectedResult)
{
AssertExtensions.Equal(expectedResult, Math.Max(x, y), 0.0);

if (double.IsNaN(x))
{
// Toggle the sign of the NaN to validate both +NaN and -NaN behave the same.
// Negate should work as well but the JIT may constant fold or do other tricks
// and normalize to a single NaN form so we do bitwise tricks to ensure we test
// the right thing.

ulong bits = BitConverter.DoubleToUInt64Bits(x);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
x = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.Max(x, y), 0.0);
}

if (double.IsNaN(y))
{
ulong bits = BitConverter.DoubleToUInt64Bits(y);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
y = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.Max(x, y), 0.0);
}
}

[Fact]
Expand Down Expand Up @@ -854,6 +877,29 @@ public static void Max_SByte()
public static void Max_Single_NotNetFramework(float x, float y, float expectedResult)
{
AssertExtensions.Equal(expectedResult, Math.Max(x, y), 0.0f);

if (float.IsNaN(x))
{
// Toggle the sign of the NaN to validate both +NaN and -NaN behave the same.
// Negate should work as well but the JIT may constant fold or do other tricks
// and normalize to a single NaN form so we do bitwise tricks to ensure we test
// the right thing.

uint bits = BitConverter.SingleToUInt32Bits(x);
bits ^= BitConverter.SingleToUInt32Bits(-0.0f);
x = BitConverter.UInt32BitsToSingle(bits);

AssertExtensions.Equal(expectedResult, Math.Max(x, y), 0.0f);
}

if (float.IsNaN(y))
{
uint bits = BitConverter.SingleToUInt32Bits(y);
bits ^= BitConverter.SingleToUInt32Bits(-0.0f);
y = BitConverter.UInt32BitsToSingle(bits);

AssertExtensions.Equal(expectedResult, Math.Max(x, y), 0.0f);
}
}

[Fact]
Expand Down Expand Up @@ -919,6 +965,29 @@ public static void Min_Decimal()
public static void Min_Double_NotNetFramework(double x, double y, double expectedResult)
{
AssertExtensions.Equal(expectedResult, Math.Min(x, y), 0.0);

if (double.IsNaN(x))
{
// Toggle the sign of the NaN to validate both +NaN and -NaN behave the same.
// Negate should work as well but the JIT may constant fold or do other tricks
// and normalize to a single NaN form so we do bitwise tricks to ensure we test
// the right thing.

ulong bits = BitConverter.DoubleToUInt64Bits(x);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
x = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.Min(x, y), 0.0);
}

if (double.IsNaN(y))
{
ulong bits = BitConverter.DoubleToUInt64Bits(y);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
y = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.Min(x, y), 0.0);
}
}

[Fact]
Expand Down Expand Up @@ -977,6 +1046,29 @@ public static void Min_SByte()
public static void Min_Single_NotNetFramework(float x, float y, float expectedResult)
{
AssertExtensions.Equal(expectedResult, Math.Min(x, y), 0.0f);

if (float.IsNaN(x))
{
// Toggle the sign of the NaN to validate both +NaN and -NaN behave the same.
// Negate should work as well but the JIT may constant fold or do other tricks
// and normalize to a single NaN form so we do bitwise tricks to ensure we test
// the right thing.

uint bits = BitConverter.SingleToUInt32Bits(x);
bits ^= BitConverter.SingleToUInt32Bits(-0.0f);
x = BitConverter.UInt32BitsToSingle(bits);

AssertExtensions.Equal(expectedResult, Math.Min(x, y), 0.0f);
}

if (float.IsNaN(y))
{
uint bits = BitConverter.SingleToUInt32Bits(y);
bits ^= BitConverter.SingleToUInt32Bits(-0.0f);
y = BitConverter.UInt32BitsToSingle(bits);

AssertExtensions.Equal(expectedResult, Math.Min(x, y), 0.0f);
}
}

[Fact]
Expand Down Expand Up @@ -2566,6 +2658,29 @@ public static void Log2(double value, double expectedResult, double allowedVaria
public static void MaxMagnitude(double x, double y, double expectedResult)
{
AssertExtensions.Equal(expectedResult, Math.MaxMagnitude(x, y), 0.0);

if (double.IsNaN(x))
{
// Toggle the sign of the NaN to validate both +NaN and -NaN behave the same.
// Negate should work as well but the JIT may constant fold or do other tricks
// and normalize to a single NaN form so we do bitwise tricks to ensure we test
// the right thing.

ulong bits = BitConverter.DoubleToUInt64Bits(x);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
x = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.MaxMagnitude(x, y), 0.0);
}

if (double.IsNaN(y))
{
ulong bits = BitConverter.DoubleToUInt64Bits(y);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
y = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.MaxMagnitude(x, y), 0.0);
}
}

[Theory]
Expand All @@ -2589,6 +2704,29 @@ public static void MaxMagnitude(double x, double y, double expectedResult)
public static void MinMagnitude(double x, double y, double expectedResult)
{
AssertExtensions.Equal(expectedResult, Math.MinMagnitude(x, y), 0.0);

if (double.IsNaN(x))
{
// Toggle the sign of the NaN to validate both +NaN and -NaN behave the same.
// Negate should work as well but the JIT may constant fold or do other tricks
// and normalize to a single NaN form so we do bitwise tricks to ensure we test
// the right thing.

ulong bits = BitConverter.DoubleToUInt64Bits(x);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
x = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.MinMagnitude(x, y), 0.0);
}

if (double.IsNaN(y))
{
ulong bits = BitConverter.DoubleToUInt64Bits(y);
bits ^= BitConverter.DoubleToUInt64Bits(-0.0);
y = BitConverter.UInt64BitsToDouble(bits);

AssertExtensions.Equal(expectedResult, Math.MinMagnitude(x, y), 0.0);
}
}

[Theory]
Expand Down Expand Up @@ -2657,7 +2795,7 @@ public static void MinMagnitude(double x, double y, double expectedResult)
[InlineData( 9.267056966972586, 2, 37.06822786789034, CrossPlatformMachineEpsilon * 100)]
[InlineData( 0.5617597462207241, 5, 17.97631187906317, CrossPlatformMachineEpsilon * 100)]
[InlineData( 0.7741522965913037, 6, 49.545746981843436, CrossPlatformMachineEpsilon * 100)]
[InlineData( -0.6787637026394024, 7, -86.88175393784351, CrossPlatformMachineEpsilon * 100)]
[InlineData( -0.6787637026394024, 7, -86.88175393784351, CrossPlatformMachineEpsilon * 100)]
[InlineData( -6.531673581913484, 1, -13.063347163826968, CrossPlatformMachineEpsilon * 100)]
[InlineData( 9.267056966972586, 2, 37.06822786789034, CrossPlatformMachineEpsilon * 100)]
[InlineData( 0.5617597462207241, 5, 17.97631187906317, CrossPlatformMachineEpsilon * 100)]
Expand Down
Loading

0 comments on commit 48adb84

Please sign in to comment.