-
Notifications
You must be signed in to change notification settings - Fork 237
fmaximum,fminimum: Fix incorrect result and add tests #939
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
base: master
Are you sure you want to change the base?
Conversation
After adding tests, the current implementation for fminimum fails when provided a negative zero and NaN as inputs: ---- math::fminimum_fmaximum_num::tests::fmaximum_num_spec_tests_f64 stdout ---- thread 'math::fminimum_fmaximum_num::tests::fmaximum_num_spec_tests_f64' panicked at libm/src/math/fminimum_fmaximum_num.rs:240:13: fmaximum_num(-0x0p+0, NaN) l: NaN (0x7ff8000000000000) r: -0.0 (0x8000000000000000) ---- math::fminimum_fmaximum_num::tests::fmaximum_num_spec_tests_f32 stdout ---- thread 'math::fminimum_fmaximum_num::tests::fmaximum_num_spec_tests_f32' panicked at libm/src/math/fminimum_fmaximum_num.rs:240:13: fmaximum_num(-0x0p+0, NaN) l: NaN (0x7fc00000) r: -0.0 (0x80000000) Add more thorough spec tests for these functions and correct the implementations.
The tests were being skipped because I did not restrict compiler-builtins/libm-test/src/precision.rs Lines 447 to 454 in 4f943d4
copysign as intended. I will fix that in a follow up.
|
`binop_common` emits a `SKIP` that is intended to apply only to `copysign`, but is instead applying to all binary operators. Correct the general case but leave the currently-failing `maximum_num` tests as a FIXME, to be resolved separately in [1]. Also simplify skip logic and NaN checking, and add a few more `copysign` checks. [1]: rust-lang#939
`binop_common` emits a `SKIP` that is intended to apply only to `copysign`, but is instead applying to all binary operators. Correct the general case but leave the currently-failing `maximum_num` tests as a FIXME, to be resolved separately in [1]. Also simplify skip logic and NaN checking, and add a few more `copysign` checks. [1]: rust-lang/compiler-builtins#939
`binop_common` emits a `SKIP` that is intended to apply only to `copysign`, but is instead applying to all binary operators. Correct the general case but leave the currently-failing `maximum_num` tests as a FIXME, to be resolved separately in [1]. Also simplify skip logic and NaN checking, and add a few more `copysign` checks. [1]: rust-lang/compiler-builtins#939
//! - Non-NaN if one operand is NaN | ||
//! - qNaN if both operands are NaNx | ||
//! | ||
//! Excluded from our implementation is sNaN handling. |
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 new implementation will definitely return sNaN
for cases like fminimum_num(sNaN, sNaN)
, contradicting the claim that a qNaN
is returned. IMO, it's reasonable to not consider non-default floating point environments since Rust doesn't support changing that, but the quieting should happen even with the default fenv.
The previous implementation at least attempted (by returning res * 1.0
), which would be correct if *
behaved as IEEE754 multiplication, but due to LLVM optimizing x * 1.0
to x
and x * -1.0
to -x
(among others), Rust's current semantics allow nondeterministically propagating sNaN without quieting.
If there's no better way to do it (like something that would generate llvm.canonicalize ), I would prefer to at least preserve the attempted canonicalization with the rationale that nondeterministically sometimes correct is better than deterministically always wrong.
(The same also applies to the other functions.)
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 problem with the res * 1.0
part was that it seemed to only do something on riscv, no other architectures. Is there a reason it would work in more places? I don't mind adding it back as long as it's not a nop on everything but one platform.
Cc @beetrees since we talked about this on the other thread
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.
In debug builds it seems to do the right thing at least on x86, presumably others as well.
Miri should also be applying nondeterministic nan-propagation.
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.
Relatedly, the current CI-failures on riscv64 seem to be due to:
fmin
&fmax
still do theres * 1.0
- riscv's native float operations always return the canonical NaN (positive quiet NaN with payload 0)
- the tests expect
fmin(-NaN, -NaN)
to return-NaN
I believe the correct thing here is to relax the tests, and not guarantee any specific qNaN
After adding tests, the current implementation for fminimum fails when provided a negative zero and NaN as inputs:
Add more thorough spec tests for these functions and correct the implementations.
ci: allow-many-extensive
ci: allow-regressions