fmaximum,fminimum: Fix incorrect result and add tests

[tgross35]

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

[tgross35]

The tests were being skipped because I did not restrict

compiler-builtins/libm-test/src/precision.rs

Lines 447 to 454 in 4f943d4

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

[quaternic]

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

[tgross35]

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

[quaternic]

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.

[quaternic]

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

[tgross35]

I've updated this to relax the tests returning NEG_NAN and retain canonicalization. Would you mind giving this a review?

[beetrees]

If the * F::ONE cannonicalization is being retained, it would be good to update the comment above it to have a FIXME saying that the cannonicalization isn't garunteed to actually work and that it should be replaced with llvm.canonicalize or equivalent in the future. In particular, res * 1.0 not getting optimised to res on RISC-V is just a missed optimisation that could be fixed at any time.

[tgross35]

Updated to make this a trait method with a FIXME.

[tgross35]

@beetrees or @quaternic any other thoughts on this PR or does it seem find in it's current state? I probably need to merge something in the next day or two so it has a chance to get synced in before the beta branch.

[beetrees]

LGTM

[quaternic]

LGTM

[tgross35]

Thank you for reviewing!