From afad9c3f644ddbfef3301f617cb9d23ca4e71fe0 Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Sat, 22 Apr 2023 21:12:35 +0000 Subject: [PATCH 1/3] Don't use direct field access in `Simd` functions --- crates/core_simd/src/lib.rs | 2 ++ crates/core_simd/src/vector.rs | 26 ++++++++++++++++++-------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/crates/core_simd/src/lib.rs b/crates/core_simd/src/lib.rs index 927b1654f8e..a372e2e40c4 100644 --- a/crates/core_simd/src/lib.rs +++ b/crates/core_simd/src/lib.rs @@ -1,6 +1,8 @@ #![no_std] #![feature( const_ptr_read, + const_refs_to_cell, + const_transmute_copy, convert_float_to_int, decl_macro, intra_doc_pointers, diff --git a/crates/core_simd/src/vector.rs b/crates/core_simd/src/vector.rs index 3e39f1d623c..c1af4af5f57 100644 --- a/crates/core_simd/src/vector.rs +++ b/crates/core_simd/src/vector.rs @@ -135,22 +135,32 @@ where /// assert_eq!(v.as_array(), &[0, 1, 2, 3]); /// ``` pub const fn as_array(&self) -> &[T; LANES] { - &self.0 + // SAFETY: Transmuting between `Simd` and `[T; LANES]` + // is always valid and `Simd` never has a lower alignment + // than `[T; LANES]`. + unsafe { &*(self as *const Self as *const [T; LANES]) } } /// Returns a mutable array reference containing the entire SIMD vector. pub fn as_mut_array(&mut self) -> &mut [T; LANES] { - &mut self.0 + // SAFETY: Transmuting between `Simd` and `[T; LANES]` + // is always valid and `Simd` never has a lower alignment + // than `[T; LANES]`. + unsafe { &mut *(self as *mut Self as *mut [T; LANES]) } } /// Converts an array to a SIMD vector. pub const fn from_array(array: [T; LANES]) -> Self { - Self(array) + // SAFETY: Transmuting between `Simd` and `[T; LANES]` + // is always valid. + unsafe { core::mem::transmute_copy(&array) } } /// Converts a SIMD vector to an array. pub const fn to_array(self) -> [T; LANES] { - self.0 + // SAFETY: Transmuting between `Simd` and `[T; LANES]` + // is always valid. + unsafe { core::mem::transmute_copy(&self) } } /// Converts a slice to a SIMD vector containing `slice[..LANES]`. @@ -735,7 +745,7 @@ where { #[inline] fn as_ref(&self) -> &[T; LANES] { - &self.0 + self.as_array() } } @@ -746,7 +756,7 @@ where { #[inline] fn as_mut(&mut self) -> &mut [T; LANES] { - &mut self.0 + self.as_mut_array() } } @@ -758,7 +768,7 @@ where { #[inline] fn as_ref(&self) -> &[T] { - &self.0 + self.as_array() } } @@ -769,7 +779,7 @@ where { #[inline] fn as_mut(&mut self) -> &mut [T] { - &mut self.0 + self.as_mut_array() } } From 52833ccbe88ed98b73d0ccd7299f2a667439bb4b Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Sat, 22 Apr 2023 23:02:45 +0000 Subject: [PATCH 2/3] Add notes to avoid direct field accesses --- crates/core_simd/src/vector.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/core_simd/src/vector.rs b/crates/core_simd/src/vector.rs index c1af4af5f57..eee105ff5fc 100644 --- a/crates/core_simd/src/vector.rs +++ b/crates/core_simd/src/vector.rs @@ -76,6 +76,11 @@ use crate::simd::{ /// [`read`]: pointer::read /// [`write`]: pointer::write /// [as_simd]: slice::as_simd +// +// NOTE: Accessing the inner array directly in any way (e.g. by using the `.0` field syntax) or +// directly constructing an instance of the type (i.e. `let vector = Simd(array)`) should be +// avoided, as it will likely become illegal on `#[repr(simd)]` structs in the future. It also +// causes rustc to emit illegal LLVM IR in some cases. #[repr(simd)] pub struct Simd([T; LANES]) where @@ -138,6 +143,9 @@ where // SAFETY: Transmuting between `Simd` and `[T; LANES]` // is always valid and `Simd` never has a lower alignment // than `[T; LANES]`. + // + // NOTE: This deliberately doesn't just use `&self.0`, see the comment + // on the struct definition for details. unsafe { &*(self as *const Self as *const [T; LANES]) } } @@ -146,6 +154,9 @@ where // SAFETY: Transmuting between `Simd` and `[T; LANES]` // is always valid and `Simd` never has a lower alignment // than `[T; LANES]`. + // + // NOTE: This deliberately doesn't just use `&mut self.0`, see the comment + // on the struct definition for details. unsafe { &mut *(self as *mut Self as *mut [T; LANES]) } } @@ -153,6 +164,9 @@ where pub const fn from_array(array: [T; LANES]) -> Self { // SAFETY: Transmuting between `Simd` and `[T; LANES]` // is always valid. + // + // NOTE: This deliberately doesn't just use `Self(array)`, see the comment + // on the struct definition for details. unsafe { core::mem::transmute_copy(&array) } } @@ -160,6 +174,9 @@ where pub const fn to_array(self) -> [T; LANES] { // SAFETY: Transmuting between `Simd` and `[T; LANES]` // is always valid. + // + // NOTE: This deliberately doesn't just use `self.0`, see the comment + // on the struct definition for details. unsafe { core::mem::transmute_copy(&self) } } From f1b86baf8453733c72e196ce2c08b4d85e94d81a Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Sat, 22 Apr 2023 23:22:39 +0000 Subject: [PATCH 3/3] Use pointer reads for better codegen in debug mode --- crates/core_simd/src/lib.rs | 1 - crates/core_simd/src/vector.rs | 18 ++++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/crates/core_simd/src/lib.rs b/crates/core_simd/src/lib.rs index a372e2e40c4..e054d483ca5 100644 --- a/crates/core_simd/src/lib.rs +++ b/crates/core_simd/src/lib.rs @@ -2,7 +2,6 @@ #![feature( const_ptr_read, const_refs_to_cell, - const_transmute_copy, convert_float_to_int, decl_macro, intra_doc_pointers, diff --git a/crates/core_simd/src/vector.rs b/crates/core_simd/src/vector.rs index eee105ff5fc..a38d701588c 100644 --- a/crates/core_simd/src/vector.rs +++ b/crates/core_simd/src/vector.rs @@ -163,21 +163,31 @@ where /// Converts an array to a SIMD vector. pub const fn from_array(array: [T; LANES]) -> Self { // SAFETY: Transmuting between `Simd` and `[T; LANES]` - // is always valid. + // is always valid. We need to use `read_unaligned` here, since + // the array may have a lower alignment than the vector. + // + // FIXME: We currently use a pointer read instead of `transmute_copy` because + // it results in better codegen with optimizations disabled, but we should + // probably just use `transmute` once that works on const generic types. // // NOTE: This deliberately doesn't just use `Self(array)`, see the comment // on the struct definition for details. - unsafe { core::mem::transmute_copy(&array) } + unsafe { (&array as *const [T; LANES] as *const Self).read_unaligned() } } /// Converts a SIMD vector to an array. pub const fn to_array(self) -> [T; LANES] { // SAFETY: Transmuting between `Simd` and `[T; LANES]` - // is always valid. + // is always valid. No need to use `read_unaligned` here, since + // the vector never has a lower alignment than the array. + // + // FIXME: We currently use a pointer read instead of `transmute_copy` because + // it results in better codegen with optimizations disabled, but we should + // probably just use `transmute` once that works on const generic types. // // NOTE: This deliberately doesn't just use `self.0`, see the comment // on the struct definition for details. - unsafe { core::mem::transmute_copy(&self) } + unsafe { (&self as *const Self as *const [T; LANES]).read() } } /// Converts a slice to a SIMD vector containing `slice[..LANES]`.