-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Implement repr(packed) for repr(simd) #117116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -435,7 +435,21 @@ fn layout_of_uncached<'tcx>( | |
.size | ||
.checked_mul(e_len, dl) | ||
.ok_or_else(|| error(cx, LayoutError::SizeOverflow(ty)))?; | ||
let align = dl.vector_align(size); | ||
|
||
let (abi, align) = if def.repr().packed() && !e_len.is_power_of_two() { | ||
// Non-power-of-two vectors have padding up to the next power-of-two. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iirc LLVM doesn't guarantee vectors don't have more padding than just to the next power of two, e.g. padding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://llvm.org/docs/LangRef.html#vector-type seems to say that LLVM guarantees that vectors have no padding in their in memory representation? Padding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's only for load/store, LLVM vector types still have big enough alignment that Rust requires padding in many cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if it's guaranteed, but rustc does make the assumption (somewhere, I forget where) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, I meant rustc assumes that npot vectors round up to the next power of two. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I thought LLVM types didn't have any sort of intrinsic alignment? And even if they have a preferred alignment that's too big, can't you just always annotate load/store instructions with the smaller alignment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
they do have an intrinsic ABI alignment, it's used for default alignment of load/store/alloca when not explicitly specified and for stack spilling and a few other things There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
just found this (yeah, I know that's not what you meant, but I couldn't resist):
|
||
// If we're a packed repr, remove the padding while keeping the alignment as close | ||
// to a vector as possible. | ||
( | ||
Abi::Aggregate { sized: true }, | ||
AbiAndPrefAlign { | ||
abi: Align::max_for_offset(size), | ||
pref: dl.vector_align(size).pref, | ||
}, | ||
) | ||
} else { | ||
(Abi::Vector { element: e_abi, count: e_len }, dl.vector_align(size)) | ||
}; | ||
let size = size.align_to(align.abi); | ||
|
||
// Compute the placement of the vector fields: | ||
|
@@ -448,7 +462,7 @@ fn layout_of_uncached<'tcx>( | |
tcx.mk_layout(LayoutS { | ||
variants: Variants::Single { index: FIRST_VARIANT }, | ||
fields, | ||
abi: Abi::Vector { element: e_abi, count: e_len }, | ||
abi, | ||
largest_niche: e_ly.largest_niche, | ||
size, | ||
align, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
// run-pass | ||
|
||
#![feature(repr_simd, platform_intrinsics)] | ||
#![allow(non_camel_case_types)] | ||
|
||
#[repr(simd, packed)] | ||
struct Simd<T, const N: usize>([T; N]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From how What happens with Would be good to have the interaction of |
||
|
||
#[repr(simd)] | ||
struct FullSimd<T, const N: usize>([T; N]); | ||
|
||
fn check_size_align<T, const N: usize>() { | ||
use std::mem; | ||
assert_eq!(mem::size_of::<Simd<T, N>>(), mem::size_of::<[T; N]>()); | ||
assert_eq!(mem::size_of::<Simd<T, N>>() % mem::align_of::<Simd<T, N>>(), 0); | ||
} | ||
|
||
fn check_ty<T>() { | ||
check_size_align::<T, 1>(); | ||
check_size_align::<T, 2>(); | ||
check_size_align::<T, 3>(); | ||
check_size_align::<T, 4>(); | ||
check_size_align::<T, 8>(); | ||
check_size_align::<T, 9>(); | ||
check_size_align::<T, 15>(); | ||
} | ||
|
||
extern "platform-intrinsic" { | ||
fn simd_add<T>(a: T, b: T) -> T; | ||
} | ||
|
||
fn main() { | ||
check_ty::<u8>(); | ||
check_ty::<i16>(); | ||
check_ty::<u32>(); | ||
check_ty::<i64>(); | ||
check_ty::<usize>(); | ||
check_ty::<f32>(); | ||
check_ty::<f64>(); | ||
|
||
unsafe { | ||
// powers-of-two have no padding and work as usual | ||
let x: Simd<f64, 4> = | ||
simd_add(Simd::<f64, 4>([0., 1., 2., 3.]), Simd::<f64, 4>([2., 2., 2., 2.])); | ||
assert_eq!(std::mem::transmute::<_, [f64; 4]>(x), [2., 3., 4., 5.]); | ||
|
||
// non-powers-of-two have padding and need to be expanded to full vectors | ||
fn load<T, const N: usize>(v: Simd<T, N>) -> FullSimd<T, N> { | ||
unsafe { | ||
let mut tmp = core::mem::MaybeUninit::<FullSimd<T, N>>::uninit(); | ||
std::ptr::copy_nonoverlapping(&v as *const _, tmp.as_mut_ptr().cast(), 1); | ||
tmp.assume_init() | ||
} | ||
} | ||
let x: FullSimd<f64, 3> = | ||
simd_add(load(Simd::<f64, 3>([0., 1., 2.])), load(Simd::<f64, 3>([2., 2., 2.]))); | ||
assert_eq!(x.0, [2., 3., 4.]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems no longer accurate... maybe that got changed by #125311? Doing simd_mul etc on a packed SIMD type works just fine now. Even in debug builds this generates the code one would hope for.