Skip to content

Commit

Permalink
[release/6.0] Fixing the handling of Positive NaN in Math.Min for flo…
Browse files Browse the repository at this point in the history 
…at/double (#70865)

* Fixing the handling of Positive NaN in Math.Min for float/double (#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

* Apply suggestions from code review

Co-authored-by: Jonathan Giannuzzi <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jonathan Giannuzzi <[email protected]>
  • Loading branch information
@tannergooding @jgiannuzzi
tannergooding and jgiannuzzi authored Jul 13, 2022
1 parent 262d14f commit 78111ad
Show file tree
Hide file tree
Showing 4 changed files with 261 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 @@ -831,8 +831,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 @@ -888,8 +888,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 @@ -941,8 +941,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 @@ -978,12 +978,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 @@ -1030,12 +1035,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 @@ -1078,8 +1088,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 @@ -216,8 +216,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 @@ -246,8 +246,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 @@ -1103,6 +1103,29 @@ public static void Max_Decimal()
public static void Max_Double_NotNetFramework(double x, double y, double expectedResult)
{
AssertEqual(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);

AssertEqual(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);

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

[Fact]
Expand Down Expand Up @@ -1161,6 +1184,29 @@ public static void Max_SByte()
public static void Max_Single_NotNetFramework(float x, float y, float expectedResult)
{
AssertEqual(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);

AssertEqual(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);

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

[Fact]
Expand Down Expand Up @@ -1226,6 +1272,29 @@ public static void Min_Decimal()
public static void Min_Double_NotNetFramework(double x, double y, double expectedResult)
{
AssertEqual(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);

AssertEqual(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);

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

[Fact]
Expand Down Expand Up @@ -1284,6 +1353,29 @@ public static void Min_SByte()
public static void Min_Single_NotNetFramework(float x, float y, float expectedResult)
{
AssertEqual(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);

AssertEqual(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);

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

[Fact]
Expand Down Expand Up @@ -2853,6 +2945,29 @@ public static void Log2(double value, double expectedResult, double allowedVaria
public static void MaxMagnitude(double x, double y, double expectedResult)
{
AssertEqual(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);

AssertEqual(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);

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

[Theory]
Expand All @@ -2866,6 +2981,29 @@ public static void MaxMagnitude(double x, double y, double expectedResult)
public static void MinMagnitude(double x, double y, double expectedResult)
{
AssertEqual(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);

AssertEqual(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);

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

[Theory]
Expand Down Expand Up @@ -2934,7 +3072,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 78111ad

Please sign in to comment.