Skip to content

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

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.

Sign up for GitHub

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

Merged
tannergooding merged 4 commits into from
Jun 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a68b3e
Adding tests validating Positive NaN for Max, MaxMagnitude, Min, and …
tannergooding Jun 15, 2022
3bfa000
Fixing the handling of Positive NaN in Math.Min for float/double
tannergooding Jun 15, 2022
2242d42
Fixing the Max/Min code comments to use greater and lesser
tannergooding Jun 16, 2022
819387f
Adding a code comment clarifying the sign toggling behavior
tannergooding Jun 16, 2022
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
42 changes: 26 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Math.cs
View file
Open in desktop
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)
Comment thread
tannergooding marked this conversation as resolved.
{
return val1 < val2 ? val1 : val2;
if (!double.IsNaN(val1))

@danmoseley danmoseley Jun 21, 2022

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.

Just curious, when we write C# that's potentially in a hot path is there any value in putting "most likely branch first" (ie., "test is most likely true")? This would mean that the codegen will maintain this ordering + the CPU in the absence of other information will assume the branch is true + this isn't overwhelmed by accurate subsequent branch prediction?

I only ask because I'd personally find something like this easier to grok if the double.IsNaN was not negated.

@tannergooding tannergooding Jun 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's why the code is how it is. With the way the C# emits IL and the JIT compiles it, then by default (that is when no PGO data is present), the code is effectively:

if (cond)
{
    // predicted
}

// unpredicted

It's the opposite for ternaries:

cond ? unpredicted : predicated

This ordering ensures that the most predicted path is the return val1 < val2 ? val1 : val2; scenario which makes a decently measurable impact on perf. We could potentially improve our PGO data here, but since the Math methods are generally always considered "hot", its worth micro-optimizing anyways.

@danmoseley danmoseley Jun 21, 2022

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.

Interesting. Why are ternaries the reverse of what I'd expect they'd be if they were translated into "if/else"?

Ideally we'd have POGO for anything "hot" so we didn't have to write our code like this.

Curious whether @AndyAyersMS @stephentoub have any color to add here

@tannergooding tannergooding Jun 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think the BCL is too large to have accurate PGO data for many of these functions, there are likewise cases where PGO data will mess up or hinder normal workflows.

Many of the Math functions in particular are things which should be intrinsic or considered "always hot". They are also functions where the default predicted case and the actual case often don't line up. Many native compilers explicitly treat them as 50/50 and "always inline" in which case the PGO data is irrelevant.

These functions are actually prime examples of where we probably (longer term) want to make them branchless by default (using SIMD and/or Conditional Instructions) and where we'd only change that with dynamic PGO that indicates otherwise for a particular call-site.

@AndyAyersMS AndyAyersMS Jun 21, 2022

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 would recommend in most cases writing the code whatever way seems clearest and not worrying about this level of detail. There is no guarantee the jit will maintain IL order for emitted code.

@tannergooding tannergooding Jun 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The BCL is filled with code that cares about ordering, especially in the hottest functions (such as for Span, etc).

I expect if the JIT ever changes that we will have a large number of regressions in the perf triage and a higher push for JIT hints to specify static data around "likely" vs "unlikely".

@AndyAyersMS AndyAyersMS Jun 21, 2022

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.

We change the code layout and branch optimization logic in the jit pretty frequently and more changes are in the plans for the future. I don't know how to reconcile that with what you are saying.

@tannergooding tannergooding Jun 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We make tweaks yes, but even then the JIT typically defaults to (barring PGO data telling it otherwise), matching the order so that if (cond) { predicted } unpredicted. Part of this is just because its the implied execution order but also because it produces the smallest (and generally fastest) code.

Changing this causes the JIT to do more work because it either has to insert more branches or rearrange blocks and that's not worth doing unless we have data (like PGO) indicating otherwise.

Functions like Math.Min are core functions that are decently hot and so having them ordered such that the default codegen is better, is general goodness. Changing it would show up in our weekly perf triage and result in it getting reverted back. If we wanted to change it, we'd need additional functionality so that we could statically tell the JIT the "intended" behavior.

{
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
Open in desktop
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
Open in desktop
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);
Comment thread
tannergooding marked this conversation as resolved.

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