From 7f7c73bd9c1f0b27078f65fc16ce79dff9d25827 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 11 Sep 2024 10:53:39 +0200 Subject: [PATCH 1/2] simplify float::classify logic --- library/core/src/num/f128.rs | 4 - library/core/src/num/f16.rs | 44 ++-------- library/core/src/num/f32.rs | 48 +++-------- library/core/src/num/f64.rs | 44 +++------- tests/ui/float/classify-runtime-const.rs | 102 +++++++++++++++++------ 5 files changed, 106 insertions(+), 136 deletions(-) diff --git a/library/core/src/num/f128.rs b/library/core/src/num/f128.rs index 1959628bd8f59..100271fa54c1f 100644 --- 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 index da92da1086dab..6bdc569df28bd 100644 --- 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 index 885f7608a337e..4c2a4ee3b3255 100644 --- 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 index 28cc231ccc76d..87fb5fe7ebeea 100644 --- 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/tests/ui/float/classify-runtime-const.rs b/tests/ui/float/classify-runtime-const.rs index 59a232c255e8d..b19e67d5284d0 100644 --- 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(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 + // . + @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() { From b72b42d36faf301f8cd6c31372525ab69ed752cc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 11 Sep 2024 20:26:56 +0200 Subject: [PATCH 2/2] move a test to a better location --- src/tools/tidy/src/issues.txt | 1 - .../int-to-float-miscompile-issue-105626.rs} | 0 2 files changed, 1 deletion(-) rename tests/ui/{numbers-arithmetic/issue-105626.rs => float/int-to-float-miscompile-issue-105626.rs} (100%) diff --git a/src/tools/tidy/src/issues.txt b/src/tools/tidy/src/issues.txt index 5205fa1429435..4627079a6479c 100644 --- a/src/tools/tidy/src/issues.txt +++ b/src/tools/tidy/src/issues.txt @@ -3182,7 +3182,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/numbers-arithmetic/issue-105626.rs b/tests/ui/float/int-to-float-miscompile-issue-105626.rs similarity index 100% rename from tests/ui/numbers-arithmetic/issue-105626.rs rename to tests/ui/float/int-to-float-miscompile-issue-105626.rs