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

checked_ilog2 test fails with imprecise float intrinsics #137591

Open
RalfJung opened this issue Feb 25, 2025 · 4 comments
Open

checked_ilog2 test fails with imprecise float intrinsics #137591

RalfJung opened this issue Feb 25, 2025 · 4 comments
Labels
A-floating-point Area: Floating point numbers and arithmetic A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Miri made its floatig-point intrinsics imprecise to reflect that we don't guarantee an exact result, and now we are seeing test failures when bumping the Miri subtree:

2025-02-24T19:16:35.0690558Z thread 'num::int_log::checked_ilog2' panicked at library/coretests/tests/num/int_log.rs:50:5:
2025-02-24T19:16:35.0691376Z assertion `left == right` failed
2025-02-24T19:16:35.0691830Z   left: Some(13)
2025-02-24T19:16:35.0692204Z  right: Some(12)
2025-02-24T19:16:35.0692798Z 
2025-02-24T19:16:35.0692807Z 
2025-02-24T19:16:35.0692960Z failures:
2025-02-24T19:16:35.0693335Z     num::int_log::checked_ilog2

The failure occurs here:

assert_eq!(8192i16.checked_ilog2(), Some((8192f32).log2() as u32));

log2 here is the float operation. We even document:

The precision of this function is non-deterministic. This means it varies by platform, Rust version, and can even differ within the same execution from one invocation to the next.

So I think the test is bogus?

Cc @saethlin @tgross35

Note that this is somewhat urgent as it blocks updating Miri's subtree.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 25, 2025
@RalfJung
Copy link
Member Author

as casts round towards zero, right? So probably the easiest fix is to add in a round to ensure a small imprecision returning 12.99999 does not lead to a result of 12.

@RalfJung
Copy link
Member Author

Ah, but the very next line also checks this for non-powers-of-2 where we can't round exactly, we have to round down, but we also have to account for floating-point error:

for i in 1..=u8::MAX {
assert_eq!(i.checked_ilog2(), Some((i as f32).log2() as u32), "checking {i}");
}

So probably we should do some sort of assert_approx_eq here, rather than comparing integers.

I'll probably just disable the test in Miri for now to proceed with the subtree update.

@RalfJung
Copy link
Member Author

Lol, the tests seem to have caused similar issues in the past already:

// Guard against Android's imprecise f32::ilog2 implementation.
if i != 8192 {
assert_eq!(i.checked_ilog2(), Some((i as f32).log2() as u32), "checking {i}");
}

@tgross35
Copy link
Contributor

I guess gating them is probably the most simple option for now. The tests could probably be reworked to instead check y = x.ilog2 => 2.pow(y) = x, but the float version is likely to be reasonably reliable in practice (and easy to identify if not) and having a different kind of basis is good.

@jieyouxu jieyouxu added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-floating-point Area: Floating point numbers and arithmetic A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants