Closed ScottKane closed 1 month ago
Quat
itself isn't repr(simd)
, it's repr(transparent)
. The Quat
type has the same attributes and inner type as Vec3A
and Vec4
, are they not also causing issues?
Quat
is only repr(simd)
when targeting spirv for rust-gpu.
Sorry yes the inner __m128
is the problem, this problem presented itself when passing bevy's built-in types over FFI (mainly Transform) which uses this Quat
for its rotation. I'm sure the other types using this would be problematic, they just haven't appeared yet.
Maybe the simplest solution is to have a wrapper type around the inner type shared by Quat
, Vec3A
and Vec4
that can be sent over FFI with a feature flag.
Happy to open a PR, just let me know if you have any preferences.
I've made all of the types that wrap simd types #[repr(transparent)]
, kind of on the premise that it might let the compiler do smarter things, but I never really looked to see if it did make a difference perf wise. If you change types like Quat from #[repr(C)]
does that fix this for you?
I would most likely be OK with making that change. Currently glam uses #[repr(transparent)]
for simd types and #[repr(C)]
when simd is not available, which might be a bit of a weird inconsistency anyway.
If this change is made in glam would the bevy change still require a repr_c
feature?
The repr_c
feature would still need to exist in bevy unless it was decided to make types always #[repr(C)]
by default but as far as I can tell, most of the inner types used from libraries are #[repr(C)]
anyways, it's just that the bevy types built on top aren't.
I'm not sure if making Quat
#[repr(C)]
would solve the issue, you would have to slot in a #[repr(C)]
compatible type for the inner __m128
etc because it isn't #[repr(C)]
compatible.
So if you made Quat
#[repr(C)]
and passed it over FFI you would have values aligned differently, at least that is what happens when you pass a bevy Transform
over. My bevy PR converted Transform
to #[repr(C)]
which has a #[repr(transparent)]
Quat
containing a #[repr(simd)]
__m128
.
The Transform.translation
would come over fine but due to the differing alignment the f32 field values in Transform.rotation
and Transform.scale
would get swapped around. By changing the inner type of the Quat
from __m128
to some #[repr(C)]
struct with x, y, z, w
, the issue is resolved.
Not sure of the perf implications here which is why I would put it behind some feature flag. I'm assuming #[repr(simd)]
are a fair bit faster.
Making Quat
#[repr(C)]
is all that I can do in glam short of disabling SIMD entirely - not something I would be willing to do. You can compile glam without SIMD support with the scalar-math
feature.
So specifying #[repr(C)]
on Quat
is not sufficient for your use case if the internal type is __m128
? It's a bit surprising. It is also possible to specify alignment with #[repr(align(16))]
although the Quat
type is already be aligned to the inner __m128
type so it should not be necessary to do that..
The inner __m128
type is a Rust core library type , not something I have control over. #[repr(simd)]
is not an attribute that glam uses, it's a Rust internal attribute which tells LLVM to use a vector register for the given type.
Yeah afaik it's currently undefined behaviour based on how the ABI is implemented per compiler per architecture. I hadn't seen the scalar feature so that could be an option, just swap to that implementation.
I will close as there isn't much more that can be done on the glam side, thanks for the help.
@bitshifter I have switched to using scalar-math
and I'm now getting compilation errors where BVec4A
is used in bevy:
cargo build --features repr_c
Compiling bevy_reflect v0.15.0-dev (/hdd/code/rust/bevy/crates/bevy_reflect)
error[E0412]: cannot find type `BVec4A` in crate `glam`
--> crates/bevy_reflect/src/impls/glam.rs:335:29
|
335 | impl_reflect_value!(::glam::BVec4A(Debug, Default));
| ^^^^^^ help: a struct with a similar name exists: `BVec3A`
It seems to be set up to point to the scalar implementation so a bit confused :thinking:
https://github.com/bitshifter/glam-rs/blob/4488b238fdd924b1339f9364b695a9644e422cef/src/bool.rs#L90 https://github.com/bitshifter/glam-rs/blob/main/src/bool/scalar/bvec4a.rs
Am I being stupid?
Internally BVec4
is four bool
s versus BVec4A
which is a 128 bit vector register or when not using SIMD
, four u32
s. There's a bit of a performance penalty using BVec4A
with scalar-math
so BVec4
is used instead, this is aliased here https://github.com/bitshifter/glam-rs/blob/4488b238fdd924b1339f9364b695a9644e422cef/src/f32/scalar/vec4.rs#L3-L4.
It's a bit of an annoying API inconsistency, but I felt it was necessarily not to penalise people who wanted to use scalar-math
. So it may require a few other changes in bevy, I'm not sure.
I have been working on a PR for Bevy which allows their types to be passed over FFI using repr(C), however as the Quat implementation Bevy is using from glam is repr(simd) it is not compatible with C FFI (see https://github.com/rust-lang/rust/issues/53346). It was suggested to upstream changes here to reduce complexity in maintaining repr(C) wrapper types to mitigate this in Bevy.
Would you be opposed to a PR implementing a repr(C) version of Quat?
See this PR for current workaround - https://github.com/bevyengine/bevy/pull/14362