Skip to content

simplify float::classify logic #130220

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 16, 2024
Merged

simplify float::classify logic #130220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f7c73b
simplify float::classify logic
RalfJung Sep 11, 2024
b72b42d
move a test to a better location
RalfJung Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions library/core/src/num/f128.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
102 changes: 75 additions & 27 deletions tests/ui/float/classify-runtime-const.rs
View file
Open in desktop
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
Open in desktop
File renamed without changes.
Loading