Skip to content

Commit

Permalink
Auto merge of #130220 - RalfJung:float-classify, r=workingjubilee
Browse files Browse the repository at this point in the history 
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](#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.
  • Loading branch information
@bors
bors committed Sep 16, 2024
2 parents c16ff44 + b72b42d commit 39b7669
Show file tree
Hide file tree
Showing 7 changed files with 106 additions and 137 deletions.
4 changes: 0 additions & 4 deletions library/core/src/num/f128.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
44 changes: 7 additions & 37 deletions library/core/src/num/f16.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
48 changes: 12 additions & 36 deletions library/core/src/num/f32.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
44 changes: 12 additions & 32 deletions library/core/src/num/f64.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
1 change: 0 additions & 1 deletion src/tools/tidy/src/issues.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
102 changes: 75 additions & 27 deletions tests/ui/float/classify-runtime-const.rs
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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() {
Expand Down
0 tests/ui/numbers-arithmetic/issue-105626.rs → ...t/int-to-float-miscompile-issue-105626.rs
View file
File renamed without changes.

0 comments on commit 39b7669

Please sign in to comment.