From c6564f814e39da74c6e551858e2040abc37e70f1 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Thu, 29 Jul 2021 15:20:37 +0200 Subject: [PATCH] Fix float min and max operations in presence of NaN Cranelift's fmin and fmax instructions propagate NaN, while Rust's min and max don't. Fixes #1049 --- ...01-stdsimd-Disable-unsupported-tests.patch | 16 -------- .../0023-sysroot-Ignore-failing-tests.patch | 40 ------------------- scripts/tests.sh | 1 + src/intrinsics/mod.rs | 24 +++++++++-- src/intrinsics/simd.rs | 2 + 5 files changed, 23 insertions(+), 60 deletions(-) diff --git a/patches/0001-stdsimd-Disable-unsupported-tests.patch b/patches/0001-stdsimd-Disable-unsupported-tests.patch index b24f67f3e..731c60fda 100644 --- a/patches/0001-stdsimd-Disable-unsupported-tests.patch +++ b/patches/0001-stdsimd-Disable-unsupported-tests.patch @@ -124,22 +124,6 @@ index cb39e73..fc0ebe1 100644 fn sqrt() { test_helpers::test_unary_elementwise( -@@ -491,6 +493,7 @@ macro_rules! impl_float_tests { - ) - } - -+ /* - fn min() { - // Regular conditions (both values aren't zero) - test_helpers::test_binary_elementwise( -@@ -536,6 +539,7 @@ macro_rules! impl_float_tests { - assert!(p_zero.max(n_zero).to_array().iter().all(|x| *x == 0.)); - assert!(n_zero.max(p_zero).to_array().iter().all(|x| *x == 0.)); - } -+ */ - - fn clamp() { - test_helpers::test_3(&|value: [Scalar; LANES], mut min: [Scalar; LANES], mut max: [Scalar; LANES]| { @@ -581,6 +585,7 @@ macro_rules! impl_float_tests { }); } diff --git a/patches/0023-sysroot-Ignore-failing-tests.patch b/patches/0023-sysroot-Ignore-failing-tests.patch index 5d2c3049f..50ef0bd94 100644 --- a/patches/0023-sysroot-Ignore-failing-tests.patch +++ b/patches/0023-sysroot-Ignore-failing-tests.patch @@ -46,45 +46,5 @@ index 4bc44e9..8e3c7a4 100644 #[test] fn cell_allows_array_cycle() { -diff --git a/library/core/tests/num/mod.rs b/library/core/tests/num/mod.rs -index a17c094..5bb11d2 100644 ---- a/library/core/tests/num/mod.rs -+++ b/library/core/tests/num/mod.rs -@@ -651,11 +651,12 @@ macro_rules! test_float { - assert_eq!((9.0 as $fty).min($neginf), $neginf); - assert_eq!(($neginf as $fty).min(-9.0), $neginf); - assert_eq!((-9.0 as $fty).min($neginf), $neginf); -- assert_eq!(($nan as $fty).min(9.0), 9.0); -- assert_eq!(($nan as $fty).min(-9.0), -9.0); -- assert_eq!((9.0 as $fty).min($nan), 9.0); -- assert_eq!((-9.0 as $fty).min($nan), -9.0); -- assert!(($nan as $fty).min($nan).is_nan()); -+ // Cranelift fmin has NaN propagation -+ //assert_eq!(($nan as $fty).min(9.0), 9.0); -+ //assert_eq!(($nan as $fty).min(-9.0), -9.0); -+ //assert_eq!((9.0 as $fty).min($nan), 9.0); -+ //assert_eq!((-9.0 as $fty).min($nan), -9.0); -+ //assert!(($nan as $fty).min($nan).is_nan()); - } - #[test] - fn max() { -@@ -673,11 +674,12 @@ macro_rules! test_float { - assert_eq!((9.0 as $fty).max($neginf), 9.0); - assert_eq!(($neginf as $fty).max(-9.0), -9.0); - assert_eq!((-9.0 as $fty).max($neginf), -9.0); -- assert_eq!(($nan as $fty).max(9.0), 9.0); -- assert_eq!(($nan as $fty).max(-9.0), -9.0); -- assert_eq!((9.0 as $fty).max($nan), 9.0); -- assert_eq!((-9.0 as $fty).max($nan), -9.0); -- assert!(($nan as $fty).max($nan).is_nan()); -+ // Cranelift fmax has NaN propagation -+ //assert_eq!(($nan as $fty).max(9.0), 9.0); -+ //assert_eq!(($nan as $fty).max(-9.0), -9.0); -+ //assert_eq!((9.0 as $fty).max($nan), 9.0); -+ //assert_eq!((-9.0 as $fty).max($nan), -9.0); -+ //assert!(($nan as $fty).max($nan).is_nan()); - } - #[test] - fn rem_euclid() { -- 2.21.0 (Apple Git-122) diff --git a/scripts/tests.sh b/scripts/tests.sh index 1b8858eb4..0eef71023 100755 --- a/scripts/tests.sh +++ b/scripts/tests.sh @@ -139,6 +139,7 @@ function extended_sysroot_tests() { pushd stdsimd echo "[TEST] rust-lang/stdsimd" + ../build/cargo clean ../build/cargo build --all-targets --target $TARGET_TRIPLE if [[ "$HOST_TRIPLE" = "$TARGET_TRIPLE" ]]; then ../build/cargo test -q diff --git a/src/intrinsics/mod.rs b/src/intrinsics/mod.rs index 12c886287..866984607 100644 --- a/src/intrinsics/mod.rs +++ b/src/intrinsics/mod.rs @@ -1029,23 +1029,39 @@ pub(crate) fn codegen_intrinsic_call<'tcx>( ret.write_cvalue(fx, old); }; + // In Rust floating point min and max don't propagate NaN. In Cranelift they do however. + // For this reason it is necessary to use `a.is_nan() ? b : (a >= b ? b : a)` for `minnumf*` + // and `a.is_nan() ? b : (a <= b ? b : a)` for `maxnumf*`. NaN checks are done by comparing + // a float against itself. Only in case of NaN is it not equal to itself. minnumf32, (v a, v b) { - let val = fx.bcx.ins().fmin(a, b); + let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a); + let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b); + let temp = fx.bcx.ins().select(a_ge_b, b, a); + let val = fx.bcx.ins().select(a_is_nan, b, temp); let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32)); ret.write_cvalue(fx, val); }; minnumf64, (v a, v b) { - let val = fx.bcx.ins().fmin(a, b); + let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a); + let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b); + let temp = fx.bcx.ins().select(a_ge_b, b, a); + let val = fx.bcx.ins().select(a_is_nan, b, temp); let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64)); ret.write_cvalue(fx, val); }; maxnumf32, (v a, v b) { - let val = fx.bcx.ins().fmax(a, b); + let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a); + let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b); + let temp = fx.bcx.ins().select(a_le_b, b, a); + let val = fx.bcx.ins().select(a_is_nan, b, temp); let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32)); ret.write_cvalue(fx, val); }; maxnumf64, (v a, v b) { - let val = fx.bcx.ins().fmax(a, b); + let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a); + let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b); + let temp = fx.bcx.ins().select(a_le_b, b, a); + let val = fx.bcx.ins().select(a_is_nan, b, temp); let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64)); ret.write_cvalue(fx, val); }; diff --git a/src/intrinsics/simd.rs b/src/intrinsics/simd.rs index 37906ab47..43e68b4af 100644 --- a/src/intrinsics/simd.rs +++ b/src/intrinsics/simd.rs @@ -378,6 +378,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>( }; simd_reduce_min, (c v) { + // FIXME support floats validate_simd_type!(fx, intrinsic, span, v.layout().ty); simd_reduce(fx, v, None, ret, |fx, layout, a, b| { let lt = fx.bcx.ins().icmp(if layout.ty.is_signed() { @@ -390,6 +391,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>( }; simd_reduce_max, (c v) { + // FIXME support floats validate_simd_type!(fx, intrinsic, span, v.layout().ty); simd_reduce(fx, v, None, ret, |fx, layout, a, b| { let gt = fx.bcx.ins().icmp(if layout.ty.is_signed() {