-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[MIR] Fix double-rounding of float constants and ignore NaN sign in tests. #34006
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
(FInfer { f64: a, .. }, FInfer { f64: b, .. }) => { | ||
unsafe{transmute::<_,u64>(a) == transmute::<_,u64>(b)} | ||
} | ||
(F32(a), F32(b)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs the FInfer
+ F32
branches for f32
inference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks for noticing!
// Ignore the "sign" (quiet / signalling flag) of NAN. | ||
// It can vary between runtime operations and LLVM folding. | ||
let (nan_m, nan_e, _nan_s) = NAN.integer_decode(); | ||
assert_eq!((nan_m, nan_e), (12582912, 105)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can’t believe this test worked for so long on all of the platforms we support, because mantissa of NaN is not specified to be anything in particular. The only requirement is that the MSB of the mantissa is set to 1 to differentiate from infinity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nagisa Well, LLVM was always doing the folding for us, and it always produces the same result.
@bors r+ |
📌 Commit 29b1af5 has been approved by |
🔒 Merge conflict |
☔ The latest upstream changes (presumably #33905) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors r=nikomatsakis |
📌 Commit c77166c has been approved by |
⌛ Testing commit c77166c with merge 6e577ca... |
💔 Test failed - auto-win-msvc-64-opt |
@bors r=nikomatsakis |
📌 Commit f158a2f has been approved by |
⌛ Testing commit f158a2f with merge e19ccb5... |
⛄ The build was interrupted to prioritize another pull request. |
⌛ Testing commit f158a2f with merge 323e9ca... |
💔 Test failed - auto-mac-64-opt-rustbuild |
@alexcrichton ^^ Spurious? |
[MIR] Fix double-rounding of float constants and ignore NaN sign in tests. Fixes #32805 by handling f32 and f64 separately in rustc_const_eval. Also removes `#[rustc_no_mir]` from a couple libstd tests by ignoring NaN sign. Turns out that runtime evaluation of `0.0 / 0.0` produces a NaN with the sign bit set, whereas LLVM constant folds it to a NaN with the sign bit unset, which we were testing for.
Fixes #32805 by handling f32 and f64 separately in rustc_const_eval.
Also removes
#[rustc_no_mir]
from a couple libstd tests by ignoring NaN sign.Turns out that runtime evaluation of
0.0 / 0.0
produces a NaN with the sign bit set,whereas LLVM constant folds it to a NaN with the sign bit unset, which we were testing for.