Skip to content

Ensure the neon vector aggregates like float32x4x4_t are #[repr(C)] #1309

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

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Jun 12, 2022

These are C structs from arm_neon.h, so having them be #[repr(Rust)] is wrong. Also they are used by certain intrinsics, and I suspect if they had a weird layout, it would be very broken.

Moreover, it seems reasonable for someone to want to do things like transmute a float32x4x4_t into a [float32x4_t; 4] and such, which requires #[repr(C)].

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@Amanieu Amanieu merged commit c4d349c into rust-lang:master Jun 13, 2022
@infinity0
Copy link

@thomcc could this be related to rust-lang/rust#96983 ?

@thomcc
Copy link
Member Author

thomcc commented Jun 20, 2022

That certainly wouldn't make sense to me. In practice under default flags this doesn't change anything -- they already had the same layout that C would use, I think. But I don't know, I guess it's possible.

TBH, I'd be somewhat opposed to reverting this change either way, as it would defeat the point of having these types.

@thomcc thomcc deleted the reprc-aarch64-structs branch June 20, 2022 16:17
@infinity0
Copy link

That bug is for 1.59 which predates this PR, so I was wondering if backporting this PR to 1.59 would fix this issue for us in Debian. However this doesn't match your explanation either, so I'll have to continue investigating. Anyway, thanks for the comment.

@thomcc
Copy link
Member Author

thomcc commented Jun 20, 2022

Ah, hm, yeah, I don't think it would do that, but could be mistaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants