Skip to content

Commit 3c030e2

Browse files
bjorn3bjorn3
committed
Fix NaN handling of simd float min and max operations
1 parent f3d97cc commit 3c030e2

File tree

7 files changed

+83
-41
lines changed

7 files changed

+83
-41
lines changed

‎example/float-minmax-pass.rs

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// Copied from https://github.com/rust-lang/rust/blob/3fe3b89cd57229343eeca753fdd8c63d9b03c65c/src/test/ui/simd/intrinsic/float-minmax-pass.rs
2+
// run-pass
3+
// ignore-emscripten
4+
5+
// Test that the simd_f{min,max} intrinsics produce the correct results.
6+
7+
#![feature(repr_simd, platform_intrinsics)]
8+
#![allow(non_camel_case_types)]
9+
10+
#[repr(simd)]
11+
#[derive(Copy, Clone, PartialEq, Debug)]
12+
struct f32x4(pub f32, pub f32, pub f32, pub f32);
13+
14+
extern "platform-intrinsic" {
15+
fn simd_fmin<T>(x: T, y: T) -> T;
16+
fn simd_fmax<T>(x: T, y: T) -> T;
17+
}
18+
19+
fn main() {
20+
let x = f32x4(1.0, 2.0, 3.0, 4.0);
21+
let y = f32x4(2.0, 1.0, 4.0, 3.0);
22+
23+
#[cfg(not(any(target_arch = "mips", target_arch = "mips64")))]
24+
let nan = f32::NAN;
25+
// MIPS hardware treats f32::NAN as SNAN. Clear the signaling bit.
26+
// See https://github.com/rust-lang/rust/issues/52746.
27+
#[cfg(any(target_arch = "mips", target_arch = "mips64"))]
28+
let nan = f32::from_bits(f32::NAN.to_bits() - 1);
29+
30+
let n = f32x4(nan, nan, nan, nan);
31+
32+
unsafe {
33+
let min0 = simd_fmin(x, y);
34+
let min1 = simd_fmin(y, x);
35+
assert_eq!(min0, min1);
36+
let e = f32x4(1.0, 1.0, 3.0, 3.0);
37+
assert_eq!(min0, e);
38+
let minn = simd_fmin(x, n);
39+
assert_eq!(minn, x);
40+
let minn = simd_fmin(y, n);
41+
assert_eq!(minn, y);
42+
43+
let max0 = simd_fmax(x, y);
44+
let max1 = simd_fmax(y, x);
45+
assert_eq!(max0, max1);
46+
let e = f32x4(2.0, 2.0, 4.0, 4.0);
47+
assert_eq!(max0, e);
48+
let maxn = simd_fmax(x, n);
49+
assert_eq!(maxn, x);
50+
let maxn = simd_fmax(y, n);
51+
assert_eq!(maxn, y);
52+
}
53+
}

‎patches/0001-portable-simd-Disable-unsupported-tests.patch

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -106,22 +106,6 @@ diff --git a/crates/core_simd/tests/ops_macros.rs b/crates/core_simd/tests/ops_m
106106
index 31b7ee2..bd04b3c 100644
107107
--- a/crates/core_simd/tests/ops_macros.rs
108108
+++ b/crates/core_simd/tests/ops_macros.rs
109-
@@ -567,6 +567,7 @@ macro_rules! impl_float_tests {
110-
});
111-
}
112-
113-
+ /*
114-
fn horizontal_max<const LANES: usize>() {
115-
test_helpers::test_1(&|x| {
116-
let vmax = Vector::<LANES>::from_array(x).horizontal_max();
117-
@@ -590,6 +591,7 @@ macro_rules! impl_float_tests {
118-
Ok(())
119-
});
120-
}
121-
+ */
122-
}
123-
124-
#[cfg(feature = "std")]
125109
@@ -604,6 +606,7 @@ macro_rules! impl_float_tests {
126110
)
127111
}

‎scripts/test_rustc_tests.sh

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ rm src/test/ui/abi/stack-protector.rs # requires stack protector support
7979

8080
# giving different but possibly correct results
8181
# =============================================
82-
rm src/test/ui/simd/intrinsic/float-minmax-pass.rs # same
8382
rm src/test/ui/mir/mir_misc_casts.rs # depends on deduplication of constants
8483
rm src/test/ui/mir/mir_raw_fat_ptr.rs # same
8584
rm src/test/ui/consts/issue-33537.rs # same

‎scripts/tests.sh

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,10 @@ function base_sysroot_tests() {
7272
$MY_RUSTC example/track-caller-attribute.rs --crate-type bin -Cpanic=abort --target "$TARGET_TRIPLE"
7373
$RUN_WRAPPER ./target/out/track-caller-attribute
7474

75+
echo "[AOT] float-minmax-pass"
76+
$MY_RUSTC example/float-minmax-pass.rs --crate-type bin -Cpanic=abort --target "$TARGET_TRIPLE"
77+
$RUN_WRAPPER ./target/out/float-minmax-pass
78+
7579
echo "[AOT] mod_bench"
7680
$MY_RUSTC example/mod_bench.rs --crate-type bin --target "$TARGET_TRIPLE"
7781
$RUN_WRAPPER ./target/out/mod_bench

‎src/intrinsics/mod.rs

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,39 +1019,23 @@ fn codegen_regular_intrinsic_call<'tcx>(
10191019
ret.write_cvalue(fx, old);
10201020
};
10211021

1022-
// In Rust floating point min and max don't propagate NaN. In Cranelift they do however.
1023-
// For this reason it is necessary to use `a.is_nan() ? b : (a >= b ? b : a)` for `minnumf*`
1024-
// and `a.is_nan() ? b : (a <= b ? b : a)` for `maxnumf*`. NaN checks are done by comparing
1025-
// a float against itself. Only in case of NaN is it not equal to itself.
10261022
minnumf32, (v a, v b) {
1027-
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1028-
let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b);
1029-
let temp = fx.bcx.ins().select(a_ge_b, b, a);
1030-
let val = fx.bcx.ins().select(a_is_nan, b, temp);
1023+
let val = crate::num::codegen_float_min(fx, a, b);
10311024
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32));
10321025
ret.write_cvalue(fx, val);
10331026
};
10341027
minnumf64, (v a, v b) {
1035-
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1036-
let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b);
1037-
let temp = fx.bcx.ins().select(a_ge_b, b, a);
1038-
let val = fx.bcx.ins().select(a_is_nan, b, temp);
1028+
let val = crate::num::codegen_float_min(fx, a, b);
10391029
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64));
10401030
ret.write_cvalue(fx, val);
10411031
};
10421032
maxnumf32, (v a, v b) {
1043-
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1044-
let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b);
1045-
let temp = fx.bcx.ins().select(a_le_b, b, a);
1046-
let val = fx.bcx.ins().select(a_is_nan, b, temp);
1033+
let val = crate::num::codegen_float_max(fx, a, b);
10471034
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32));
10481035
ret.write_cvalue(fx, val);
10491036
};
10501037
maxnumf64, (v a, v b) {
1051-
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
1052-
let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b);
1053-
let temp = fx.bcx.ins().select(a_le_b, b, a);
1054-
let val = fx.bcx.ins().select(a_is_nan, b, temp);
1038+
let val = crate::num::codegen_float_max(fx, a, b);
10551039
let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64));
10561040
ret.write_cvalue(fx, val);
10571041
};

‎src/intrinsics/simd.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -354,8 +354,8 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
354354
_ => unreachable!("{:?}", lane_ty),
355355
}
356356
match intrinsic {
357-
sym::simd_fmin => fx.bcx.ins().fmin(x_lane, y_lane),
358-
sym::simd_fmax => fx.bcx.ins().fmax(x_lane, y_lane),
357+
sym::simd_fmin => crate::num::codegen_float_min(fx, x_lane, y_lane),
358+
sym::simd_fmax => crate::num::codegen_float_max(fx, x_lane, y_lane),
359359
_ => unreachable!(),
360360
}
361361
});
@@ -495,7 +495,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
495495
let lt = match ty.kind() {
496496
ty::Int(_) => fx.bcx.ins().icmp(IntCC::SignedLessThan, a, b),
497497
ty::Uint(_) => fx.bcx.ins().icmp(IntCC::UnsignedLessThan, a, b),
498-
ty::Float(_) => fx.bcx.ins().fcmp(FloatCC::LessThan, a, b),
498+
ty::Float(_) => return crate::num::codegen_float_min(fx, a, b),
499499
_ => unreachable!(),
500500
};
501501
fx.bcx.ins().select(lt, a, b)
@@ -512,7 +512,7 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
512512
let gt = match ty.kind() {
513513
ty::Int(_) => fx.bcx.ins().icmp(IntCC::SignedGreaterThan, a, b),
514514
ty::Uint(_) => fx.bcx.ins().icmp(IntCC::UnsignedGreaterThan, a, b),
515-
ty::Float(_) => fx.bcx.ins().fcmp(FloatCC::GreaterThan, a, b),
515+
ty::Float(_) => return crate::num::codegen_float_max(fx, a, b),
516516
_ => unreachable!(),
517517
};
518518
fx.bcx.ins().select(gt, a, b)

‎src/num.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -420,3 +420,21 @@ pub(crate) fn codegen_ptr_binop<'tcx>(
420420
CValue::by_val(fx.bcx.ins().bint(types::I8, res), fx.layout_of(fx.tcx.types.bool))
421421
}
422422
}
423+
424+
// In Rust floating point min and max don't propagate NaN. In Cranelift they do however.
425+
// For this reason it is necessary to use `a.is_nan() ? b : (a >= b ? b : a)` for `minnumf*`
426+
// and `a.is_nan() ? b : (a <= b ? b : a)` for `maxnumf*`. NaN checks are done by comparing
427+
// a float against itself. Only in case of NaN is it not equal to itself.
428+
pub(crate) fn codegen_float_min(fx: &mut FunctionCx<'_, '_, '_>, a: Value, b: Value) -> Value {
429+
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
430+
let a_ge_b = fx.bcx.ins().fcmp(FloatCC::GreaterThanOrEqual, a, b);
431+
let temp = fx.bcx.ins().select(a_ge_b, b, a);
432+
fx.bcx.ins().select(a_is_nan, b, temp)
433+
}
434+
435+
pub(crate) fn codegen_float_max(fx: &mut FunctionCx<'_, '_, '_>, a: Value, b: Value) -> Value {
436+
let a_is_nan = fx.bcx.ins().fcmp(FloatCC::NotEqual, a, a);
437+
let a_le_b = fx.bcx.ins().fcmp(FloatCC::LessThanOrEqual, a, b);
438+
let temp = fx.bcx.ins().select(a_le_b, b, a);
439+
fx.bcx.ins().select(a_is_nan, b, temp)
440+
}

0 commit comments

Comments
 (0)