Enable Non-determinism of float operations in Miri and change std tests #138062

LorrensP-2158466 · 2025-03-05T14:37:39Z

Links to #4208 and #3555 in Miri.

Non-determinism of floating point operations was disabled in #137594 because it breaks the tests and doc-tests in core/coretests and std.

This PR:

enables the float non-determinism but with a lower relative error of 4ULP instead of 16ULP
changes tests that made incorrect assumptions about the operations not to make that assumption anymore (from assert_eq! to assert_approx_eq!.
changes the assert_approx_eq! macro to allow up to 1e-4 to make the tests pass

TODO:

I didn't touch the doc tests because I do not know nearly enough to come near them :)
probably change the assert_approx_eq to use the same technique as Miri (i.e., using ULP instead of EPSILON)

…ndard specify

…754 and C Standards

…inistic

rustbot · 2025-03-05T14:37:44Z

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-03-05T14:37:46Z

The Miri subtree was changed

cc @rust-lang/miri

tgross35 · 2025-03-05T22:55:41Z

The library changes lgtm, for the rest

r? @RalfJung

src/tools/miri/tests/pass/float.rs

Co-authored-by: Dante Broggi <34220985+Dante-Broggi@users.noreply.github.com>

LorrensP-2158466 · 2025-03-06T11:20:06Z

I didn't touch the doc tests because I do not know nearly enough to come near them :)

I should have clarified: the doc tests fail when running ./x miri --doc --no-fail-fast core coretests std -- f64 f32 because of the extra 4ULP error.

src/tools/miri/src/intrinsics/mod.rs

RalfJung · 2025-03-09T19:19:46Z

src/tools/miri/src/intrinsics/mod.rs

+                        2, // log2(4)
+                    );
+
+                    // Clamp values to the output range defined in IEEE 754 9.1.


We said we're going to follow the C standard, which is more permissive than the IEEE spec. Is there a reference for this in the C standard?

Yes and no...

For some operations it specifies it like this:

The atan functions return arctan x in the interval [−π/2, +π/2] radians

But for sin and cos like this:

The sin functions return sin x

If I read "returns sin x", I understand it as "the output is [-1, +1]", but maybe that's just me.

src/tools/miri/src/intrinsics/mod.rs

RalfJung · 2025-03-09T19:20:50Z

src/tools/miri/src/intrinsics/mod.rs

+                let fixed_res = match (f1.category(), f2.category()) {
+                    // 1^y = 1 for any y even a NaN.
+                    // TODO: C Standard says any NaN, IEEE says not a Signaling NaN
+                    (Category::Normal, _) if f1 == 1.0f32.to_soft() => Some(1.0f32.to_soft()),


Can you avoid using soft floats here?

In the miri issue you said this:

except that the logic for fixed_res and clamp has to be done with soft-floats.

Did I misunderstand what you meant?

Argh sorry, I meant "avoid using hard floats" :)

RalfJung · 2025-03-09T19:22:11Z

src/tools/miri/src/intrinsics/mod.rs

+                let fixed_res = match (f1.category(), f2.category()) {
+                    // 1^y = 1 for any y even a NaN.
+                    // TODO: C says any NaN, IEEE says no a Sign NaN
+                    (Category::Normal, _) if f1 == 1.0f64.to_soft() => Some(1.0f64.to_soft()),


Please do not duplicate the same logic 4 times. We were a bit lazy here so far as the boilerplate for making the code generic was bigger than the code, but with all this special-case handling that is no longer the case.

src/tools/miri/src/intrinsics/mod.rs

RalfJung · 2025-03-09T19:23:05Z

src/tools/miri/tests/pass/float.rs

-        // accept up to 64ULP (16ULP for host floats and 16ULP for miri artificial error and 32 for any rounding errors)
-        assert_approx_eq!($a, $b, 64);
+        // accept up to 52ULP (16ULP for host floats, 4ULP for miri artificial error and 32 for any rounding errors)
+        assert_approx_eq!($a, $b, 52);


Do we really still need 52 ULP? I would have hoped that 32 works now.

Indeed 32 works, don't know why I didn't tried it.

I tried lower but got some ULP differences of 27.

RalfJung · 2025-03-09T19:23:41Z

src/tools/miri/tests/pass/float.rs

+    // TODO: How to test NaN inputs? f*::NAN is not guaranteed
+    // to be any specific bit pattern (in std).


It is guaranteed to be a NaN though. Why do you want a specific bit pattern?

IEEE is not as permissive as C, so it doesn't matter much now, just one case: C standard says for pown(powi) the following:

pown(x, 0) returns 1 for all x not a signaling NaN

And since std doesn't guarantee the specific bit pattern of NaN, I'm not quite sure how to test it.

Oh, since NAN might be signaling? That'd be quite bad.^^

Please rely on it being not signalling, I'll see if we can fix the docs.

src/tools/miri/tests/pass/float.rs

RalfJung · 2025-03-09T19:25:42Z

I should have clarified: the doc tests fail when running ./x miri --doc --no-fail-fast core coretests std -- f64 f32 because of the extra 4ULP error.

it is failing the tests that use f32::EPSILON, right? Can you change that to 4.0 * f32::EPSILON (or maybe more if needed)? Once we know the change that makes the tests pass, we can ask the libs folks if that is reasonable.

RalfJung · 2025-03-09T19:27:12Z

library/std/tests/floats/lib.rs

 macro_rules! assert_approx_eq {
-    ($a:expr, $b:expr) => {{ assert_approx_eq!($a, $b, 1.0e-6) }};
+    ($a:expr, $b:expr) => {{ assert_approx_eq!($a, $b, 1.0e-4) }};


I think I'd rather not change this for all tests, and in particular not for f64.

An alternative would be to just set this precision with the affected tests in library/std/tests/floats/f32.rs.

Maybe the precision should be only loosened only with cfg!(miri) as well. I think it is beneficial to learn which platforms, if any, have worse precision than we expect and address that on a case-by-case basis.

…olerance to pass the tests in miri

LorrensP-2158466 · 2025-03-10T19:23:37Z

So I:

only used 1e-4 on failed tests in std, so 1e-6 is back.
increased epsilon threshold in the failed doctests to 4.0 or 8.0
used a macro instead of the duplicate logic in the pow intrinsic. I didn't find another way to cleanly do this + I don't know where I should put this macro.

RalfJung · 2025-03-10T19:33:35Z

No, a macro is not right. The way to avoid this duplication is to add a function that is generic over the apfloat float type.

I'd say just remove pow from the PR for now, let's use sin/cos to test out the waters. Already there the code duplication bothers me but it's less bad.

LorrensP-2158466 · 2025-03-10T19:41:50Z

Yeah Sorry, I was confused with your comment: avoid using soft floats. It should be easier with soft floats. I can try deduplicating sin/cos and pow, and if it isn't what you're looking for, I'll remove them.

RalfJung · 2025-03-10T19:47:23Z

I was confused with your comment: avoid using soft floats.

sorry for that :(

LorrensP-2158466 · 2025-03-10T19:56:20Z

No problems! This float stuff it getting to me too :)

… for powi + a function that clamps a float based on the operation used

LorrensP-2158466 · 2025-03-11T19:10:58Z

So I extended the fixed_float_value to accept powf, but this required accepting a slice of arguments and matching on the size. This pattern matching is getting extensive, but I don't find it confusing atm. powi accepts i32, so I made a separate function to handle that one.

There is still some repetitiveness, like applying the error or adjust_nan, but that's not the focus of this pr.

RalfJung · 2025-03-19T14:53:32Z

src/tools/miri/src/intrinsics/mod.rs

@@ -522,3 +572,121 @@ fn apply_random_float_error_to_imm<'tcx>(

    interp_ok(ImmTy::from_scalar_int(res, val.layout))
 }
+
+// TODO(lorrens): This can be moved to `helpers` when we implement the other intrinsics.


No please only but things in helpers that are widely useful across Miri. Specific functionality should remain in its specific file.

Alright, I thought that this would be the case when implementing the foreign_itmes, since they probably will share this functionality, like asin and such.

Ah yeah... we'll figure that out when we get there.

RalfJung · 2025-03-19T14:54:46Z

src/tools/miri/src/intrinsics/mod.rs

+        // x^(±0) = 1 for any x, even a NaN
+        ("powf32" | "powf64", [_, exp]) if exp.is_zero() => Some(one),
+
+        // C standard doesn't specify or invalid combination


This is not a sentence...?

Yeah, now that I am reading that again... I'll update it, excuse me for my bad English :)

RalfJung · 2025-03-19T14:55:16Z

src/tools/miri/src/intrinsics/mod.rs

+    // TODO: not sure about this pattern matching stuff. It's definitly cleaner than if-else chains
+    // Error code 0158 explains this: https://doc.rust-lang.org/stable/error_codes/E0158.html
+    // The only reason I did this is to use the same function for powf as for sin/cos/exp/log


I don't understand the comment. The code looks nice though :)

Okay, I should have explained why the error code applies here.
Consider this arm:

// sin(+- 0) = +- 0. ("sinf32" | "sinf64", [input]) if input.is_zero() => Some(*input),

I still have to put an if-guard on it to check that input is Zero, but in my opinion, it would be a lot nicer if I could do the following:

// sin(+- 0) = +- 0. ("sinf32" | "sinf64", [IeeeFloat::<S>::Zero]) => Some(*input),

If I then were able to make a constant for 1 and maybe other values, I wouldn't need those if-guards. But unfortunately, the compiler doesn't try to see if this can work; it just assumes this to be non-exhaustive, regardless of a _ => ... arm.

Luckily, you like the code :)

You can't use consts that depend on generics in patterns like that, that is expected.

src/tools/miri/src/intrinsics/mod.rs

RalfJung · 2025-03-19T14:56:32Z

src/tools/miri/tests/pass/float.rs

+    // TODO: How to test NaN inputs? f*::NAN is not guaranteed
+    // to be any specific bit pattern (in std).


Oh, since NAN might be signaling? That'd be quite bad.^^

Please rely on it being not signalling, I'll see if we can fix the docs.

RalfJung · 2025-03-19T14:58:17Z

Regarding the library tests, I suggested a plan above for the doc tests. Have you tried that?

EDIT: Ah, yes seems you did. :) I assume all tests pass now in the current state of the PR?

Also:

probably change the assert_approx_eq to use the same technique as Miri (i.e., using ULP instead of EPSILON)

I am not convinced we want to do that, so unless @tgross35 asks for it I'd say please stick to the current approach.

RalfJung · 2025-03-19T15:00:28Z

@tgross35 could you have a look at the library diff again?
If you prefer, instead of hard-coding 1e-4 I guess we could also have a constant like

// Miri adds some extra error to float functions, make sure the tests still pass.
const APPROX_DETLA: f32 = if cfg!(miri) { 1e-4 } else { 1e-6 };

LorrensP-2158466 · 2025-03-19T19:15:49Z

EDIT: Ah, yes seems you did. :) I assume all tests pass now in the current state of the PR?

Yes, they indeed pass now.

On the topic of doc and doctests, for example, powf:

// part of powf doctest
let x = 2.0_f32;
let abs_difference = (x.powf(2.0) - (x * x)).abs();
assert!(abs_difference <= 8.0 * f32::EPSILON);

This is not wrong, but shouldn't it be noted that this only works with small values?
And I think this applies to a lot of operations.

LorrensP-2158466 added 5 commits March 5, 2025 15:00

add tests to check that the operations behave like the Ieee and C sta…

d10109a

…ndard specify

enable non-determinism but with smaller error, 4ULP; conform to IEEE …

ebb4eda

…754 and C Standards

set 2^100 directly in integer decode tests because powf is non-determ…

71d0edf

…inistic

assert_approx_eq! now accepts up to 1e-4

a078a38

change tests that made wrong assumptions about the Ieee and C standards

ae325e6

rustbot assigned tgross35 Mar 5, 2025

rustbot added S-waiting-on-review T-libs labels Mar 5, 2025

LorrensP-2158466 changed the title ~~Enable Non-determinism of float operations and change std tests~~ Enable Non-determinism of float operations in Miri and change std tests Mar 5, 2025

rustbot assigned RalfJung and unassigned tgross35 Mar 5, 2025

Dante-Broggi reviewed Mar 6, 2025

View reviewed changes

src/tools/miri/tests/pass/float.rs Outdated Show resolved Hide resolved

fix copy/paste mistake

6274c4e

Co-authored-by: Dante Broggi <34220985+Dante-Broggi@users.noreply.github.com>

RalfJung reviewed Mar 9, 2025

View reviewed changes

LorrensP-2158466 added 4 commits March 10, 2025 20:18

apply simple requests from reviewer

b67dbfa

change assert_approx_eq! back to old version and manually set error t…

aa2f5fa

…olerance to pass the tests in miri

change doc-tests to increase EPSILON to make the tests pass

86bb950

deduplicate code of pow functions to use a macro instead

f4d65ad

extend fixed_float_value to accept powf + create fix_float_value fn…

e11a804

… for powi + a function that clamps a float based on the operation used

clean up

9271fda

RalfJung reviewed Mar 19, 2025

View reviewed changes

apply changes from review + minor doc fixes

3caf613

		// TODO: How to test NaN inputs? f*::NAN is not guaranteed
		// to be any specific bit pattern (in std).

Enable Non-determinism of float operations in Miri and change std tests #138062

Are you sure you want to change the base?

Enable Non-determinism of float operations in Miri and change std tests #138062

Conversation

LorrensP-2158466 commented Mar 5, 2025

rustbot commented Mar 5, 2025

rustbot commented Mar 5, 2025

tgross35 commented Mar 5, 2025

LorrensP-2158466 commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Mar 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LorrensP-2158466 commented Mar 10, 2025 • edited Loading

RalfJung commented Mar 10, 2025

LorrensP-2158466 commented Mar 10, 2025

RalfJung commented Mar 10, 2025 via email

LorrensP-2158466 commented Mar 10, 2025

LorrensP-2158466 commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Mar 19, 2025 • edited Loading

RalfJung commented Mar 19, 2025

LorrensP-2158466 commented Mar 19, 2025

LorrensP-2158466 commented Mar 10, 2025 •

edited

Loading

RalfJung commented Mar 19, 2025 •

edited

Loading