Skip to content

Commit c6564f8

Browse files
bjorn3bjorn3
committed
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
1 parent e0b9f3b commit c6564f8

File tree

5 files changed

+23
-60
lines changed

5 files changed

+23
-60
lines changed

‎patches/0001-stdsimd-Disable-unsupported-tests.patch‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -124,22 +124,6 @@ index cb39e73..fc0ebe1 100644
124124

125125
fn sqrt<const LANES: usize>() {
126126
test_helpers::test_unary_elementwise(
127-
@@ -491,6 +493,7 @@ macro_rules! impl_float_tests {
128-
)
129-
}
130-
131-
+ /*
132-
fn min<const LANES: usize>() {
133-
// Regular conditions (both values aren't zero)
134-
test_helpers::test_binary_elementwise(
135-
@@ -536,6 +539,7 @@ macro_rules! impl_float_tests {
136-
assert!(p_zero.max(n_zero).to_array().iter().all(|x| *x == 0.));
137-
assert!(n_zero.max(p_zero).to_array().iter().all(|x| *x == 0.));
138-
}
139-
+ */
140-
141-
fn clamp<const LANES: usize>() {
142-
test_helpers::test_3(&|value: [Scalar; LANES], mut min: [Scalar; LANES], mut max: [Scalar; LANES]| {
143127
@@ -581,6 +585,7 @@ macro_rules! impl_float_tests {
144128
});
145129
}

‎patches/0023-sysroot-Ignore-failing-tests.patch‎

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -46,45 +46,5 @@ index 4bc44e9..8e3c7a4 100644
4646

4747
#[test]
4848
fn cell_allows_array_cycle() {
49-
diff --git a/library/core/tests/num/mod.rs b/library/core/tests/num/mod.rs
50-
index a17c094..5bb11d2 100644
51-
--- a/library/core/tests/num/mod.rs
52-
+++ b/library/core/tests/num/mod.rs
53-
@@ -651,11 +651,12 @@ macro_rules! test_float {
54-
assert_eq!((9.0 as $fty).min($neginf), $neginf);
55-
assert_eq!(($neginf as $fty).min(-9.0), $neginf);
56-
assert_eq!((-9.0 as $fty).min($neginf), $neginf);
57-
- assert_eq!(($nan as $fty).min(9.0), 9.0);
58-
- assert_eq!(($nan as $fty).min(-9.0), -9.0);
59-
- assert_eq!((9.0 as $fty).min($nan), 9.0);
60-
- assert_eq!((-9.0 as $fty).min($nan), -9.0);
61-
- assert!(($nan as $fty).min($nan).is_nan());
62-
+ // Cranelift fmin has NaN propagation
63-
+ //assert_eq!(($nan as $fty).min(9.0), 9.0);
64-
+ //assert_eq!(($nan as $fty).min(-9.0), -9.0);
65-
+ //assert_eq!((9.0 as $fty).min($nan), 9.0);
66-
+ //assert_eq!((-9.0 as $fty).min($nan), -9.0);
67-
+ //assert!(($nan as $fty).min($nan).is_nan());
68-
}
69-
#[test]
70-
fn max() {
71-
@@ -673,11 +674,12 @@ macro_rules! test_float {
72-
assert_eq!((9.0 as $fty).max($neginf), 9.0);
73-
assert_eq!(($neginf as $fty).max(-9.0), -9.0);
74-
assert_eq!((-9.0 as $fty).max($neginf), -9.0);
75-
- assert_eq!(($nan as $fty).max(9.0), 9.0);
76-
- assert_eq!(($nan as $fty).max(-9.0), -9.0);
77-
- assert_eq!((9.0 as $fty).max($nan), 9.0);
78-
- assert_eq!((-9.0 as $fty).max($nan), -9.0);
79-
- assert!(($nan as $fty).max($nan).is_nan());
80-
+ // Cranelift fmax has NaN propagation
81-
+ //assert_eq!(($nan as $fty).max(9.0), 9.0);
82-
+ //assert_eq!(($nan as $fty).max(-9.0), -9.0);
83-
+ //assert_eq!((9.0 as $fty).max($nan), 9.0);
84-
+ //assert_eq!((-9.0 as $fty).max($nan), -9.0);
85-
+ //assert!(($nan as $fty).max($nan).is_nan());
86-
}
87-
#[test]
88-
fn rem_euclid() {
8949
--
9050
2.21.0 (Apple Git-122)

‎scripts/tests.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ function extended_sysroot_tests() {
139139

140140
pushd stdsimd
141141
echo "[TEST] rust-lang/stdsimd"
142+
../build/cargo clean
142143
../build/cargo build --all-targets --target $TARGET_TRIPLE
143144
if [[ "$HOST_TRIPLE" = "$TARGET_TRIPLE" ]]; then
144145
../build/cargo test -q

‎src/intrinsics/mod.rs‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1029,23 +1029,39 @@ pub(crate) fn codegen_intrinsic_call<'tcx>(
10291029
ret.write_cvalue(fx, old);
10301030
};
10311031

1032+
// In Rust floating point min and max don't propagate NaN. In Cranelift they do however.
1033+
// For this reason it is necessary to use `a.is_nan() ? b : (a >= b ? b : a)` for `minnumf*`
1034+
// and `a.is_nan() ? b : (a <= b ? b : a)` for `maxnumf*`. NaN checks are done by comparing
1035+
// a float against itself. Only in case of NaN is it not equal to itself.
10321036
minnumf32, (v a, v b) {
1033-
let val = fx.bcx.ins().fmin(a, b);
1037+
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1038+
let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b);
1039+
let temp = fx.bcx.ins().select(a_ge_b, b, a);
1040+
let val = fx.bcx.ins().select(a_is_nan, b, temp);
10341041
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32));
10351042
ret.write_cvalue(fx, val);
10361043
};
10371044
minnumf64, (v a, v b) {
1038-
let val = fx.bcx.ins().fmin(a, b);
1045+
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1046+
let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b);
1047+
let temp = fx.bcx.ins().select(a_ge_b, b, a);
1048+
let val = fx.bcx.ins().select(a_is_nan, b, temp);
10391049
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64));
10401050
ret.write_cvalue(fx, val);
10411051
};
10421052
maxnumf32, (v a, v b) {
1043-
let val = fx.bcx.ins().fmax(a, b);
1053+
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1054+
let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b);
1055+
let temp = fx.bcx.ins().select(a_le_b, b, a);
1056+
let val = fx.bcx.ins().select(a_is_nan, b, temp);
10441057
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32));
10451058
ret.write_cvalue(fx, val);
10461059
};
10471060
maxnumf64, (v a, v b) {
1048-
let val = fx.bcx.ins().fmax(a, b);
1061+
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1062+
let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b);
1063+
let temp = fx.bcx.ins().select(a_le_b, b, a);
1064+
let val = fx.bcx.ins().select(a_is_nan, b, temp);
10491065
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64));
10501066
ret.write_cvalue(fx, val);
10511067
};

‎src/intrinsics/simd.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
378378
};
379379

380380
simd_reduce_min, (c v) {
381+
// FIXME support floats
381382
validate_simd_type!(fx, intrinsic, span, v.layout().ty);
382383
simd_reduce(fx, v, None, ret, |fx, layout, a, b| {
383384
let lt = fx.bcx.ins().icmp(if layout.ty.is_signed() {
@@ -390,6 +391,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
390391
};
391392

392393
simd_reduce_max, (c v) {
394+
// FIXME support floats
393395
validate_simd_type!(fx, intrinsic, span, v.layout().ty);
394396
simd_reduce(fx, v, None, ret, |fx, layout, a, b| {
395397
let gt = fx.bcx.ins().icmp(if layout.ty.is_signed() {

0 commit comments

Comments
 (0)