Skip to content
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

Apply random float error #4156

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

Conversation

LorrensP-2158466
Copy link

@LorrensP-2158466 LorrensP-2158466 commented Jan 27, 2025

This partially tries to fix #3555 (does not include the pow operations)

this just does apply_random_float_error on the results of the intrinsics.
Things changed from original:

  • seperated the sqrtf* intrinsics to make the implementation cleaner, since applying the error was not required
  • changes the way we do approximation checks, instead of using the epsilon technique, we use ULP.
  • create the relative errors of 16ULP using ulp_err_scale, which does -(F::PRECISION - 1 - n for a 2^n ULP error.
  • adds a test test_non_determinism which tests a lot of non-deterministic operations with the same input to check if they are almost but not entirely equal.

@LorrensP-2158466 LorrensP-2158466 marked this pull request as draft January 27, 2025 19:51
@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jan 29, 2025
@RalfJung
Copy link
Member

RalfJung commented Jan 29, 2025

Please also add a test (as a new function in tests/pass/float.rs) that checks for non-determinism by running the same operation multiple times and ensuring we get different (but similar) results.

@@ -202,14 +212,33 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
_ => bug!(),
};
let res = f.round_to_integral(mode).value;
// Apply a relative error with a magnitude on the order of 2^-12 to simulate
// non-deterministic behaviour of floats
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rounding is fully precise, it should not get any error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I caught that and fixed it locally

"sqrtf32" => {
let [f] = check_arg_count(args)?;
let f = this.read_scalar(f)?.to_f32()?;
// no host floats for sqrt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// no host floats for sqrt
// Sqrt is specified to be fully precise.

(same below for sqrtf64)

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@LorrensP-2158466
Copy link
Author

Almost done I think.

I have 1 TODO left, inside of the non-determinism test, it is about this:

Please also add a test (as a new function in tests/pass/float.rs) that checks for non-determinism by running the same operation multiple times and ensuring we get different (but similar) results.

Should we do an assert_ne! test on the results, because it is possible that the same error is applied to 2 separate operations that we then check. Maybe it is possible to count these exactly equal results and if it is the same as the amount of checks we can safely say it is deterministic and should fail the test?.

@LorrensP-2158466 LorrensP-2158466 force-pushed the apply-random-float-error branch from a64db7e to b82c735 Compare February 5, 2025 18:21
@LorrensP-2158466
Copy link
Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Feb 5, 2025
@RalfJung
Copy link
Member

RalfJung commented Feb 5, 2025

Should we do an assert_ne! test on the results, because it is possible that the same error is applied to 2 separate operations that we then check. Maybe it is possible to count these exactly equal results and if it is the same as the amount of checks we can safely say it is deterministic and should fail the test?.

I think the easiest way is something like this; you can do that with a closure returning any PartialEq type instead of bool.

@LorrensP-2158466
Copy link
Author

@rustbot ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use non-deterministic precision for float operations that do not have guaranteed precision
3 participants