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 rust-lang#1049
bjorn3 · Jul 29, 2021 · c6564f8 · c6564f8
1 parent e0b9f3b
commit c6564f8
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 60 deletions.
diff --git 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<const LANES: usize>() {
                      test_helpers::test_unary_elementwise(
-@@ -491,6 +493,7 @@ macro_rules! impl_float_tests {
-                     )
-                 }
-
-+                /*
-                 fn min<const LANES: usize>() {
-                     // 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<const LANES: usize>() {
-                     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
@@ -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
@@ -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
@@ -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
@@ -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() {