-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
Test against MPFR #311
base: tgross35/test-refactoring
Are you sure you want to change the base?
Test against MPFR #311
Conversation
e386722
to
ded5a3e
Compare
2bd6dbb
to
4d51e47
Compare
I think this is ready for a review too @Amanieu. This adds tests against MPFR for almost all functions, which gives us infinite precision results in cases where we want to be more accurate than musl. The few untested functions are at libm/crates/libm-test/tests/multiprecision.rs Lines 47 to 61 in 4d51e47
This targets my |
09d82a7
to
2722bde
Compare
a20b0c8
to
c76cece
Compare
6bf4102
to
f778935
Compare
f778935
to
044142f
Compare
b33d482
to
e661694
Compare
5cb946e
to
56c98a8
Compare
044142f
to
93a4bd9
Compare
bba7079
to
0f22dac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
crates/libm-test/src/mpfloat.rs
Outdated
where | ||
for<'a> &'a MpFloat: az::Cast<F>, | ||
{ | ||
mp.subnormalize_ieee(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will currently cause double rounding for subnormal results, as the result will be rounded once to the full precision and then again to the subnormal precision. Use subnormalize_ieee_round
instead, passing it the Ordering
from the relevant _round
-suffixed function for the operation (e.g. acos_round
) so that the subnormalize function is aware of what rounding previously occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch. Is there a point in asserting that the rounding returned by subnormalize_ieee_round
is Equal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rounding returned from subnormalize_ieee_round
will only be Equal
if no rounding has occured (i.e. the result is exact). Unless you need to know what rounding has occured, you can just ignore the Ordering
returned by subnormalize_ieee_round
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my clarity, is it correct the result will be correctly rounded to 1/2 ULP regardless of what the return value is? Meaning the returned Ordering
provides information about what was truncated rather than about accuracy to a bitwise representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use _round
rather than _mut
and pass the result to prep_retval
.
crates/libm-test/src/xfail.rs
Outdated
let outputs_nan = all_nan(&[actual, expected]); | ||
|
||
match &ctx.basis { | ||
CheckBasis::Musl => match ctx.fname { | ||
// We return +NaN, Musl returns -NaN | ||
"tgammaf" => input.0 < 0.0, | ||
_ => false, | ||
}, | ||
CheckBasis::MultiPrecision => match ctx.fname { | ||
// For almost everything we return -NaN but MPFR does +NaN | ||
_ if (input.0.is_nan() || input.0.is_infinite()) && outputs_nan => true, | ||
// x86 MacOS NaN doesn't seem to depend on input sign | ||
_ if cfg!(x86_macos) && outputs_nan => true, | ||
// Out of domain we return +NaN, MPFR returns -NaN | ||
"atanhf" => input.0 < -1.0 && outputs_nan, | ||
// We return -NaN, MPFR says +NaN | ||
"tgammaf" => input.0 < 0.0 && outputs_nan, | ||
_ => false, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, none of the libm
functions (except for copysign and abs) provide any guarantees about the sign of returned NaNs, so mismatches in NaN signs can just be blanket ignored (except for copysign and abs) instead of carving out exceptions for where they happen to currently mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds cleaner. Do you know if these functions should do anything specific with signaling NaNs they receive as input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do you know if MPFR's operations provide determinism for NaNs? I was thinking that maybe there is some benefit to being deterministic even if not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- According to Rust's NaN guarantees, signalling NaNs can be returned from arthimetic operations if a signalling NaN was part of the input (Rust also restricts the bitpatterns of returned NaNs, but the restrictions are extremely unlikely to be a problem unless a
libm
function implementation deliberately creates a NaN with a custom payload). In practice the allowance for signalling NaNs to be returned is mainly to allow LLVM to do transformations like1.0 * x
=>x
at compile time. - The C23 draft standard recommends (but does not require) that signalling NaNs are quietened (C17, AFAIK from the draft I have access to, doesn't place any requirements on signalling NaN handling).
- The IEEE standard (AFAIK; I don't have access to a copy) states that signaling NaNs should be quietened (except for copysign and abs).
Quietening signalling NaNs is probably the "better" behaviour (in complies with more standards). However, ensuring all NaNs returned are quietened may take some extra implementation effort, as Rust explicitly doesn't guarantee the quietening of signalling NaNs by any floating-point operation (although in practice all arithmetic operations will quieten signalling NaNs unless LLVM has optimised out the operation itself). That said, most function implementations have an explicit if-NaN-return-NaN check at the start, so as long as that returns a quietened NaN everything should be fine (as signalling NaNs cannot be produced by any arithmetic operation that doesn't have a signalling NaN in its input, meaning the only possible source of a signalling NaN is the function arguments).
In summary, I think libm
should quieten signalling NaNs before returning them (or just return f{16,32,64,128}::NAN
instead), but it's not essential.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MPFR will always return the same NaN when converting a MPFR NaN to f32
/f64
, and only has a single kind of NaN (documentation, source code for converting to f32
/f64
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the context, your suggestion sounds reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted so we no longer check for NaN except the cases you mentioned. This logic is in maybe_check_nan_bits
.
93a4bd9
to
12bf64a
Compare
12bf64a
to
8ef0575
Compare
96bada7
to
d3401a2
Compare
8ef0575
to
c3862a3
Compare
Sometimes we want to be able to xfail specific inputs without changing the checked ULP for all cases or skipping the tests. There are also some cases where we need to perform extra checks for only specific functions. Add a trait that provides a hook for providing extra checks or skipping existing checks on a per-function or per-input basis.
Add a way to call MPFR versions of functions in a predictable way, using the `MpOp` trait. Everything new here is guarded by the feature `test-multiprecision` since MPFR cannot easily build on Windows or any cross compiled targets.
This effectively gives us tests against infinite-precision results on MacOS and x86+sse Linux.
Test against MPFR via rug for infinite precision results in cases where musl might not be accurate.