Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 2, 2025

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.

ci: allow-many-extensive
ci: allow-regressions

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.
@tgross35
Copy link
Contributor Author

tgross35 commented Jun 2, 2025

The tests were being skipped because I did not restrict

// MPFR only has one NaN bitpattern; allow the default `.is_nan()` checks to validate. Skip if
// the first input (magnitude source) is NaN and the output is also a NaN, or if the second
// input (sign source) is NaN.
if ctx.basis == CheckBasis::Mpfr
&& ((input.0.is_nan() && actual.is_nan() && expected.is_nan()) || input.1.is_nan())
{
return SKIP;
}
to copysign as intended. I will fix that in a follow up.

tgross35 added a commit to tgross35/compiler-builtins that referenced this pull request Jun 2, 2025
`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
tgross35 added a commit to tgross35/rust that referenced this pull request Jun 3, 2025
`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
kiseitai3 pushed a commit to kiseitai3/rust that referenced this pull request Jun 6, 2025
`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
Comment on lines +9 to 12
//! - Non-NaN if one operand is NaN
//! - qNaN if both operands are NaNx
//!
//! Excluded from our implementation is sNaN handling.
Copy link
Contributor

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.)

Copy link
Contributor Author

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

Copy link
Contributor

@quaternic quaternic Jun 6, 2025

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.

Copy link
Contributor

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 the res * 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants