From a2074e5bb64d43e3376bc547ef83b0fd3099dc31 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sat, 25 Jan 2025 04:18:44 +0000 Subject: [PATCH 1/3] Remove remnants of the `checked` feature The Cargo feature `checked` was added in d166a30c1137 ("Overhaul tests") and later removed in 5e0eca75fb14 ("swap stable to be unstable, checked is now debug_assertions"). However, there are a few remaining uses of `feature = "checked"` that did not get removed. Clean these up here. --- build.rs | 19 +++++++------------ .../compiler-builtins-smoke-test/Cargo.toml | 1 - src/math/rem_pio2_large.rs | 5 +++-- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/build.rs b/build.rs index ca4a639a1..caf5a108a 100644 --- a/build.rs +++ b/build.rs @@ -8,18 +8,13 @@ fn main() { println!("cargo:rerun-if-changed=build.rs"); println!("cargo:rustc-check-cfg=cfg(assert_no_panic)"); - println!("cargo:rustc-check-cfg=cfg(feature, values(\"checked\"))"); - - #[allow(unexpected_cfgs)] - if !cfg!(feature = "checked") { - let lvl = env::var("OPT_LEVEL").unwrap(); - if lvl != "0" && !cfg!(debug_assertions) { - println!("cargo:rustc-cfg=assert_no_panic"); - } else if env::var("ENSURE_NO_PANIC").is_ok() { - // Give us a defensive way of ensureing that no-panic is checked when we - // expect it to be. - panic!("`assert_no_panic `was not enabled"); - } + let lvl = env::var("OPT_LEVEL").unwrap(); + if lvl != "0" && !cfg!(debug_assertions) { + println!("cargo:rustc-cfg=assert_no_panic"); + } else if env::var("ENSURE_NO_PANIC").is_ok() { + // Give us a defensive way of ensureing that no-panic is checked when we + // expect it to be. + panic!("`assert_no_panic `was not enabled"); } configure::emit_libm_config(&cfg); diff --git a/crates/compiler-builtins-smoke-test/Cargo.toml b/crates/compiler-builtins-smoke-test/Cargo.toml index d578b0dcd..24b33645e 100644 --- a/crates/compiler-builtins-smoke-test/Cargo.toml +++ b/crates/compiler-builtins-smoke-test/Cargo.toml @@ -22,7 +22,6 @@ unexpected_cfgs = { level = "warn", check-cfg = [ "cfg(arch_enabled)", "cfg(assert_no_panic)", "cfg(intrinsics_enabled)", - 'cfg(feature, values("checked"))', 'cfg(feature, values("force-soft-floats"))', 'cfg(feature, values("unstable"))', 'cfg(feature, values("unstable-intrinsics"))', diff --git a/src/math/rem_pio2_large.rs b/src/math/rem_pio2_large.rs index ec8397f4b..6d679bbe9 100644 --- a/src/math/rem_pio2_large.rs +++ b/src/math/rem_pio2_large.rs @@ -226,8 +226,9 @@ pub(crate) fn rem_pio2_large(x: &[f64], y: &mut [f64], e0: i32, prec: usize) -> let x1p24 = f64::from_bits(0x4170000000000000); // 0x1p24 === 2 ^ 24 let x1p_24 = f64::from_bits(0x3e70000000000000); // 0x1p_24 === 2 ^ (-24) - #[cfg(all(target_pointer_width = "64", feature = "checked"))] - assert!(e0 <= 16360); + if cfg!(target_pointer_width = "64") { + debug_assert!(e0 <= 16360); + } let nx = x.len(); From b1e7ea093ecca1f866269c625975e0df1bbca46f Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Sat, 25 Jan 2025 05:22:11 +0000 Subject: [PATCH 2/3] Rework the available Cargo profiles Currently the default release profile enables LTO and single CGU builds, which is very slow to build. Most tests are better run with optimizations enabled since it allows testing a much larger number of inputs, so it is inconvenient that building can sometimes take significantly longer than the tests. Remedy this by doing the following: * Move the existing `release` profile to `release-opt`. * With the above, the default `release` profile is untouched (16 CGUs and thin local LTO). * `release-checked` inherits `release`, so no LTO or single CGU. This means that the simple `cargo test --release` becomes much faster for local development. We are able to enable the other profiles as needed in CI. Tests should ideally still be run with `--profile release-checked` to ensure there are no debug assetions or unexpected wrapping math hit. `no-panic` still needs a single CGU, so must be run with `--profile release-opt`. Since it is not possible to detect CGU or profilel configuration from within build scripts, the `ENSURE_NO_PANIC` environment variable must now always be set. --- Cargo.toml | 14 ++++++++------ build.rs | 8 ++------ ci/run.sh | 12 +++++++++++- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7b6f9e1ce..08342a929 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,18 +61,20 @@ exclude = [ [dev-dependencies] no-panic = "0.1.33" -[profile.release] -# Options for no-panic to correctly detect the lack of panics -codegen-units = 1 -lto = "fat" +# The default release profile is unchanged. # Release mode with debug assertions [profile.release-checked] -codegen-units = 1 +inherits = "release" debug-assertions = true +overflow-checks = true + +# Release with maximum optimizations, which is very slow to build. This is also +# what is needed to check `no-panic`. +[profile.release-opt] inherits = "release" +codegen-units = 1 lto = "fat" -overflow-checks = true [profile.bench] # Required for iai-callgrind diff --git a/build.rs b/build.rs index caf5a108a..7042b54d7 100644 --- a/build.rs +++ b/build.rs @@ -8,13 +8,9 @@ fn main() { println!("cargo:rerun-if-changed=build.rs"); println!("cargo:rustc-check-cfg=cfg(assert_no_panic)"); - let lvl = env::var("OPT_LEVEL").unwrap(); - if lvl != "0" && !cfg!(debug_assertions) { + // If set, enable `no-panic`. Requires LTO (`release-opt` profile). + if env::var("ENSURE_NO_PANIC").is_ok() { println!("cargo:rustc-cfg=assert_no_panic"); - } else if env::var("ENSURE_NO_PANIC").is_ok() { - // Give us a defensive way of ensureing that no-panic is checked when we - // expect it to be. - panic!("`assert_no_panic `was not enabled"); } configure::emit_libm_config(&cfg); diff --git a/ci/run.sh b/ci/run.sh index 296986d97..a946d325e 100755 --- a/ci/run.sh +++ b/ci/run.sh @@ -117,4 +117,14 @@ $cmd "$profile" release-checked --features unstable-intrinsics $cmd "$profile" release-checked --features unstable-intrinsics --benches # Ensure that the routines do not panic. -ENSURE_NO_PANIC=1 cargo build -p libm --target "$target" --no-default-features --release +# +# `--tests` must be passed because no-panic is only enabled as a dev +# dependency. The `release-opt` profile must be used to enable LTO and a +# single CGU. +ENSURE_NO_PANIC=1 cargo build \ + -p libm \ + --target "$target" \ + --no-default-features \ + --features unstable-float \ + --tests \ + --profile release-opt From f5f789adec1c7d257ecb676acb4a225dbbb6484f Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Mon, 27 Jan 2025 11:37:01 +0000 Subject: [PATCH 3/3] Ignore specific `atan2` and `sin` tests on i586 There seems to be a case of unsoundness with the `i586` version of `atan2`. For the following test: assert_eq!(atan2(2.0, -1.0), atan(2.0 / -1.0) + PI);atan2(2.0, -1.0) The output is optimization-dependent. The new `release-checked` profile produces the following failure: thread 'math::atan2::sanity_check' panicked at src/math/atan2.rs:123:5: assertion `left == right` failed left: 2.0344439357957027 right: 2.0344439357957027 Similarly, `sin::test_near_pi` fails with the following: thread 'math::sin::test_near_pi' panicked at src/math/sin.rs:91:5: assertion `left == right` failed left: 6.273720864039203e-7 right: 6.273720864039205e-7 Mark the tests ignored on `i586` for now. --- src/math/atan2.rs | 22 ++++++++++++++-------- src/math/sin.rs | 19 +++++++++++-------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/math/atan2.rs b/src/math/atan2.rs index b9bf0da93..c668731cf 100644 --- a/src/math/atan2.rs +++ b/src/math/atan2.rs @@ -114,12 +114,18 @@ pub fn atan2(y: f64, x: f64) -> f64 { } } -#[test] -fn sanity_check() { - assert_eq!(atan2(0.0, 1.0), 0.0); - assert_eq!(atan2(0.0, -1.0), PI); - assert_eq!(atan2(-0.0, -1.0), -PI); - assert_eq!(atan2(3.0, 2.0), atan(3.0 / 2.0)); - assert_eq!(atan2(2.0, -1.0), atan(2.0 / -1.0) + PI); - assert_eq!(atan2(-2.0, -1.0), atan(-2.0 / -1.0) - PI); +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[cfg_attr(x86_no_sse, ignore = "FIXME(i586): possible incorrect rounding")] + fn sanity_check() { + assert_eq!(atan2(0.0, 1.0), 0.0); + assert_eq!(atan2(0.0, -1.0), PI); + assert_eq!(atan2(-0.0, -1.0), -PI); + assert_eq!(atan2(3.0, 2.0), atan(3.0 / 2.0)); + assert_eq!(atan2(2.0, -1.0), atan(2.0 / -1.0) + PI); + assert_eq!(atan2(-2.0, -1.0), atan(-2.0 / -1.0) - PI); + } } diff --git a/src/math/sin.rs b/src/math/sin.rs index e04e0d6a0..229fa4bef 100644 --- a/src/math/sin.rs +++ b/src/math/sin.rs @@ -81,12 +81,15 @@ pub fn sin(x: f64) -> f64 { } } -#[test] -fn test_near_pi() { - let x = f64::from_bits(0x400921fb000FD5DD); // 3.141592026217707 - let sx = f64::from_bits(0x3ea50d15ced1a4a2); // 6.273720864039205e-7 - let result = sin(x); - #[cfg(all(target_arch = "x86", not(target_feature = "sse2")))] - let result = force_eval!(result); - assert_eq!(result, sx); +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[cfg_attr(x86_no_sse, ignore = "FIXME(i586): possible incorrect rounding")] + fn test_near_pi() { + let x = f64::from_bits(0x400921fb000FD5DD); // 3.141592026217707 + let sx = f64::from_bits(0x3ea50d15ced1a4a2); // 6.273720864039205e-7 + assert_eq!(sin(x), sx); + } }