Enable Non-determinism of float operations in Miri and change std tests #138062
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
The Miri subtree was changed cc @rust-lang/miri |
LorrensP-2158466
changed the title
|
The library changes lgtm, for the rest r? @RalfJung |
Dante-Broggi
reviewed
Mar 6, 2025
I should have clarified: the doc tests fail when running |
RalfJung
reviewed
Mar 9, 2025
src/tools/miri/src/intrinsics/mod.rs
Outdated
| 2, // log2(4) | ||
| ); | ||
|
|
||
| // Clamp values to the output range defined in IEEE 754 9.1. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Outdated
| 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()), |
There was a problem hiding this comment.
Can you avoid using soft floats here?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Argh sorry, I meant "avoid using hard floats" :)
src/tools/miri/src/intrinsics/mod.rs
Outdated
Comment on lines
416
to
419
| 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()), |
There was a problem hiding this comment.
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/tests/pass/float.rs
Outdated
| // 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); |
There was a problem hiding this comment.
Do we really still need 52 ULP? I would have hoped that 32 works now.
There was a problem hiding this comment.
Indeed 32 works, don't know why I didn't tried it.
There was a problem hiding this comment.
I tried lower but got some ULP differences of 27.
src/tools/miri/tests/pass/float.rs
Outdated
Comment on lines
1011
to
1012
| // TODO: How to test NaN inputs? f*::NAN is not guaranteed | ||
| // to be any specific bit pattern (in std). |
There was a problem hiding this comment.
It is guaranteed to be a NaN though. Why do you want a specific bit pattern?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
it is failing the tests that use |
RalfJung
reviewed
Mar 9, 2025
library/std/tests/floats/lib.rs
Outdated
| 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) }}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
So I:
|
|
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 |
|
Yeah Sorry, I was confused with your comment: |
|
I was confused with your comment: avoid using soft floats.
sorry for that :(
|
|
No problems! This float stuff it getting to me too :) |
|
So I extended the There is still some repetitiveness, like applying the error or |
RalfJung
reviewed
Mar 19, 2025
src/tools/miri/src/intrinsics/mod.rs
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
No please only but things in helpers that are widely useful across Miri. Specific functionality should remain in its specific file.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah yeah... we'll figure that out when we get there.
src/tools/miri/src/intrinsics/mod.rs
Outdated
| // 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 |
There was a problem hiding this comment.
Yeah, now that I am reading that again... I'll update it, excuse me for my bad English :)
src/tools/miri/src/intrinsics/mod.rs
Outdated
Comment on lines
590
to
592
| // 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 |
There was a problem hiding this comment.
I don't understand the comment. The code looks nice though :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
You can't use consts that depend on generics in patterns like that, that is expected.
src/tools/miri/tests/pass/float.rs
Outdated
Comment on lines
1011
to
1012
| // TODO: How to test NaN inputs? f*::NAN is not guaranteed | ||
| // to be any specific bit pattern (in std). |
There was a problem hiding this comment.
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.
|
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:
I am not convinced we want to do that, so unless @tgross35 asks for it I'd say please stick to the current approach. |
|
@tgross35 could you have a look at the library diff again? // 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 }; |
Yes, they indeed pass now. On the topic of doc and doctests, for example, // 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? |
|
r? @tgross35 to reflect the current status. Feel free to re-assign to me if the libs changes are fine. |
|
Let's avoid a conflict with the other in-flight PR, #136457. So let's deal with the algebraic and fast-math operations separately. |
bors
added
the
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jun 9, 2025
…RalfJung Enable Non-determinism of float operations in Miri and change std tests Links to [#4208](rust-lang/miri#4208) and [#3555](rust-lang/miri#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 some of them. This pr includes the following changes: - Enables the float non-determinism but with a lower relative error of 4ULP instead of 16ULP - These operations now have a fixed output based on the C23 standard, except the pow operations, this is tracked in [#4286](rust-lang/miri#4286 (comment)) - Changes tests that made incorrect assumptions about the operations, not to make that assumption anymore (from `assert_eq!` to `assert_approx_eq!`. - Changed the doctests of the stdlib of these operations to compare against fixed constants instead of `f*::EPSILON`, which now succeed with Miri and `-Zmiri-many-seeds` - Added a constant `APPROX_DELTA` in `std/tests/floats/f32.rs` which is used for approximation tests, but with a different value when run in Miri. This is to make these tests succeed. - Added tests in the float tests of Miri to test the C23 behaviour. Fixes rust-lang/miri#4208
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Wut? |
bors
added
S-waiting-on-bors
|
@bors2 try jobs=aarch64-apple |
rust-bors bot
added a commit
that referenced
this pull request
Jun 9, 2025
…<try>
Enable Non-determinism of float operations in Miri and change std tests
<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.
This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using
r? <reviewer name>
-->
<!-- homu-ignore:end -->
Links to [#4208](rust-lang/miri#4208) and [#3555](rust-lang/miri#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 some of them.
This pr includes the following changes:
- Enables the float non-determinism but with a lower relative error of 4ULP instead of 16ULP
- These operations now have a fixed output based on the C23 standard, except the pow operations, this is tracked in [#4286](rust-lang/miri#4286 (comment))
- Changes tests that made incorrect assumptions about the operations, not to make that assumption anymore (from `assert_eq!` to `assert_approx_eq!`.
- Changed the doctests of the stdlib of these operations to compare against fixed constants instead of `f*::EPSILON`, which now succeed with Miri and `-Zmiri-many-seeds`
- Added a constant `APPROX_DELTA` in `std/tests/floats/f32.rs` which is used for approximation tests, but with a different value when run in Miri. This is to make these tests succeed.
- Added tests in the float tests of Miri to test the C23 behaviour.
Fixes rust-lang/miri#4208
try-job: aarch64-apple
|
💔 Test failed
|
|
With #142241 merged @bors2 try jobs=aarch64-apple |
rust-bors bot
added a commit
that referenced
this pull request
Jun 9, 2025
…<try>
Enable Non-determinism of float operations in Miri and change std tests
<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.
This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using
r? <reviewer name>
-->
<!-- homu-ignore:end -->
Links to [#4208](rust-lang/miri#4208) and [#3555](rust-lang/miri#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 some of them.
This pr includes the following changes:
- Enables the float non-determinism but with a lower relative error of 4ULP instead of 16ULP
- These operations now have a fixed output based on the C23 standard, except the pow operations, this is tracked in [#4286](rust-lang/miri#4286 (comment))
- Changes tests that made incorrect assumptions about the operations, not to make that assumption anymore (from `assert_eq!` to `assert_approx_eq!`.
- Changed the doctests of the stdlib of these operations to compare against fixed constants instead of `f*::EPSILON`, which now succeed with Miri and `-Zmiri-many-seeds`
- Added a constant `APPROX_DELTA` in `std/tests/floats/f32.rs` which is used for approximation tests, but with a different value when run in Miri. This is to make these tests succeed.
- Added tests in the float tests of Miri to test the C23 behaviour.
Fixes rust-lang/miri#4208
try-job: aarch64-apple
This was referenced Jun 9, 2025
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing d13a431 (parent) -> c6768de (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard c6768de2d63de7a41124a0fb8fc78f9e26111c01 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (c6768de): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary 2.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 754.509s -> 755.17s (0.09%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 some of them.
This pr includes the following changes:
assert_eq!toassert_approx_eq!.f*::EPSILON, which now succeed with Miri and-Zmiri-many-seedsAPPROX_DELTAinstd/tests/floats/f32.rswhich is used for approximation tests, but with a different value when run in Miri. This is to make these tests succeed.Fixes rust-lang/miri#4208