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)

try-job: x86_64-gnu-aux

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

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.

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

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.

RalfJung · 2025-03-30T18:34:31Z

r? @tgross35

to reflect the current status. Feel free to re-assign to me if the libs changes are fine.

LorrensP-2158466 · 2025-04-02T11:20:28Z

@RalfJung should this PR also alter the algebraic operations that were mentioned in #136457?

RalfJung · 2025-04-02T15:41:52Z

Let's avoid a conflict with the other in-flight PR, #136457. So let's deal with the algebraic and fast-math operations separately.

LorrensP-2158466 · 2025-05-23T13:39:13Z

Yes, I'm sorry for the lack of updates. I do plan to finish this pr, a lot of changes are still local because they are not done yet.

All my project deadlines are this and next week... so yeah :)

I expect to push all my changes no later than Monday.

RalfJung · 2025-05-23T13:53:59Z

No worries, and thanks for the update. :)
I'd just hate for all this work to just go to waste because we don't finish this.

(Also, my next review will then take a lot longer as this PR has been fully swapped out of my short-term memory by now.^^)

LorrensP-2158466 · 2025-05-23T13:59:37Z

I'd just hate for all this work to just go to waste because we don't finish this.

Don't worry :), I'm finding this really fun to do, but also somewhat hard, so I like to give it my full attention when I'm working on it.

(Also, my next review will then take a lot longer as this PR has been fully swapped out of my short-term memory by now.^^)

😆, completely understandable. When I push my changes, I'll post a status of the pr.

LorrensP-2158466 · 2025-05-24T20:52:17Z

The current context is as follows.

We only create a fixed value when it is non-zero, these can be found in miri/src/intrinsics/mod.rs at the bottom of the file. One is for the operations where all args are floats and the other for powi. We test these in the float tests of Miri.

The library tests (and doctests) were changed so that they don't fail under Miri and Trevor approved these, however when testing again today, f32::log2 also failed 2 tests, so I changed these the same way as the others.

We also added some tests for pow(SNaN, 0), but these are not entirely correct, but as we know this is tracked in Miri/#4286.

LorrensP-2158466 · 2025-05-24T20:55:04Z

I also tried to rebase the upstream master branch because my fork is a bit outdated. :)
But this affects my branch quite a bit; the changes I made in Miri are now causing conflicts in this branch.

All my changes get deleted, and only some things stay. Is there an easier way I can fix this? I followed the rustc-dev-guide.

@rustbot ready

RalfJung · 2025-05-24T21:06:40Z

You'll have to resolve those conflicts before I will do another review -- I'd like to see a green CI for that. So I'm afraid there's no way around dealing with them. You can ask for help on Zulip if you're stuck with a rebase.

@rustbot author

RalfJung · 2025-05-24T21:09:02Z

It's probably easier to first squash this into a single big commit, and then rebase that -- otherwise you'll likely see repeated conflicts while rebasing the many commits in your branch.

LorrensP-2158466 · 2025-05-24T21:45:55Z

Yes that's understandable. Thanks for the help!

LorrensP-2158466 · 2025-05-25T09:50:25Z

Alright, squashing and then rebasing helped, thanks! But it's all still local because updating my branch pulled in a bunch of tests, which test behaviour we have not yet implemented. The test now is this one:

assert_eq!(1.0, 0.0f64.exp());

These operations do not yet use the fixed_float_val of the clamp_float_val function. We first wanted to implement the basic ones and in a follow-up pr the rest. So, should I disable these failing tests and add a fixme in the relevant part of the code?

RalfJung · 2025-05-25T10:50:37Z

These operations do not yet use the fixed_float_val of the clamp_float_val function.

They should also not have a random error then. Then the test should still work right?

change tests in std.

LorrensP-2158466 · 2025-05-25T11:23:06Z

Alright, my previous comment was wrong... I have learned a valuable lesson to never have my branch out of date for so long. This was all a bit confusing hahahaha :).

My comment about the current status/context still holds, but I had to change some things.

the tests for f32::asin and f64::asin are disabled because I couldn't make them pass (assert is commented out). for f32 12.0 * f32::EPSILON didn't work for example.
had to add 2 extra APPROX_DELTA in the f32 tests
changed this test: assert_eq(1.0.powi(1), 1.0) to assert_approx_eq, because c23 doesn't specify this case.

LorrensP-2158466 · 2025-05-25T12:37:29Z

@rustbot ready

RalfJung · 2025-05-25T14:00:13Z

Are you talking about doctests, library tests, or miri tests here?

LorrensP-2158466 · 2025-05-25T14:32:26Z

The asin ones are doctests, the other ones are library tests.

RalfJung · 2025-05-27T13:28:41Z

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

@@ -1008,17 +1033,82 @@ pub fn libm() {
    assert_approx_eq!(25f32.powf(-2f32), 0.0016f32);
    assert_approx_eq!(400f64.powf(0.5f64), 20f64);

+    // Some inputs to powf and powi result in fixed outputs
+    // and thus must be exactly equal to that value


Suggested change

// and thus must be exactly equal to that value

// and thus must be exactly equal to that value.

RalfJung · 2025-05-27T13:30:19Z

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

+    assert_eq!((-1f64).powf(f64::NEG_INFINITY), 1.0);
+
+    // For pow (powf in rust) the C standard says:
+    // x^0 = 1 for all x even a sNaN


Suggested change

// x^0 = 1 for all x even a sNaN

// x^0 = 1 for all x even a sNaN

// FIXME(#4286): this does not match the behavior of all implementations.

RalfJung · 2025-05-27T13:30:24Z

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

+    assert_eq!(SNAN_F64.powf(0.0), 1.0);
+
+    // For pown (powi in rust) the C standard says:
+    // x^0 = 1 for all x even a sNaN


Suggested change

// x^0 = 1 for all x even a sNaN

// x^0 = 1 for all x even a sNaN

// FIXME(#4286): this does not match the behavior of all implementations.

RalfJung · 2025-05-27T13:31:43Z

library/std/src/f32.rs

@@ -748,7 +748,7 @@ impl f32 {
    /// // asin(sin(pi/2))
    /// let abs_difference = (f.sin().asin() - std::f32::consts::FRAC_PI_2).abs();
    ///
-    /// assert!(abs_difference <= f32::EPSILON);
+    /// // assert!(abs_difference <= 12.0 * f32::EPSILON);


We can't have commented-out doctests. What exactly is happening here?

I've run this test with many seeds and printed abs_difference / EPSILON each time. I got results between 0 and 8192. As usual, FP arithmetic precision degrades a lot when one subtracts two numbers that are fairly close to each other.

@tgross35 what do you suggest we do in this case? There's not really a reason to promise that f.sin().asin() - std::f32::consts::FRAC_PI_2 will be only a few epsilon away from 0. We could make this assert!(abs_difference < 1e-3); which passes on 10k different random seeds (for the RNG used to apply the error). That's quite far from EPSILON, but, well EPSILON is just really small...

I don't think epsilon is really that meaningful as an error reference point, using 1e-3 seems better to me here anyway. asin is quite sensitive around 1.0 so the error compounding is pretty understandable.

Should some of the other tests that are changed in this PR also use fixed constants like 1e-4 or so, or should we stick to EPSILON there?

Maybe 1e-4 is clearer than the operation on EPSILON?

Yeah that's the point I was trying to make :)

I agree with your point :)

I'll change all of those to the 1e notation

RalfJung · 2025-05-27T13:31:59Z

library/std/src/f64.rs

@@ -748,7 +748,7 @@ impl f64 {
    /// // asin(sin(pi/2))
    /// let abs_difference = (f.sin().asin() - std::f64::consts::FRAC_PI_2).abs();
    ///
-    /// assert!(abs_difference < 1e-10);
+    /// // assert!(abs_difference < 1e-8);


Same here regarding commented-out tests.

RalfJung · 2025-05-27T13:32:58Z

library/std/tests/floats/f32.rs

+    assert_approx_eq!(
+        3.4f32.powf(4.5),
+        246.408218,
+        APPROX_DELTA /* Miri float-non-det: Make tests pass for now */


Why does every use of this variable have that comment? The variable itself already explains everything there is to say, right?

You're right, I did it to make it clearer why the constant is used in those locations. I'll remove them!

RalfJung · 2025-05-27T13:34:27Z

@rustbot author

rustbot assigned tgross35 Mar 5, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. 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

RalfJung reviewed Mar 9, 2025

View reviewed changes

RalfJung reviewed Mar 19, 2025

View reviewed changes

rustbot assigned tgross35 and unassigned RalfJung Mar 30, 2025

RalfJung mentioned this pull request Mar 31, 2025

Expose algebraic floating point intrinsics #136457

Merged

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2025

LorrensP-2158466 force-pushed the miri-enable-float-nondet branch from 61df399 to 557f172 Compare May 25, 2025 09:13

LorrensP-2158466 added 3 commits May 25, 2025 13:15

Enable Float non-determinism in miri. Update and add tests and change

f7579a1

change tests in std.

some cleanup

b1b9abf

make the tests pass for now

bddfc69

LorrensP-2158466 force-pushed the miri-enable-float-nondet branch from 557f172 to bddfc69 Compare May 25, 2025 11:16

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2025

RalfJung reviewed May 27, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2025

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

	// and thus must be exactly equal to that value
	// and thus must be exactly equal to that value.

	// x^0 = 1 for all x even a sNaN
	// x^0 = 1 for all x even a sNaN
	// FIXME(#4286): this does not match the behavior of all implementations.

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 • edited by RalfJung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 5, 2025

Uh oh!

rustbot commented Mar 5, 2025

Uh oh!

tgross35 commented Mar 5, 2025

Uh oh!

Uh oh!

LorrensP-2158466 commented Mar 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RalfJung commented Mar 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LorrensP-2158466 commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Mar 10, 2025

Uh oh!

LorrensP-2158466 commented Mar 10, 2025

Uh oh!

RalfJung commented Mar 10, 2025 via email

Uh oh!

LorrensP-2158466 commented Mar 10, 2025

Uh oh!

LorrensP-2158466 commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

LorrensP-2158466 commented Mar 5, 2025 •

edited by RalfJung

Loading

LorrensP-2158466 commented Mar 10, 2025 •

edited

Loading

RalfJung commented Mar 19, 2025 •

edited

Loading

LorrensP-2158466 commented May 24, 2025 •

edited

Loading