Skip to content
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

move to array simd #1422

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

Ezrashaw
Copy link

As part of compiler-team/MCP#621, this repo must be updated to use the new-style array fields instead.

Note that a very recent nightly is needed which fixed a bug where inline assembly could not be used with the array style.

Note also that this cannot be pulled to rust-lang/rust until beta updates with the bugfix.

cc @scottmcm

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

@Ezrashaw Ezrashaw marked this pull request as draft April 25, 2023 09:50
@Ezrashaw
Copy link
Author

Gah, different architectures cause pain here.

pub(crate) const fn new($($elem_name: $elem_ty),*) -> Self {
$id($($elem_name),*)
pub(crate) const fn new(val: [$ety; $ecount]) -> Self {
$id(val)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've heard, the plan is to make any direct field accesses (including calling the constructor like this) illegal on #[repr(simd)] types and only allow creation of the type and access to the inner array through transmute or pointer operations, so I think it might make sense to make the struct field private and to do something similar to rust-lang/portable-simd#339 or rust-lang/portable-simd#342 in new and splat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd also be fine for that to be a separate PR, if you prefer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

direct field accesses (including calling the constructor like this) illegal

Emphasis on plan. This isn't happening with the current MCP, we are just removing the field-style #[repr(simd)]. In the longer term, I think scottmcm and other people have plans for that but not immediately.


#[allow(clippy::use_self)]
impl $id {
#[inline(always)]
pub(crate) const fn new($($elem_name: $elem_ty),*) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is less ergonomic, shall we consider adding From implementation to support both tuple and array and keep new as it is now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this was always pub(crate), so users couldn't call it anyway.

They can only be created using intrinsics (or memory stuff like transmutes) in stable today: https://doc.rust-lang.org/core/arch/x86/struct.__m128i.html.

(The portable-simd types do have nice safe public constructors. But these arch ones intentionally don't.)

@scottmcm
Copy link
Member

scottmcm commented Jul 1, 2024

@Ezrashaw With rust-lang/rust#110672 merged a while ago, are we good to go on this again?

@Ezrashaw
Copy link
Author

Ezrashaw commented Jul 1, 2024

@Ezrashaw With rust-lang/rust#110672 merged a while ago, are we good to go on this again?

Yeah, the technical side of this pull request should be good to go. Personally, I haven't been contributing to Rust recently, so if someone wants to take this up, then free totally free. However, I'll try to find some time to complete this PR in maybe a week or so.

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.

6 participants