Removing the special handling in classlibnative for atan2(±∞, ±∞) and pow(-1.0, ±∞).#10295
Removing the special handling in classlibnative for atan2(±∞, ±∞) and pow(-1.0, ±∞).#10295danmoseley merged 2 commits intodotnet:masterfrom tannergooding:ieee-math
Conversation
|
FYI. @mellinoe |
|
Also fixed the logic for Pow in the PAL layer to match the current windows behavior and the IEEE spec. This should resolve https://github.com/dotnet/coreclr/issues/10289 Edit: The |
|
The |
|
The OSX10.12 job is failing, but in a manner unrelated to this PR. |
|
OSX10.12 jobs in general seem to have been failing for a while now. |
|
This should be good now, @mellinoe. |
|
Someone more familiar with the native code here should probably review this, but I'm supportive of the direction. |
|
tagging @janvorli, @AndyAyersMS, and @jkotas, who reviewed things last time I touched this code 😄 |
dcwuser
left a comment
There was a problem hiding this comment.
This is a good change. I'm particularly pleased to see that it is accomplished by removing unnecessary checks instead of adding new ones.
| { -2.7182818284590452, 1, -2.7182818284590452, PAL_EPSILON * 10 }, // x: -(e) expected: e | ||
| { -2.7182818284590452, PAL_POSINF, PAL_POSINF, 0 }, // x: -(e) | ||
|
|
||
| { -1.0, PAL_NEGINF, 1.0, PAL_EPSILON * 10 }, |
There was a problem hiding this comment.
These are exact results. Why do we allow variance PAL_EPSILON * 10 instead of PAL_EPSILON or 0?
There was a problem hiding this comment.
I have a work item to clean up these tests in general (actually doing validation for exact values, and against negative zero, etc). I've already done this in the corefx pr (which adds the managed version of these tests), for the most part. I just haven't gotten to it here yet.
There was a problem hiding this comment.
As for why PAL_EPSILON * 10, the comment on PAL_EPSILON explains.
Basically, we are allowed 15-17 digits of precision. That is 0.xxxxxxxxxxxxxxxxx (validate against PAL_EPSILON) or x.xxxxxxxxxxxxxxxx (PAL_EPSILON * 10) or xx.xxxxxxxxxxxxxxx (PAL_EPSILON * 100) or even 0.0xxxxxxxxxxxxxxxxx (PAL_EPSILON / 10).
If we just used PAL_EPSILON everywhere, we would get incorrect results for larger or smaller results.
There was a problem hiding this comment.
I'm not talking about everywhere, I'm talking about the specific tests you added in this PR, where we can expect the result to be exact, e.g. (-1)^(-inf) = 1.0, not 1.0 +/ 1.0E-15.
There was a problem hiding this comment.
Yes, its probably the case that we can expect an exact result here. I was just being consistent with the existing convention here.
There are plenty of things that could be improved. For example, the PAL layer (as a compatibility test) currently checks if atan2(0.0, -0.1) is within 0.0000001 digits of PI, (https://github.com/dotnet/coreclr/blob/master/src/pal/src/configure.cmake#L797), such logic is incredibly broken 😄
I have been working on a separate PR that fixes all of this up, and it will be corrected when that goes out. This PR, at the very least, will fix the validate function to properly check:
- Exact results
- NaN
- -0.0
- +0.0
- Negative Infinity
- Positive Infinity
- Results that are within 15 digits of the expected value
There was a problem hiding this comment.
That being said, if you feel strongly about this going in as inexact (especially considering the 1500 other scenarios we validate that are also inexact), I can fix it now, rather than as a whole in the separate PR.
There was a problem hiding this comment.
No, this is very minor. I just wanted to understand the reason. It's fine to fix them all in one go later.
|
test OSX10.12 x64 Checked Build and Test please Looks like jobs are passing now. Will rebase if it fails again. |
|
Ping Could I get a review of the PAL/native changes here. |
|
ping of @janvorli, @AndyAyersMS, and @jkotas could someone look? |
jkotas
left a comment
There was a problem hiding this comment.
The floating point functions in the PAL exist just to support System.Math, they are not used for anything else.
The behavior of System.Math functions in corner cases will differ from full framework after this change - the tests will need to be special cased for netfx. I think it is fine because of it aligns us with the standard behavior.
… pow(-1.0, ±∞). (dotnet#10295) * Removing the special handling in classlibnative for atan2(±∞, ±∞) and pow(-1.0, ±∞). * Fixing up the logic for HAVE_COMPATIBLE_POW in the PAL layer.
We need test updates from dotnet/coreclr#10295, but updating the entire test suite is kind of expensive because it will need rebaselining.
We need test updates from dotnet/coreclr#10295, but updating the entire test suite is kind of expensive because it will need rebaselining.
These functions were incorrectly adjusting the handling for specific edge cases and making them differ from the result specified in IEEE 754.
This resolves https://github.com/dotnet/coreclr/issues/9806 and https://github.com/dotnet/coreclr/issues/9807