Auto merge of rust-lang#130220 - RalfJung:float-classify, r=workingjubilee

bors · bors · commit c3b2ce7e79f6 · 2024-09-15T18:27:17.000Z
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.
diff --git a/library/core/src/num/f128.rs b/library/core/src/num/f128.rs
@@ -439,10 +439,6 @@ impl f128 {
     #[unstable(feature = "f128", issue = "116909")]
     #[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
     pub const fn classify(self) -> FpCategory {
-        // Other float types suffer from various platform bugs that violate the usual IEEE semantics
-        // and also make bitwise classification not always work reliably. However, `f128` cannot fit
-        // into any other float types so this is not a concern, and we can rely on bit patterns.
-
         let bits = self.to_bits();
         match (bits & Self::MAN_MASK, bits & Self::EXP_MASK) {
             (0, Self::EXP_MASK) => FpCategory::Infinite,
diff --git a/library/core/src/num/f16.rs b/library/core/src/num/f16.rs
@@ -424,43 +424,13 @@ impl f16 {
     #[unstable(feature = "f16", issue = "116909")]
     #[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
     pub const fn classify(self) -> FpCategory {
-        // A previous implementation for f32/f64 tried to only use bitmask-based checks,
-        // using `to_bits` to transmute the float to its bit repr and match on that.
-        // If we only cared about being "technically" correct, that's an entirely legit
-        // implementation.
-        //
-        // Unfortunately, there are platforms out there that do not correctly implement the IEEE
-        // float semantics Rust relies on: some hardware flushes denormals to zero, and some
-        // platforms convert to `f32` to perform operations without properly rounding back (e.g.
-        // WASM, see llvm/llvm-project#96437). These are platforms bugs, and Rust will misbehave on
-        // such platforms, but we can at least try to make things seem as sane as possible by being
-        // careful here.
-        // see also https://github.com/rust-lang/rust/issues/114479
-        if self.is_infinite() {
-            // Thus, a value may compare unequal to infinity, despite having a "full" exponent mask.
-            FpCategory::Infinite
-        } else if self.is_nan() {
-            // And it may not be NaN, as it can simply be an "overextended" finite value.
-            FpCategory::Nan
-        } else {
-            // However, std can't simply compare to zero to check for zero, either,
-            // as correctness requires avoiding equality tests that may be Subnormal == -0.0
-            // because it may be wrong under "denormals are zero" and "flush to zero" modes.
-            // Most of std's targets don't use those, but they are used for thumbv7neon.
-            // So, this does use bitpattern matching for the rest. On x87, due to the incorrect
-            // float codegen on this hardware, this doesn't actually return a right answer for NaN
-            // because it cannot correctly discern between a floating point NaN, and some normal
-            // floating point numbers truncated from an x87 FPU -- but we took care of NaN above, so
-            // we are fine.
-            // FIXME(jubilee): This probably could at least answer things correctly for Infinity,
-            // like the f64 version does, but I need to run more checks on how things go on x86.
-            // I fear losing mantissa data that would have answered that differently.
-            let b = self.to_bits();
-            match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
-                (0, 0) => FpCategory::Zero,
-                (_, 0) => FpCategory::Subnormal,
-                _ => FpCategory::Normal,
-            }
+        let b = self.to_bits();
+        match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
+            (0, Self::EXP_MASK) => FpCategory::Infinite,
+            (_, Self::EXP_MASK) => FpCategory::Nan,
+            (0, 0) => FpCategory::Zero,
+            (_, 0) => FpCategory::Subnormal,
+            _ => FpCategory::Normal,
         }
     }
 
diff --git a/library/core/src/num/f32.rs b/library/core/src/num/f32.rs
@@ -652,42 +652,18 @@ impl f32 {
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
     pub const fn classify(self) -> FpCategory {
-        // A previous implementation tried to only use bitmask-based checks,
-        // using f32::to_bits to transmute the float to its bit repr and match on that.
-        // If we only cared about being "technically" correct, that's an entirely legit
-        // implementation.
-        //
-        // Unfortunately, there is hardware out there that does not correctly implement the IEEE
-        // float semantics Rust relies on: x87 uses a too-large mantissa and exponent, and some
-        // hardware flushes subnormals to zero. These are platforms bugs, and Rust will misbehave on
-        // such hardware, but we can at least try to make things seem as sane as possible by being
-        // careful here.
-        // see also https://github.com/rust-lang/rust/issues/114479
-        if self.is_infinite() {
-            // A value may compare unequal to infinity, despite having a "full" exponent mask.
-            FpCategory::Infinite
-        } else if self.is_nan() {
-            // And it may not be NaN, as it can simply be an "overextended" finite value.
-            FpCategory::Nan
-        } else {
-            // However, std can't simply compare to zero to check for zero, either,
-            // as correctness requires avoiding equality tests that may be Subnormal == -0.0
-            // because it may be wrong under "denormals are zero" and "flush to zero" modes.
-            // Most of std's targets don't use those, but they are used for thumbv7neon.
-            // So, this does use bitpattern matching for the rest. On x87, due to the incorrect
-            // float codegen on this hardware, this doesn't actually return a right answer for NaN
-            // because it cannot correctly discern between a floating point NaN, and some normal
-            // floating point numbers truncated from an x87 FPU -- but we took care of NaN above, so
-            // we are fine.
-            // FIXME(jubilee): This probably could at least answer things correctly for Infinity,
-            // like the f64 version does, but I need to run more checks on how things go on x86.
-            // I fear losing mantissa data that would have answered that differently.
-            let b = self.to_bits();
-            match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
-                (0, 0) => FpCategory::Zero,
-                (_, 0) => FpCategory::Subnormal,
-                _ => FpCategory::Normal,
-            }
+        // We used to have complicated logic here that avoids the simple bit-based tests to work
+        // around buggy codegen for x87 targets (see
+        // https://github.com/rust-lang/rust/issues/114479). However, some LLVM versions later, none
+        // of our tests is able to find any difference between the complicated and the naive
+        // version, so now we are back to the naive version.
+        let b = self.to_bits();
+        match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
+            (0, Self::EXP_MASK) => FpCategory::Infinite,
+            (_, Self::EXP_MASK) => FpCategory::Nan,
+            (0, 0) => FpCategory::Zero,
+            (_, 0) => FpCategory::Subnormal,
+            _ => FpCategory::Normal,
         }
     }
 
diff --git a/library/core/src/num/f64.rs b/library/core/src/num/f64.rs
@@ -651,38 +651,18 @@ impl f64 {
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_unstable(feature = "const_float_classify", issue = "72505")]
     pub const fn classify(self) -> FpCategory {
-        // A previous implementation tried to only use bitmask-based checks,
-        // using f64::to_bits to transmute the float to its bit repr and match on that.
-        // If we only cared about being "technically" correct, that's an entirely legit
-        // implementation.
-        //
-        // Unfortunately, there is hardware out there that does not correctly implement the IEEE
-        // float semantics Rust relies on: x87 uses a too-large exponent, and some hardware flushes
-        // subnormals to zero. These are platforms bugs, and Rust will misbehave on such hardware,
-        // but we can at least try to make things seem as sane as possible by being careful here.
-        // see also https://github.com/rust-lang/rust/issues/114479
-        //
-        // Thus, a value may compare unequal to infinity, despite having a "full" exponent mask.
-        // And it may not be NaN, as it can simply be an "overextended" finite value.
-        if self.is_nan() {
-            FpCategory::Nan
-        } else {
-            // However, std can't simply compare to zero to check for zero, either,
-            // as correctness requires avoiding equality tests that may be Subnormal == -0.0
-            // because it may be wrong under "denormals are zero" and "flush to zero" modes.
-            // Most of std's targets don't use those, but they are used for thumbv7neon.
-            // So, this does use bitpattern matching for the rest. On x87, due to the incorrect
-            // float codegen on this hardware, this doesn't actually return a right answer for NaN
-            // because it cannot correctly discern between a floating point NaN, and some normal
-            // floating point numbers truncated from an x87 FPU -- but we took care of NaN above, so
-            // we are fine.
-            let b = self.to_bits();
-            match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
-                (0, Self::EXP_MASK) => FpCategory::Infinite,
-                (0, 0) => FpCategory::Zero,
-                (_, 0) => FpCategory::Subnormal,
-                _ => FpCategory::Normal,
-            }
+        // We used to have complicated logic here that avoids the simple bit-based tests to work
+        // around buggy codegen for x87 targets (see
+        // https://github.com/rust-lang/rust/issues/114479). However, some LLVM versions later, none
+        // of our tests is able to find any difference between the complicated and the naive
+        // version, so now we are back to the naive version.
+        let b = self.to_bits();
+        match (b & Self::MAN_MASK, b & Self::EXP_MASK) {
+            (0, Self::EXP_MASK) => FpCategory::Infinite,
+            (_, Self::EXP_MASK) => FpCategory::Nan,
+            (0, 0) => FpCategory::Zero,
+            (_, 0) => FpCategory::Subnormal,
+            _ => FpCategory::Normal,
         }
     }
 
diff --git a/src/tools/tidy/src/issues.txt b/src/tools/tidy/src/issues.txt
@@ -3180,7 +3180,6 @@ ui/nll/user-annotations/issue-55219.rs
 ui/nll/user-annotations/issue-55241.rs
 ui/nll/user-annotations/issue-55748-pat-types-constrain-bindings.rs
 ui/nll/user-annotations/issue-57731-ascibed-coupled-types.rs
-ui/numbers-arithmetic/issue-105626.rs
 ui/numbers-arithmetic/issue-8460.rs
 ui/object-safety/issue-102762.rs
 ui/object-safety/issue-102933.rs
diff --git a/tests/ui/float/classify-runtime-const.rs b/tests/ui/float/classify-runtime-const.rs
@@ -1,5 +1,7 @@
-//@ compile-flags: -Zmir-opt-level=0 -Znext-solver
 //@ run-pass
+//@ revisions: opt noopt ctfe
+//@[opt] compile-flags: -O
+//@[noopt] compile-flags: -Zmir-opt-level=0
 // ignore-tidy-linelength
 
 // This tests the float classification functions, for regular runtime code and for const evaluation.
@@ -8,71 +10,117 @@
 #![feature(f128_const)]
 #![feature(const_float_classify)]
 
-use std::hint::black_box;
 use std::num::FpCategory::*;
 
-macro_rules! both_assert {
+#[cfg(not(ctfe))]
+use std::hint::black_box;
+#[cfg(ctfe)]
+#[allow(unused)]
+const fn black_box<T>(x: T) -> T { x }
+
+#[cfg(not(ctfe))]
+macro_rules! assert_test {
+    ($a:expr, NonDet) => {
+        {
+            // Compute `a`, but do not compare with anything as the result is non-deterministic.
+            let _val = $a;
+        }
+    };
+    ($a:expr, $b:ident) => {
+        {
+            // Let-bind to avoid promotion.
+            // No black_box here! That can mask x87 failures.
+            let a = $a;
+            let b = $b;
+            assert_eq!(a, b, "{} produces wrong result", stringify!($a));
+        }
+    };
+}
+#[cfg(ctfe)]
+macro_rules! assert_test {
     ($a:expr, NonDet) => {
         {
             // Compute `a`, but do not compare with anything as the result is non-deterministic.
             const _: () = { let _val = $a; };
-            // `black_box` prevents promotion, and MIR opts are disabled above, so this is truly
-            // going through LLVM.
-            let _val = black_box($a);
         }
     };
     ($a:expr, $b:ident) => {
         {
             const _: () = assert!(matches!($a, $b));
-            assert!(black_box($a) == black_box($b));
         }
     };
 }
 
 macro_rules! suite {
-    ( $tyname:ident: $( $tt:tt )* ) => {
+    ( $tyname:ident => $( $tt:tt )* ) => {
         fn f32() {
+            #[allow(unused)]
             type $tyname = f32;
-            suite_inner!(f32 $($tt)*);
+            suite_inner!(f32 => $($tt)*);
         }
 
         fn f64() {
+            #[allow(unused)]
             type $tyname = f64;
-            suite_inner!(f64 $($tt)*);
+            suite_inner!(f64 => $($tt)*);
         }
     }
 }
 
 macro_rules! suite_inner {
     (
-        $ty:ident [$( $fn:ident ),*]
-        $val:expr => [$($out:ident),*]
+        $ty:ident => [$( $fn:ident ),*]:
+        $(@cfg: $attr:meta)?
+        $val:expr => [$($out:ident),*],
 
         $( $tail:tt )*
     ) => {
-        $( both_assert!($ty::$fn($val), $out); )*
-        suite_inner!($ty [$($fn),*] $($tail)*)
+        $(#[cfg($attr)])?
+        {
+            // No black_box here! That can mask x87 failures.
+            $( assert_test!($ty::$fn($val), $out); )*
+        }
+        suite_inner!($ty => [$($fn),*]: $($tail)*)
     };
 
-    ( $ty:ident [$( $fn:ident ),*]) => {};
+    ( $ty:ident => [$( $fn:ident ),*]:) => {};
 }
 
 // The result of the `is_sign` methods are not checked for correctness, since we do not
 // guarantee anything about the signedness of NaNs. See
 // https://rust-lang.github.io/rfcs/3514-float-semantics.html.
 
-suite! { T: // type alias for the type we are testing
-                   [ classify, is_nan, is_infinite, is_finite, is_normal, is_sign_positive, is_sign_negative]
-     -0.0 / 0.0 => [      Nan,   true,       false,     false,     false,           NonDet,           NonDet]
-      0.0 / 0.0 => [      Nan,   true,       false,     false,     false,           NonDet,           NonDet]
-            1.0 => [   Normal,  false,       false,      true,      true,             true,            false]
-           -1.0 => [   Normal,  false,       false,      true,      true,            false,             true]
-            0.0 => [     Zero,  false,       false,      true,     false,             true,            false]
-           -0.0 => [     Zero,  false,       false,      true,     false,            false,             true]
-      1.0 / 0.0 => [ Infinite,  false,        true,     false,     false,             true,            false]
-     -1.0 / 0.0 => [ Infinite,  false,        true,     false,     false,            false,             true]
-   1.0 / T::MAX => [Subnormal,  false,       false,      true,     false,             true,            false]
-  -1.0 / T::MAX => [Subnormal,  false,       false,      true,     false,            false,             true]
+suite! { T => // type alias for the type we are testing
+                    [ classify, is_nan, is_infinite, is_finite, is_normal, is_sign_positive, is_sign_negative]:
+    black_box(0.0) / black_box(0.0) =>
+                    [      Nan,   true,       false,     false,     false,           NonDet,           NonDet],
+    black_box(0.0) / black_box(-0.0) =>
+                    [      Nan,   true,       false,     false,     false,           NonDet,           NonDet],
+    black_box(0.0) * black_box(T::INFINITY) =>
+                    [      Nan,   true,       false,     false,     false,           NonDet,           NonDet],
+    black_box(0.0) * black_box(T::NEG_INFINITY) =>
+                    [      Nan,   true,       false,     false,     false,           NonDet,           NonDet],
+             1.0 => [   Normal,  false,       false,      true,      true,             true,            false],
+            -1.0 => [   Normal,  false,       false,      true,      true,            false,             true],
+             0.0 => [     Zero,  false,       false,      true,     false,             true,            false],
+            -0.0 => [     Zero,  false,       false,      true,     false,            false,             true],
+    1.0 / black_box(0.0) =>
+                    [ Infinite,  false,        true,     false,     false,             true,            false],
+    -1.0 / black_box(0.0) =>
+                    [ Infinite,  false,        true,     false,     false,            false,             true],
+    2.0 * black_box(T::MAX) =>
+                    [ Infinite,  false,        true,     false,     false,             true,            false],
+    -2.0 * black_box(T::MAX) =>
+                    [ Infinite,  false,        true,     false,     false,            false,             true],
+    1.0 / black_box(T::MAX) =>
+                    [Subnormal,  false,       false,      true,     false,             true,            false],
+   -1.0 / black_box(T::MAX) =>
+                    [Subnormal,  false,       false,      true,     false,            false,             true],
+    // This specific expression causes trouble on x87 due to
+    // <https://github.com/rust-lang/rust/issues/114479>.
+    @cfg: not(all(target_arch = "x86", not(target_feature = "sse2")))
+    { let x = black_box(T::MAX); x * x } =>
+                    [ Infinite,  false,        true,     false,     false,             true,            false],
 }
 
 fn main() {
diff --git a/tests/ui/float/int-to-float-miscompile-issue-105626.rs b/tests/ui/float/int-to-float-miscompile-issue-105626.rs
