Some cleanup of the Math functions from #20788 #20912
Conversation
| 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 |
There was a problem hiding this comment.
This is a breaking change, but matches what the C Runtime does and what the IEEE spec defines
|
The |
| { | ||
| return val1; | ||
| } | ||
|
|
||
| return val2; | ||
| // We do this comparison first and separately to handle the -0.0 to +0.0 comparision |
There was a problem hiding this comment.
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".
|
CC. @AndyAyersMS, @eerhardt who have reviewed the past Math API additions. Also CC. @ViktorHofer who is a System.Numerics area owner and @danmosemsft who raised the |
| #if !HAVE_COMPATIBLE_ILOGB0 | ||
| if (x == 0.0) | ||
| { | ||
| ret = -2147483648; |
There was a problem hiding this comment.
Would it be better if we had a constant with a descriptive name for these?
There was a problem hiding this comment.
There are existing constants (FP_ILOGB0 and FP_ILOGBNAN) and they have incorrect values. I don't think we want to have another name which might cause more confusion for what is only handled in two places here.
There was a problem hiding this comment.
(we also couldn't consume the constant everywhere, such as the configure.cmake files; which might make it even more confusing)
| // 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)) |
There was a problem hiding this comment.
it is super unfortunate that we went from val1 and val2 in other methods to x and y here...
There was a problem hiding this comment.
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.
* Fixing Max, MaxMagnitude, Min, and MinMagnitude for Math/MathF to be IEEE compliant * Disabling the System.Math.Max/Min tests * Adding the new c_runtime PAL tests to the CMakeLists to ensure they actually get run. * Fixing the casing of IlogB to ILogB * Fixing the new PAL tests to match the correct/expected outputs * Fixing up PAL_ilogb to correctly handle 0 and NaN Signed-off-by: dotnet-bot <[email protected]>
* Fixing Max, MaxMagnitude, Min, and MinMagnitude for Math/MathF to be IEEE compliant * Disabling the System.Math.Max/Min tests * Adding the new c_runtime PAL tests to the CMakeLists to ensure they actually get run. * Fixing the casing of IlogB to ILogB * Fixing the new PAL tests to match the correct/expected outputs * Fixing up PAL_ilogb to correctly handle 0 and NaN Signed-off-by: dotnet-bot <[email protected]>
* Fixing Max, MaxMagnitude, Min, and MinMagnitude for Math/MathF to be IEEE compliant * Disabling the System.Math.Max/Min tests * Adding the new c_runtime PAL tests to the CMakeLists to ensure they actually get run. * Fixing the casing of IlogB to ILogB * Fixing the new PAL tests to match the correct/expected outputs * Fixing up PAL_ilogb to correctly handle 0 and NaN Signed-off-by: dotnet-bot <[email protected]>
* Fixing Max, MaxMagnitude, Min, and MinMagnitude for Math/MathF to be IEEE compliant * Disabling the System.Math.Max/Min tests * Adding the new c_runtime PAL tests to the CMakeLists to ensure they actually get run. * Fixing the casing of IlogB to ILogB * Fixing the new PAL tests to match the correct/expected outputs * Fixing up PAL_ilogb to correctly handle 0 and NaN Signed-off-by: dotnet-bot <[email protected]>
* Fixing Max, MaxMagnitude, Min, and MinMagnitude for Math/MathF to be IEEE compliant * Disabling the System.Math.Max/Min tests * Adding the new c_runtime PAL tests to the CMakeLists to ensure they actually get run. * Fixing the casing of IlogB to ILogB * Fixing the new PAL tests to match the correct/expected outputs * Fixing up PAL_ilogb to correctly handle 0 and NaN Signed-off-by: dotnet-bot <[email protected]>
…coreclr#20912) * Fixing Max, MaxMagnitude, Min, and MinMagnitude for Math/MathF to be IEEE compliant * Disabling the System.Math.Max/Min tests * Adding the new c_runtime PAL tests to the CMakeLists to ensure they actually get run. * Fixing the casing of IlogB to ILogB * Fixing the new PAL tests to match the correct/expected outputs * Fixing up PAL_ilogb to correctly handle 0 and NaN Commit migrated from dotnet/coreclr@a49296e
This fixes a few issues that were in #20788: