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

Add missing #[target_feature(enable = "simd128")] #1609

Closed

Conversation

daxpedda
Copy link
Contributor

I believe that adding these was missed in #874 and adding this now should not be a breaking change.

Cc @alexcrichton.

@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@@ -33,6 +33,7 @@ types! {
/// type in WebAssembly. Operations on `v128` can only be performed with the
/// functions in this module.
// N.B., internals here are arbitrary.
#[target_feature(enable = "simd128")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this doesn't work on types.
I guess we also don't want to add cfg(target_feature) here because otherwise using it would require build-std.

@daxpedda daxpedda force-pushed the enable-target-feature-simd128 branch from 7536645 to 6abab4e Compare July 11, 2024 09:35
@Amanieu
Copy link
Member

Amanieu commented Jul 11, 2024

I believe these were intentionally omitted to allow using SIMD type constructors in a const context.

@Amanieu
Copy link
Member

Amanieu commented Jul 11, 2024

And in the end, just using the SIMD types doesn't require the feature to be available, the compiler will just fall back to treating it as an array of scalars.

@alexcrichton
Copy link
Member

Ah yes at the time this wasn't possible due to the usage of const, but if the compiler now allows #[target_feature] on const functions I think this is ok to land. I also agree with @Amanieu though that these should be optional and not required.

@daxpedda
Copy link
Contributor Author

I also agree with @Amanieu though that these should be optional and not required.

Does #[target_feature(enable = "simd128")] cause LLVM to generate SIMD128 instructions here even without using -Ctarget-feature=simd128?

@alexcrichton
Copy link
Member

Right yeah, the -C flag applies to all functions within a crate and #[target_feature] only applies to a single function, and LLVM only needs it enabled for the function-at-hand.

@daxpedda
Copy link
Contributor Author

I see, thanks!
Will go ahead and close this then.

@daxpedda daxpedda closed this Jul 12, 2024
alexcrichton added a commit to alexcrichton/stdarch that referenced this pull request Dec 2, 2024
This is a resubmission of rust-lang#1609 which was ruled optional but not
necessary at the time but it's now necessary. These weren't originally
applied as they weren't allowed in a `const` context but that's no
longer applicable. At the same time though be sure to add some small
tests to ensure that these intrinsics can be used in a `const` context.
github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2024
This is a resubmission of #1609 which was ruled optional but not
necessary at the time but it's now necessary. These weren't originally
applied as they weren't allowed in a `const` context but that's no
longer applicable. At the same time though be sure to add some small
tests to ensure that these intrinsics can be used in a `const` context.
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