Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Some cleanup of the Math functions from #20788 #20912

Merged
merged 6 commits into from
Nov 12, 2018
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
8 changes: 8 additions & 0 deletions cross/android/arm/tryrun.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ SET( HAVE_COMPATIBLE_EXP_EXITCODE
1
CACHE STRING "Result from TRY_RUN" FORCE)

SET( HAVE_COMPATIBLE_ILOGB0_EXITCODE
0
CACHE STRING "Result from TRY_RUN" FORCE)

SET( HAVE_COMPATIBLE_ILOGBNAN_EXITCODE
0
CACHE STRING "Result from TRY_RUN" FORCE)

SET( REALPATH_SUPPORTS_NONEXISTENT_FILES_EXITCODE
1
CACHE STRING "Result from TRY_RUN" FORCE)
Expand Down
8 changes: 8 additions & 0 deletions cross/android/arm64/tryrun.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@ SET( HAVE_COMPATIBLE_EXP_EXITCODE
1
CACHE STRING "Result from TRY_RUN" FORCE)

SET( HAVE_COMPATIBLE_ILOGB0_EXITCODE
0
CACHE STRING "Result from TRY_RUN" FORCE)

SET( HAVE_COMPATIBLE_ILOGBNAN_EXITCODE
0
CACHE STRING "Result from TRY_RUN" FORCE)

SET( REALPATH_SUPPORTS_NONEXISTENT_FILES_EXITCODE
1
CACHE STRING "Result from TRY_RUN" FORCE)
Expand Down
2 changes: 2 additions & 0 deletions cross/tryrun.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ if(TARGET_ARCH_NAME MATCHES "^(armel|arm|arm64|x86)$")
set_cache_value(HAVE_COMPATIBLE_ACOS_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_ASIN_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_ATAN2_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_ILOGB0_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_ILOGBNAN_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_LOG10_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_LOG_EXITCODE 0)
set_cache_value(HAVE_COMPATIBLE_POW_EXITCODE 0)
Expand Down
140 changes: 122 additions & 18 deletions src/System.Private.CoreLib/shared/System/Math.cs
Original file line number Diff line number Diff line change
Expand Up @@ -538,17 +538,31 @@ public static decimal Max(decimal val1, decimal val2)

public static double Max(double val1, double val2)
{
if (val1 > val2)
// When val1 and val2 are both finite or infinite, return the larger
// * We count +0.0 as larger than -0.0 to match MSVC
// When val1 or val2, but not both, are NaN return the opposite
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a breaking change, but matches what the C Runtime does and what the IEEE spec defines

// * We return the opposite if either is NaN to match MSVC

if (double.IsNaN(val1))
{
return val1;
return val2;
}

if (double.IsNaN(val1))
if (double.IsNaN(val2))
{
return val1;
}

return val2;
// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
Copy link
Member Author

@tannergooding tannergooding Nov 9, 2018

Choose a reason for hiding this comment

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

Just as a note here: This optimization is completely fine for the JIT to do since the IEEE spec indicates that comparisons shall ignore the sign of zero and for Min/Max, the JIT specifies that either x or y can be returned if x == y.

I opted to respect the sign of zero here since the MSVC/GCC/Clang implementation does and since Annex F of the C Language specification (which dictates IEC 60559 compliance) has a footnote indicating that respecting the sign of zero here would be "ideal".

// * Doing (val1 < val2) first could get transformed into (val2 >= val1) by the JIT
// which would then return an incorrect value

if (val1 == val2)
{
return double.IsNegative(val1) ? val2 : val1;
}

return (val1 < val2) ? val2 : val1;
}

[NonVersionable]
Expand Down Expand Up @@ -578,17 +592,31 @@ public static sbyte Max(sbyte val1, sbyte val2)

public static float Max(float val1, float val2)
{
if (val1 > val2)
// When val1 and val2 are both finite or infinite, return the larger
// * We count +0.0 as larger than -0.0 to match MSVC
// When val1 or val2, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (float.IsNaN(val1))
{
return val1;
return val2;
}

if (float.IsNaN(val1))
if (float.IsNaN(val2))
{
return val1;
}

return val2;
// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (val1 < val2) first could get transformed into (val2 >= val1) by the JIT
// which would then return an incorrect value

if (val1 == val2)
{
return float.IsNegative(val1) ? val2 : val1;
}

return (val1 < val2) ? val2 : val1;
}

[CLSCompliant(false)]
Expand All @@ -614,7 +642,31 @@ public static ulong Max(ulong val1, ulong val2)

public static double MaxMagnitude(double x, double y)
{
return Max(Abs(x), Abs(y));
// When x and y are both finite or infinite, return the larger magnitude
// * We count +0.0 as larger than -0.0 to match MSVC
// When x or y, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (double.IsNaN(x))
{
return y;
}

if (double.IsNaN(y))
{
return x;
}

// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (x < y) first could get transformed into (y >= x) by the JIT which would
// then return an incorrect value

if (x == y)
{
return double.IsNegative(x) ? y : x;
}

return (Abs(x) < Abs(y)) ? y : x;
}

[NonVersionable]
Expand All @@ -631,17 +683,31 @@ public static decimal Min(decimal val1, decimal val2)

public static double Min(double val1, double val2)
{
if (val1 < val2)
// When val1 and val2 are both finite or infinite, return the smaller
// * We count -0.0 as smaller than -0.0 to match MSVC
// When val1 or val2, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (double.IsNaN(val1))
{
return val1;
return val2;
}

if (double.IsNaN(val1))
if (double.IsNaN(val2))
{
return val1;
}

return val2;
// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (val1 < val2) first could get transformed into (val2 >= val1) by the JIT
// which would then return an incorrect value

if (val1 == val2)
{
return double.IsNegative(val1) ? val1 : val2;
}

return (val1 < val2) ? val1 : val2;
}

[NonVersionable]
Expand Down Expand Up @@ -671,17 +737,31 @@ public static sbyte Min(sbyte val1, sbyte val2)

public static float Min(float val1, float val2)
{
if (val1 < val2)
// When val1 and val2 are both finite or infinite, return the smaller
// * We count -0.0 as smaller than -0.0 to match MSVC
// When val1 or val2, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (float.IsNaN(val1))
{
return val1;
return val2;
}

if (float.IsNaN(val1))
if (float.IsNaN(val2))
{
return val1;
}

return val2;
// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (val1 < val2) first could get transformed into (val2 >= val1) by the JIT
// which would then return an incorrect value

if (val1 == val2)
{
return float.IsNegative(val1) ? val1 : val2;
}

return (val1 < val2) ? val1 : val2;
}

[CLSCompliant(false)]
Expand All @@ -707,7 +787,31 @@ public static ulong Min(ulong val1, ulong val2)

public static double MinMagnitude(double x, double y)
{
return Min(Abs(x), Abs(y));
// When x and y are both finite or infinite, return the smaller magnitude
// * We count -0.0 as smaller than -0.0 to match MSVC
// When x or y, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (double.IsNaN(x))
Copy link
Member

Choose a reason for hiding this comment

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

it is super unfortunate that we went from val1 and val2 in other methods to x and y here...

Copy link
Member Author

Choose a reason for hiding this comment

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

x and y are likely the better choices here overall. They are the terms used in C/C++ (and a number of other languages), in the IEEE spec, and commonly in mathematics when referring to these functions and their parameters.

If we had the ability to standardize on one, without it being a breaking change, x and y are likely the names I would push for.

{
return y;
}

if (double.IsNaN(y))
{
return x;
}

// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (x < y) first could get transformed into (y >= x) by the JIT which would
// then return an incorrect value

if (x == y)
{
return double.IsNegative(x) ? x : y;
}

return (Abs(x) < Abs(y)) ? x : y;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
52 changes: 50 additions & 2 deletions src/System.Private.CoreLib/shared/System/MathF.cs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,31 @@ public static float Max(float x, float y)

public static float MaxMagnitude(float x, float y)
{
return Max(Abs(x), Abs(y));
// When x and y are both finite or infinite, return the larger magnitude
// * We count +0.0 as larger than -0.0 to match MSVC
// When x or y, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (float.IsNaN(x))
{
return y;
}

if (float.IsNaN(y))
{
return x;
}

// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (x < y) first could get transformed into (y >= x) by the JIT which would
// then return an incorrect value

if (x == y)
{
return float.IsNegative(x) ? y : x;
}

return (Abs(x) < Abs(y)) ? y : x;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand All @@ -201,7 +225,31 @@ public static float Min(float x, float y)

public static float MinMagnitude(float x, float y)
{
return Min(Abs(x), Abs(y));
// When x and y are both finite or infinite, return the smaller magnitude
// * We count -0.0 as smaller than -0.0 to match MSVC
// When x or y, but not both, are NaN return the opposite
// * We return the opposite if either is NaN to match MSVC

if (float.IsNaN(x))
{
return y;
}

if (float.IsNaN(y))
{
return x;
}

// We do this comparison first and separately to handle the -0.0 to +0.0 comparision
// * Doing (x < y) first could get transformed into (y >= x) by the JIT which would
// then return an incorrect value

if (x == y)
{
return float.IsNegative(x) ? x : y;
}

return (Abs(x) < Abs(y)) ? x : y;
}

[Intrinsic]
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/src/System/Math.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static partial class Math
public static extern double FusedMultiplyAdd(double x, double y, double z);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int IlogB(double x);
public static extern int ILogB(double x);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern double Log(double d);
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/src/System/MathF.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public static partial class MathF
public static extern float FusedMultiplyAdd(float x, float y, float z);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int IlogB(float x);
public static extern int ILogB(float x);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern float Log(float x);
Expand Down
2 changes: 1 addition & 1 deletion src/classlibnative/float/floatdouble.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ FCIMPLEND
/*=====================================Ilog2====================================
**
==============================================================================*/
FCIMPL1_V(int, COMDouble::IlogB, double x)
FCIMPL1_V(int, COMDouble::ILogB, double x)
FCALL_CONTRACT;

return (int)ilogb(x);
Expand Down
2 changes: 1 addition & 1 deletion src/classlibnative/float/floatsingle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ FCIMPLEND
/*=====================================Ilog2====================================
**
==============================================================================*/
FCIMPL1_V(int, COMSingle::IlogB, float x)
FCIMPL1_V(int, COMSingle::ILogB, float x)
FCALL_CONTRACT;

return (int)ilogbf(x);
Expand Down
2 changes: 1 addition & 1 deletion src/classlibnative/inc/floatdouble.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class COMDouble {
FCDECL1_V(static double, Floor, double x);
FCDECL2_VV(static double, FMod, double x, double y);
FCDECL3_VVV(static double, FusedMultiplyAdd, double x, double y, double z);
FCDECL1_V(static int, IlogB, double x);
FCDECL1_V(static int, ILogB, double x);
FCDECL1_V(static double, Log, double x);
FCDECL1_V(static double, Log2, double x);
FCDECL1_V(static double, Log10, double x);
Expand Down
2 changes: 1 addition & 1 deletion src/classlibnative/inc/floatsingle.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class COMSingle {
FCDECL1_V(static float, Floor, float x);
FCDECL2_VV(static float, FMod, float x, float y);
FCDECL3_VVV(static float, FusedMultiplyAdd, float x, float y, float z);
FCDECL1_V(static int, IlogB, float x);
FCDECL1_V(static int, ILogB, float x);
FCDECL1_V(static float, Log, float x);
FCDECL1_V(static float, Log2, float x);
FCDECL1_V(static float, Log10, float x);
Expand Down
2 changes: 2 additions & 0 deletions src/pal/src/config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@
#cmakedefine01 HAVE_VALID_POSITIVE_INF_POW
#cmakedefine01 HAVE_COMPATIBLE_ATAN2
#cmakedefine01 HAVE_COMPATIBLE_EXP
#cmakedefine01 HAVE_COMPATIBLE_ILOGB0
#cmakedefine01 HAVE_COMPATIBLE_ILOGBNAN
#cmakedefine01 HAVE_COMPATIBLE_LOG
#cmakedefine01 HAVE_COMPATIBLE_LOG10
#cmakedefine01 UNGETC_NOT_RETURN_EOF
Expand Down
Loading