From 4cc696709877042bd1aadadeecfa9d3c134487a4 Mon Sep 17 00:00:00 2001 From: Atlas Dostal Date: Thu, 29 Sep 2022 22:50:01 -0700 Subject: [PATCH] Fix negation and signum to consistently handle negative zero across platforms (#344) * Fix negation and signum to consistently handle negative zero across platforms * rustfmt --- codegen/templates/vec.rs.tera | 19 +++++++++++++------ src/f32/sse2/vec3a.rs | 11 ++++++----- src/f32/sse2/vec4.rs | 11 ++++++----- src/f32/wasm32/vec3a.rs | 9 +++++---- src/f32/wasm32/vec4.rs | 9 +++++---- tests/vec2.rs | 4 +++- tests/vec3.rs | 10 +++++++++- tests/vec4.rs | 10 +++++++++- 8 files changed, 56 insertions(+), 27 deletions(-) diff --git a/codegen/templates/vec.rs.tera b/codegen/templates/vec.rs.tera index e8128ce0..18506be3 100644 --- a/codegen/templates/vec.rs.tera +++ b/codegen/templates/vec.rs.tera @@ -811,11 +811,18 @@ impl {{ self_t }} { } {% elif is_coresimd %} Self(self.0.signum()) - {% else %} - let mask = self.cmpge(Self::ZERO); - let result = Self::select(mask, Self::ONE, Self::NEG_ONE); - let mask = self.is_nan_mask(); - Self::select(mask, self, result) + {% elif is_sse2 %} + unsafe { + let result = Self(_mm_or_ps(_mm_and_ps(self.0, Self::NEG_ONE.0), Self::ONE.0)); + let mask = self.is_nan_mask(); + Self::select(mask, self, result) + } + {% elif is_wasm32 %} + unsafe { + let result = Self(v128_or(v128_and(self.0, Self::NEG_ONE.0), Self::ONE.0)); + let mask = self.is_nan_mask(); + Self::select(mask, self, result) + } {% endif %} } {% endif %} @@ -2035,7 +2042,7 @@ impl Neg for {{ self_t }} { {%- endfor %} } {% elif is_sse2 %} - Self(unsafe { _mm_sub_ps(Self::ZERO.0, self.0) }) + Self(unsafe { _mm_xor_ps(_mm_set1_ps(-0.0), self.0) }) {% elif is_wasm32 %} Self(f32x4_neg(self.0)) {% elif is_coresimd %} diff --git a/src/f32/sse2/vec3a.rs b/src/f32/sse2/vec3a.rs index 734d3eaa..ace0933f 100644 --- a/src/f32/sse2/vec3a.rs +++ b/src/f32/sse2/vec3a.rs @@ -312,10 +312,11 @@ impl Vec3A { /// - `NAN` if the number is `NAN` #[inline] pub fn signum(self) -> Self { - let mask = self.cmpge(Self::ZERO); - let result = Self::select(mask, Self::ONE, Self::NEG_ONE); - let mask = self.is_nan_mask(); - Self::select(mask, self, result) + unsafe { + let result = Self(_mm_or_ps(_mm_and_ps(self.0, Self::NEG_ONE.0), Self::ONE.0)); + let mask = self.is_nan_mask(); + Self::select(mask, self, result) + } } /// Returns `true` if, and only if, all elements are finite. If any element is either @@ -993,7 +994,7 @@ impl Neg for Vec3A { type Output = Self; #[inline] fn neg(self) -> Self { - Self(unsafe { _mm_sub_ps(Self::ZERO.0, self.0) }) + Self(unsafe { _mm_xor_ps(_mm_set1_ps(-0.0), self.0) }) } } diff --git a/src/f32/sse2/vec4.rs b/src/f32/sse2/vec4.rs index e318cade..906f78a6 100644 --- a/src/f32/sse2/vec4.rs +++ b/src/f32/sse2/vec4.rs @@ -287,10 +287,11 @@ impl Vec4 { /// - `NAN` if the number is `NAN` #[inline] pub fn signum(self) -> Self { - let mask = self.cmpge(Self::ZERO); - let result = Self::select(mask, Self::ONE, Self::NEG_ONE); - let mask = self.is_nan_mask(); - Self::select(mask, self, result) + unsafe { + let result = Self(_mm_or_ps(_mm_and_ps(self.0, Self::NEG_ONE.0), Self::ONE.0)); + let mask = self.is_nan_mask(); + Self::select(mask, self, result) + } } /// Returns `true` if, and only if, all elements are finite. If any element is either @@ -906,7 +907,7 @@ impl Neg for Vec4 { type Output = Self; #[inline] fn neg(self) -> Self { - Self(unsafe { _mm_sub_ps(Self::ZERO.0, self.0) }) + Self(unsafe { _mm_xor_ps(_mm_set1_ps(-0.0), self.0) }) } } diff --git a/src/f32/wasm32/vec3a.rs b/src/f32/wasm32/vec3a.rs index 678b781e..4ee5d7ec 100644 --- a/src/f32/wasm32/vec3a.rs +++ b/src/f32/wasm32/vec3a.rs @@ -293,10 +293,11 @@ impl Vec3A { /// - `NAN` if the number is `NAN` #[inline] pub fn signum(self) -> Self { - let mask = self.cmpge(Self::ZERO); - let result = Self::select(mask, Self::ONE, Self::NEG_ONE); - let mask = self.is_nan_mask(); - Self::select(mask, self, result) + unsafe { + let result = Self(v128_or(v128_and(self.0, Self::NEG_ONE.0), Self::ONE.0)); + let mask = self.is_nan_mask(); + Self::select(mask, self, result) + } } /// Returns `true` if, and only if, all elements are finite. If any element is either diff --git a/src/f32/wasm32/vec4.rs b/src/f32/wasm32/vec4.rs index 55386ada..1fc475bf 100644 --- a/src/f32/wasm32/vec4.rs +++ b/src/f32/wasm32/vec4.rs @@ -275,10 +275,11 @@ impl Vec4 { /// - `NAN` if the number is `NAN` #[inline] pub fn signum(self) -> Self { - let mask = self.cmpge(Self::ZERO); - let result = Self::select(mask, Self::ONE, Self::NEG_ONE); - let mask = self.is_nan_mask(); - Self::select(mask, self, result) + unsafe { + let result = Self(v128_or(v128_and(self.0, Self::NEG_ONE.0), Self::ONE.0)); + let mask = self.is_nan_mask(); + Self::select(mask, self, result) + } } /// Returns `true` if, and only if, all elements are finite. If any element is either diff --git a/tests/vec2.rs b/tests/vec2.rs index 04c452b6..8a4163cb 100644 --- a/tests/vec2.rs +++ b/tests/vec2.rs @@ -478,6 +478,8 @@ macro_rules! impl_vec2_signed_tests { glam_test!(test_neg, { let a = $new(1 as $t, 2 as $t); assert_eq!($new(-1 as $t, -2 as $t), (-a)); + assert_eq!($new(-0.0 as $t, -0.0 as $t), -$new(0.0 as $t, 0.0 as $t)); + assert_eq!($new(0.0 as $t, -0.0 as $t), -$new(-0.0 as $t, 0.0 as $t)); }); glam_test!(test_perp, { @@ -596,7 +598,7 @@ macro_rules! impl_vec2_float_tests { glam_test!(test_sign, { assert_eq!($vec2::ZERO.signum(), $vec2::ONE); - assert_eq!(-$vec2::ZERO.signum(), -$vec2::ONE); + assert_eq!((-$vec2::ZERO).signum(), -$vec2::ONE); assert_eq!($vec2::ONE.signum(), $vec2::ONE); assert_eq!((-$vec2::ONE).signum(), -$vec2::ONE); assert_eq!($vec2::splat(INFINITY).signum(), $vec2::ONE); diff --git a/tests/vec3.rs b/tests/vec3.rs index 781368e1..095264b1 100644 --- a/tests/vec3.rs +++ b/tests/vec3.rs @@ -523,6 +523,14 @@ macro_rules! impl_vec3_signed_tests { glam_test!(test_neg, { let a = $new(1 as $t, 2 as $t, 3 as $t); assert_eq!((-1 as $t, -2 as $t, -3 as $t), (-a).into()); + assert_eq!( + $new(-0.0 as $t, -0.0 as $t, -0.0 as $t), + -$new(0.0 as $t, 0.0 as $t, 0.0 as $t) + ); + assert_eq!( + $new(0.0 as $t, -0.0 as $t, -0.0 as $t), + -$new(-0.0 as $t, 0.0 as $t, 0.0 as $t) + ); }); glam_test!(test_dot_signed, { @@ -652,7 +660,7 @@ macro_rules! impl_vec3_float_tests { glam_test!(test_signum, { assert_eq!($vec3::ZERO.signum(), $vec3::ONE); - assert_eq!(-$vec3::ZERO.signum(), -$vec3::ONE); + assert_eq!((-$vec3::ZERO).signum(), -$vec3::ONE); assert_eq!($vec3::ONE.signum(), $vec3::ONE); assert_eq!((-$vec3::ONE).signum(), -$vec3::ONE); assert_eq!($vec3::splat(INFINITY).signum(), $vec3::ONE); diff --git a/tests/vec4.rs b/tests/vec4.rs index e016a597..2646727b 100644 --- a/tests/vec4.rs +++ b/tests/vec4.rs @@ -598,6 +598,14 @@ macro_rules! impl_vec4_signed_tests { glam_test!(test_neg, { let a = $new(1 as $t, 2 as $t, 3 as $t, 4 as $t); assert_eq!((-1 as $t, -2 as $t, -3 as $t, -4 as $t), (-a).into()); + assert_eq!( + $new(-0.0 as $t, -0.0 as $t, -0.0 as $t, -0.0 as $t), + -$new(0.0 as $t, 0.0 as $t, 0.0 as $t, 0.0 as $t) + ); + assert_eq!( + $new(0.0 as $t, -0.0 as $t, -0.0 as $t, -0.0 as $t), + -$new(-0.0 as $t, 0.0 as $t, 0.0 as $t, 0.0 as $t) + ); }); glam_test!(test_dot_signed, { @@ -734,7 +742,7 @@ macro_rules! impl_vec4_float_tests { glam_test!(test_signum, { assert_eq!($vec4::ZERO.signum(), $vec4::ONE); - assert_eq!(-$vec4::ZERO.signum(), -$vec4::ONE); + assert_eq!((-$vec4::ZERO).signum(), -$vec4::ONE); assert_eq!($vec4::ONE.signum(), $vec4::ONE); assert_eq!((-$vec4::ONE).signum(), -$vec4::ONE); assert_eq!($vec4::splat(INFINITY).signum(), $vec4::ONE);