Remove CoreCLR math.h CRT PAL Redefines #98048
Conversation
…consistently across platforms into a library in src/native that we can use across our libraries (or remove).
jkoritzinsky
requested review from
BrzVlad,
vargaz,
kotlarmilos,
lambdageek,
SamMonoRT,
thaystg,
marek-safar and
MichalStrehovsky
as code owners
|
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsIn the CoreCLR PAL, we redefine some of the math.h functions. These functions were redefined to provide consistent behavior in various boundary conditions (such as getting a byte-for-byte exact answer in some scenarios). These APIs were used by CoreCLR and RyuJIT, but were not used by NativeAOT or Mono, so users could get unexpectedly different behavior (NativeAOT in particular, where a constant-propagated result would have the result from the PAL, but a non-const-prop result would have the underlying CRT function's results instead). This PR extracts these APIs from the CoreCLR PAL and moves them into a library under src/native to provide this consistent functionality across all runtimes for these APIs. This does not change the experience for the LLVM-backend, which uses LLVM-intrinsics today to implement these functions.
|
ghost
assigned 
|
The mono changes look ok to me. |
|
@tannergooding @jkotas instead of having the separate |
…efine the double overloads.
Diff results for #98048Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64MinOpts (-0.01% to +0.00%)
Throughput diffs for windows/arm64 ran on windows/x64MinOpts (-0.00% to +0.01%)
Details here |
jkoritzinsky
removed request for
vargaz,
marek-safar,
thaystg,
BrzVlad,
kotlarmilos,
MichalStrehovsky and
SamMonoRT
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@tannergooding theres some JitStress failures for TensorPrimitives, but the TC=0 and TC=1 lanes are both green for all numerics tests (there's one timeout in Dataflow which I don't think is related). Let me know if you think there's any more test lanes I should run here. |
The
At a glance looks like this is failing for x86 in outerloop due to a timeout (which Jakob fixed earlier today) and more recently for Arm64 under JitStress2 due to some bug in |
|
Now that there's no Mono or NativeAOT changes, I don't think we'd get any additional coverage by running extra-platforms I'll remove the mono infra change, resolve the merge conflicts, and rerun CI. |
|
Cleaned everything up I wanted to clean up in this PR. This is ready for final review before merging. |
| #define acos DUMMMY_acos | ||
| #define asin DUMMMY_asin | ||
| #define atan2 DUMMMY_atan2 | ||
| #define exp DUMMMY_exp |
There was a problem hiding this comment.
@jkoritzinsky I have noticed that you have by accident used DUMMMY instead of DUMMY prefix (tripple M instead of double). While this doesn't have any effect on the functionality, it would be nice to change that eventually.
There was a problem hiding this comment.
I'll clean this up in the next week or so (either by fixing it or enabling the usage of standard CRT headers by removing the rest of the overrides)
In the CoreCLR PAL, we redefine some of the math.h functions. These functions were redefined to provide consistent behavior in various boundary conditions (such as getting a byte-for-byte exact answer in some scenarios).
These APIs were used by CoreCLR and RyuJIT, but were not used by NativeAOT or Mono, so users could get unexpectedly different behavior (NativeAOT in particular, where a constant-propagated result would have the result from the PAL, but a non-const-prop result would have the underlying CRT function's results instead).
This PR extracts these APIs from the CoreCLR PAL and moves them into a library under src/native to provide this consistent functionality across all runtimes for these APIs.
This does not change the experience for the LLVM-backend, which uses LLVM-intrinsics today to implement these functions.