Skip to content

Commit 39b7669

Browse files
borsbors
committed
Auto merge of rust-lang#130220 - RalfJung:float-classify, r=workingjubilee
simplify float::classify logic I played around with the float-classify test in the hope of triggering x87 bugs by strategically adding `black_box`, and still the exact expression `@beetrees` suggested [here](rust-lang#129835 (comment)) remains the only case I found where we get the wrong result on x87. Curiously, this bug only occurs when MIR optimizations are enabled -- probably the extra inlining that does is required for LLVM to hit the right "bad" case in the backend. But even for that case, it makes no difference whether `classify` is implemented in the simple bit-pattern-based version or the more complicated version we had before. Without even a single testcase that can distinguish our `classify` from the naive version, I suggest we switch to the naive version.
2 parents c16ff44 + b72b42d commit 39b7669

File tree

7 files changed

+106
-137
lines changed

7 files changed

+106
-137
lines changed

Diff for: ‎library/core/src/num/f128.rs

-4
Original file line numberDiff line numberDiff line change
@@ -439,10 +439,6 @@ impl f128 {
439439
#[unstable(feature = "f128", issue = "116909")]
440440
#[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
441441
pub const fn classify(self) -> FpCategory {
442-
// Other float types suffer from various platform bugs that violate the usual IEEE semantics
443-
// and also make bitwise classification not always work reliably. However, `f128` cannot fit
444-
// into any other float types so this is not a concern, and we can rely on bit patterns.
445-
446442
let bits = self.to_bits();
447443
match (bits & Self::MAN_MASK, bits & Self::EXP_MASK) {
448444
(0, Self::EXP_MASK) => FpCategory::Infinite,

Diff for: ‎library/core/src/num/f16.rs

+7-37
Original file line numberDiff line numberDiff line change
@@ -424,43 +424,13 @@ impl f16 {
424424
#[unstable(feature = "f16", issue = "116909")]
425425
#[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
426426
pub const fn classify(self) -> FpCategory {
427-
// A previous implementation for f32/f64 tried to only use bitmask-based checks,
428-
// using `to_bits` to transmute the float to its bit repr and match on that.
429-
// If we only cared about being "technically" correct, that's an entirely legit
430-
// implementation.
431-
//
432-
// Unfortunately, there are platforms out there that do not correctly implement the IEEE
433-
// float semantics Rust relies on: some hardware flushes denormals to zero, and some
434-
// platforms convert to `f32` to perform operations without properly rounding back (e.g.
435-
// WASM, see llvm/llvm-project#96437). These are platforms bugs, and Rust will misbehave on
436-
// such platforms, but we can at least try to make things seem as sane as possible by being
437-
// careful here.
438-
// see also https://github.com/rust-lang/rust/issues/114479
439-
if self.is_infinite() {
440-
// Thus, a value may compare unequal to infinity, despite having a "full" exponent mask.
441-
FpCategory::Infinite
442-
} else if self.is_nan() {
443-
// And it may not be NaN, as it can simply be an "overextended" finite value.
444-
FpCategory::Nan
445-
} else {
446-
// However, std can't simply compare to zero to check for zero, either,
447-
// as correctness requires avoiding equality tests that may be Subnormal == -0.0
448-
// because it may be wrong under "denormals are zero" and "flush to zero" modes.
449-
// Most of std's targets don't use those, but they are used for thumbv7neon.
450-
// So, this does use bitpattern matching for the rest. On x87, due to the incorrect
451-
// float codegen on this hardware, this doesn't actually return a right answer for NaN
452-
// because it cannot correctly discern between a floating point NaN, and some normal
453-
// floating point numbers truncated from an x87 FPU -- but we took care of NaN above, so
454-
// we are fine.
455-
// FIXME(jubilee): This probably could at least answer things correctly for Infinity,
456-
// like the f64 version does, but I need to run more checks on how things go on x86.
457-
// I fear losing mantissa data that would have answered that differently.
458-
let b = self.to_bits();
459-
match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
460-
(0, 0) => FpCategory::Zero,
461-
(_, 0) => FpCategory::Subnormal,
462-
_ => FpCategory::Normal,
463-
}
427+
let b = self.to_bits();
428+
match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
429+
(0, Self::EXP_MASK) => FpCategory::Infinite,
430+
(_, Self::EXP_MASK) => FpCategory::Nan,
431+
(0, 0) => FpCategory::Zero,
432+
(_, 0) => FpCategory::Subnormal,
433+
_ => FpCategory::Normal,
464434
}
465435
}
466436

Diff for: ‎library/core/src/num/f32.rs

+12-36
Original file line numberDiff line numberDiff line change
@@ -652,42 +652,18 @@ impl f32 {
652652
#[stable(feature = "rust1", since = "1.0.0")]
653653
#[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
654654
pub const fn classify(self) -> FpCategory {
655-
// A previous implementation tried to only use bitmask-based checks,
656-
// using f32::to_bits to transmute the float to its bit repr and match on that.
657-
// If we only cared about being "technically" correct, that's an entirely legit
658-
// implementation.
659-
//
660-
// Unfortunately, there is hardware out there that does not correctly implement the IEEE
661-
// float semantics Rust relies on: x87 uses a too-large mantissa and exponent, and some
662-
// hardware flushes subnormals to zero. These are platforms bugs, and Rust will misbehave on
663-
// such hardware, but we can at least try to make things seem as sane as possible by being
664-
// careful here.
665-
// see also https://github.com/rust-lang/rust/issues/114479
666-
if self.is_infinite() {
667-
// A value may compare unequal to infinity, despite having a "full" exponent mask.
668-
FpCategory::Infinite
669-
} else if self.is_nan() {
670-
// And it may not be NaN, as it can simply be an "overextended" finite value.
671-
FpCategory::Nan
672-
} else {
673-
// However, std can't simply compare to zero to check for zero, either,
674-
// as correctness requires avoiding equality tests that may be Subnormal == -0.0
675-
// because it may be wrong under "denormals are zero" and "flush to zero" modes.
676-
// Most of std's targets don't use those, but they are used for thumbv7neon.
677-
// So, this does use bitpattern matching for the rest. On x87, due to the incorrect
678-
// float codegen on this hardware, this doesn't actually return a right answer for NaN
679-
// because it cannot correctly discern between a floating point NaN, and some normal
680-
// floating point numbers truncated from an x87 FPU -- but we took care of NaN above, so
681-
// we are fine.
682-
// FIXME(jubilee): This probably could at least answer things correctly for Infinity,
683-
// like the f64 version does, but I need to run more checks on how things go on x86.
684-
// I fear losing mantissa data that would have answered that differently.
685-
let b = self.to_bits();
686-
match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
687-
(0, 0) => FpCategory::Zero,
688-
(_, 0) => FpCategory::Subnormal,
689-
_ => FpCategory::Normal,
690-
}
655+
// We used to have complicated logic here that avoids the simple bit-based tests to work
656+
// around buggy codegen for x87 targets (see
657+
// https://github.com/rust-lang/rust/issues/114479). However, some LLVM versions later, none
658+
// of our tests is able to find any difference between the complicated and the naive
659+
// version, so now we are back to the naive version.
660+
let b = self.to_bits();
661+
match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
662+
(0, Self::EXP_MASK) => FpCategory::Infinite,
663+
(_, Self::EXP_MASK) => FpCategory::Nan,
664+
(0, 0) => FpCategory::Zero,
665+
(_, 0) => FpCategory::Subnormal,
666+
_ => FpCategory::Normal,
691667
}
692668
}
693669

Diff for: ‎library/core/src/num/f64.rs

+12-32
Original file line numberDiff line numberDiff line change
@@ -651,38 +651,18 @@ impl f64 {
651651
#[stable(feature = "rust1", since = "1.0.0")]
652652
#[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
653653
pub const fn classify(self) -> FpCategory {
654-
// A previous implementation tried to only use bitmask-based checks,
655-
// using f64::to_bits to transmute the float to its bit repr and match on that.
656-
// If we only cared about being "technically" correct, that's an entirely legit
657-
// implementation.
658-
//
659-
// Unfortunately, there is hardware out there that does not correctly implement the IEEE
660-
// float semantics Rust relies on: x87 uses a too-large exponent, and some hardware flushes
661-
// subnormals to zero. These are platforms bugs, and Rust will misbehave on such hardware,
662-
// but we can at least try to make things seem as sane as possible by being careful here.
663-
// see also https://github.com/rust-lang/rust/issues/114479
664-
//
665-
// Thus, a value may compare unequal to infinity, despite having a "full" exponent mask.
666-
// And it may not be NaN, as it can simply be an "overextended" finite value.
667-
if self.is_nan() {
668-
FpCategory::Nan
669-
} else {
670-
// However, std can't simply compare to zero to check for zero, either,
671-
// as correctness requires avoiding equality tests that may be Subnormal == -0.0
672-
// because it may be wrong under "denormals are zero" and "flush to zero" modes.
673-
// Most of std's targets don't use those, but they are used for thumbv7neon.
674-
// So, this does use bitpattern matching for the rest. On x87, due to the incorrect
675-
// float codegen on this hardware, this doesn't actually return a right answer for NaN
676-
// because it cannot correctly discern between a floating point NaN, and some normal
677-
// floating point numbers truncated from an x87 FPU -- but we took care of NaN above, so
678-
// we are fine.
679-
let b = self.to_bits();
680-
match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
681-
(0, Self::EXP_MASK) => FpCategory::Infinite,
682-
(0, 0) => FpCategory::Zero,
683-
(_, 0) => FpCategory::Subnormal,
684-
_ => FpCategory::Normal,
685-
}
654+
// We used to have complicated logic here that avoids the simple bit-based tests to work
655+
// around buggy codegen for x87 targets (see
656+
// https://github.com/rust-lang/rust/issues/114479). However, some LLVM versions later, none
657+
// of our tests is able to find any difference between the complicated and the naive
658+
// version, so now we are back to the naive version.
659+
let b = self.to_bits();
660+
match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
661+
(0, Self::EXP_MASK) => FpCategory::Infinite,
662+
(_, Self::EXP_MASK) => FpCategory::Nan,
663+
(0, 0) => FpCategory::Zero,
664+
(_, 0) => FpCategory::Subnormal,
665+
_ => FpCategory::Normal,
686666
}
687667
}
688668

Diff for: ‎src/tools/tidy/src/issues.txt

-1
Original file line numberDiff line numberDiff line change
@@ -3180,7 +3180,6 @@ ui/nll/user-annotations/issue-55219.rs
31803180
ui/nll/user-annotations/issue-55241.rs
31813181
ui/nll/user-annotations/issue-55748-pat-types-constrain-bindings.rs
31823182
ui/nll/user-annotations/issue-57731-ascibed-coupled-types.rs
3183-
ui/numbers-arithmetic/issue-105626.rs
31843183
ui/numbers-arithmetic/issue-8460.rs
31853184
ui/object-safety/issue-102762.rs
31863185
ui/object-safety/issue-102933.rs

Diff for: ‎tests/ui/float/classify-runtime-const.rs

+75-27
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
//@ compile-flags: -Zmir-opt-level=0 -Znext-solver
21
//@ run-pass
2+
//@ revisions: opt noopt ctfe
3+
//@[opt] compile-flags: -O
4+
//@[noopt] compile-flags: -Zmir-opt-level=0
35
// ignore-tidy-linelength
46

57
// This tests the float classification functions, for regular runtime code and for const evaluation.
@@ -8,71 +10,117 @@
810
#![feature(f128_const)]
911
#![feature(const_float_classify)]
1012

11-
use std::hint::black_box;
1213
use std::num::FpCategory::*;
1314

14-
macro_rules! both_assert {
15+
#[cfg(not(ctfe))]
16+
use std::hint::black_box;
17+
#[cfg(ctfe)]
18+
#[allow(unused)]
19+
const fn black_box<T>(x: T) -> T { x }
20+
21+
#[cfg(not(ctfe))]
22+
macro_rules! assert_test {
23+
($a:expr, NonDet) => {
24+
{
25+
// Compute `a`, but do not compare with anything as the result is non-deterministic.
26+
let _val = $a;
27+
}
28+
};
29+
($a:expr, $b:ident) => {
30+
{
31+
// Let-bind to avoid promotion.
32+
// No black_box here! That can mask x87 failures.
33+
let a = $a;
34+
let b = $b;
35+
assert_eq!(a, b, "{} produces wrong result", stringify!($a));
36+
}
37+
};
38+
}
39+
#[cfg(ctfe)]
40+
macro_rules! assert_test {
1541
($a:expr, NonDet) => {
1642
{
1743
// Compute `a`, but do not compare with anything as the result is non-deterministic.
1844
const _: () = { let _val = $a; };
19-
// `black_box` prevents promotion, and MIR opts are disabled above, so this is truly
20-
// going through LLVM.
21-
let _val = black_box($a);
2245
}
2346
};
2447
($a:expr, $b:ident) => {
2548
{
2649
const _: () = assert!(matches!($a, $b));
27-
assert!(black_box($a) == black_box($b));
2850
}
2951
};
3052
}
3153

3254
macro_rules! suite {
33-
( $tyname:ident: $( $tt:tt )* ) => {
55+
( $tyname:ident => $( $tt:tt )* ) => {
3456
fn f32() {
57+
#[allow(unused)]
3558
type $tyname = f32;
36-
suite_inner!(f32 $($tt)*);
59+
suite_inner!(f32 => $($tt)*);
3760
}
3861

3962
fn f64() {
63+
#[allow(unused)]
4064
type $tyname = f64;
41-
suite_inner!(f64 $($tt)*);
65+
suite_inner!(f64 => $($tt)*);
4266
}
4367
}
4468
}
4569

4670
macro_rules! suite_inner {
4771
(
48-
$ty:ident [$( $fn:ident ),*]
49-
$val:expr => [$($out:ident),*]
72+
$ty:ident => [$( $fn:ident ),*]:
73+
$(@cfg: $attr:meta)?
74+
$val:expr => [$($out:ident),*],
5075

5176
$( $tail:tt )*
5277
) => {
53-
$( both_assert!($ty::$fn($val), $out); )*
54-
suite_inner!($ty [$($fn),*] $($tail)*)
78+
$(#[cfg($attr)])?
79+
{
80+
// No black_box here! That can mask x87 failures.
81+
$( assert_test!($ty::$fn($val), $out); )*
82+
}
83+
suite_inner!($ty => [$($fn),*]: $($tail)*)
5584
};
5685

57-
( $ty:ident [$( $fn:ident ),*]) => {};
86+
( $ty:ident => [$( $fn:ident ),*]:) => {};
5887
}
5988

6089
// The result of the `is_sign` methods are not checked for correctness, since we do not
6190
// guarantee anything about the signedness of NaNs. See
6291
// https://rust-lang.github.io/rfcs/3514-float-semantics.html.
6392

64-
suite! { T: // type alias for the type we are testing
65-
[ classify, is_nan, is_infinite, is_finite, is_normal, is_sign_positive, is_sign_negative]
66-
-0.0 / 0.0 => [ Nan, true, false, false, false, NonDet, NonDet]
67-
0.0 / 0.0 => [ Nan, true, false, false, false, NonDet, NonDet]
68-
1.0 => [ Normal, false, false, true, true, true, false]
69-
-1.0 => [ Normal, false, false, true, true, false, true]
70-
0.0 => [ Zero, false, false, true, false, true, false]
71-
-0.0 => [ Zero, false, false, true, false, false, true]
72-
1.0 / 0.0 => [ Infinite, false, true, false, false, true, false]
73-
-1.0 / 0.0 => [ Infinite, false, true, false, false, false, true]
74-
1.0 / T::MAX => [Subnormal, false, false, true, false, true, false]
75-
-1.0 / T::MAX => [Subnormal, false, false, true, false, false, true]
93+
suite! { T => // type alias for the type we are testing
94+
[ classify, is_nan, is_infinite, is_finite, is_normal, is_sign_positive, is_sign_negative]:
95+
black_box(0.0) / black_box(0.0) =>
96+
[ Nan, true, false, false, false, NonDet, NonDet],
97+
black_box(0.0) / black_box(-0.0) =>
98+
[ Nan, true, false, false, false, NonDet, NonDet],
99+
black_box(0.0) * black_box(T::INFINITY) =>
100+
[ Nan, true, false, false, false, NonDet, NonDet],
101+
black_box(0.0) * black_box(T::NEG_INFINITY) =>
102+
[ Nan, true, false, false, false, NonDet, NonDet],
103+
1.0 => [ Normal, false, false, true, true, true, false],
104+
-1.0 => [ Normal, false, false, true, true, false, true],
105+
0.0 => [ Zero, false, false, true, false, true, false],
106+
-0.0 => [ Zero, false, false, true, false, false, true],
107+
1.0 / black_box(0.0) =>
108+
[ Infinite, false, true, false, false, true, false],
109+
-1.0 / black_box(0.0) =>
110+
[ Infinite, false, true, false, false, false, true],
111+
2.0 * black_box(T::MAX) =>
112+
[ Infinite, false, true, false, false, true, false],
113+
-2.0 * black_box(T::MAX) =>
114+
[ Infinite, false, true, false, false, false, true],
115+
1.0 / black_box(T::MAX) =>
116+
[Subnormal, false, false, true, false, true, false],
117+
-1.0 / black_box(T::MAX) =>
118+
[Subnormal, false, false, true, false, false, true],
119+
// This specific expression causes trouble on x87 due to
120+
// <https://github.com/rust-lang/rust/issues/114479>.
121+
@cfg: not(all(target_arch = "x86", not(target_feature = "sse2")))
122+
{ let x = black_box(T::MAX); x * x } =>
123+
[ Infinite, false, true, false, false, true, false],
76124
}
77125

78126
fn main() {

Diff for: ‎tests/ui/numbers-arithmetic/issue-105626.rs renamed to ‎tests/ui/float/int-to-float-miscompile-issue-105626.rs

File renamed without changes.

0 commit comments

Comments
 (0)