non-power-of-2 Simd types have wrong size

[programmerjake]

I tried this code:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=f4ee5c373508bcb37be6cda3bb627b87

#![feature(repr_simd)]
use core::alloc::Layout;
#[repr(simd)]
struct Simd<T, const N: usize>(pub [T; N]);

fn main() {
    assert_eq!(
        Layout::new::<Simd<i8, 3>>(),
        Layout::from_size_align(3, 1).unwrap()
    );
}

I expected to see this happen: run with no errors because Simd<T, N> types should be the same size as the corresponding array type [T; N] (something we unofficially agreed on iirc, but is not formally decided)

Instead, this happened:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Layout { size: 4, align: 4 (1 << 2) }`,
 right: `Layout { size: 3, align: 1 (1 << 0) }`', src/main.rs:7:5

Meta

rustc --version --verbose:

<current nightly as of issue creation -- probably 2022-11-29 nightly>

crate version in Cargo.toml:
N/A

[programmerjake]

see also #63

[programmerjake]

see some previous discussion: https://rust-lang.zulipchat.com/#narrow/stream/257879-project-portable-simd/topic/Bytemuckable.20Simd/near/311734698

[programmerjake]

imho we should define layout such that sizeof(Simd<T, N>) == sizeof([T; N]) and alignof(Simd<T, N>) == gcd(alignof(T), llvm_alignof(Simd<T, N>)) >= alignof(T) (edit: wrong. see: #319 (comment))
or, if we don't want to depend on llvm, we could pick the largest power of two <= sizeof([T; N]) such that no padding is required.

[programmerjake]

Note the changes to vector layout by @RalfJung in rust-lang/rust@891a4da are the exact opposite of what we probably want: that commit adds padding until size is divisible by alignment, what we probably want is to decrease alignment enough that we can enforce vectors are the exact same size as the corresponding array (usually means no padding, there could still be padding if the element type contains padding, portable-simd will presumably enforce that those element types aren't allowed but repr(simd) might allow them in the future)

[programmerjake]

imho we should define layout such that sizeof(Simd<T, N>) == sizeof([T; N]) and alignof(Simd<T, N>) == gcd(alignof(T), llvm_alignof(Simd<T, N>)) >= alignof(T)

now that I think about some more, gcd of aligns is wrong, instead we probably want alignof(Simd<T, N>) == gcd(sizeof([T; N]), llvm_alignof(Simd<T, N>)) >= alignof(T)

[calebzulawski]

I wonder, what's the motivation for that commit? If it's absolutely necessary for some other reason, we could pretty easily add a new repr for portable SIMD.

[programmerjake]

I wonder, what's the motivation for that commit? If it's absolutely necessary for some other reason, we could pretty easily add a new repr for portable SIMD.

I would assume it's for enforcing rust's rule that every type must have its size be divisible by its alignment. llvm doesn't enforce that rule, so relying on llvm to calculate layout for vectors can violate that rule.

[RalfJung]

My commit does not change layout at all, it just changes the layout consistency check, assuming that the existing layout code is right. If the existing layout code is buggy then yeah I also ported the bug to the layout checker now. Feel free to fix both at the same time. :) The important part is the the size and align of the type must be uniquely determined by the size and align of the vector elements and the number of elements.

[RalfJung]

The actual layout computation is here. And yes size.align_to(align.abi) is needed because size must be a multiple of align in Rust. So if you want 3-element SIMD vectors to not have trailing padding, then vector_align needs to be adjusted to return a lower alignment for these types.

This behavior of rustc (to round up vector sizes to the next power of two) has existed at least since 2016.

Cc @eddyb

[eddyb]

To be clear, this is not rustc-specific behavior but LLVM behavior - the reason we have to reuse the datalayout string is that mismatches between rustc and LLVM could lead to miscompilations if we have to use APIs where we assume LLVM is working with the same algorithms and configuration we are.

IIRC the logic is roughly:

• datalayout string can contain an alignment for some exact vector size
• otherwise, the default is "natural alignment", i.e. the alignment is the same as the size (except rounded up to a power of two, if not already? I forget if it's meant to be stricter or not)

[eddyb]

Reading more of the discussion above, you really cannot talk about llvm_alignof(Simd<T, N>) as if it's different than the Rust alignment.

At the point where you start diverging in alignments from LLVM, you can no longer use primitive vector types and have to wrap them in packed structs to decrease alignment... except the vector is already rounded up to a "plausible hardware size" by that point so the packed struct can no longer decrease the size!

You would probably need to "just" not use non-pow2 LLVM vector types, i.e. do not have Abi::Vector for such malformed #[repr(simd)] types. Or maybe they can still be used when by-value but great care must be taken around loading and storing - you effectively would need to generate e.g. 3 loads/stores to read/write to/from <3 x T> values (or weird FCA loads/stores of [3 x T] which LLVM would desugar).

Frankly, after what I've seen from GLSL (which has e.g. size=12 align=16 types that "eat" padding when used as fields!), my humble suggestion might be to ban these types instead of giving them magical semantics, since you generally can't "take back" the magic, even if you manage to make it work initially.

[programmerjake]

what i'm envisioning is using npot vectors by value (not in memory) just uses usual llvm semantics, whereas when loading/storing llvm allows specifying the alignment, which is where we would override it to use the rust abi alignment.
if llvm decides to store vectors in temporary memory variables, it's free to use whatever layout it pleases, since llvm is allowed to modify code using the as-if rule.

note that npot vectors are natively supported on some ISAs (e.g. SVE, RVV, and SimpleV)

[programmerjake]

for llvm npot vector loads/stores, afaict from reading the llvm language reference llvm guarantees that they will not load/store padding if the elements are an integer byte size (so not i4 since that's 1/2 byte) and have no padding -- since it says here vector load/stores act the same as array load/stores when the element type is an integer byte size. so, no, we won't need multiple llvm load/store instructions to load/store npot vectors.

[programmerjake]

Frankly, after what I've seen from GLSL (which has e.g. size=12 align=16 types that "eat" padding when used as fields!), my humble suggestion might be to ban these types instead of giving them magical semantics, since you generally can't "take back" the magic, even if you manage to make it work initially.

we shouldn't run into that particular problem since the proposed layouts will always have size % align == 0 like any other rust type, they're basically arrays' layouts but with the align increased as close as possible to llvm's preferred align without introducing padding or violating size % align == 0