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

Stabilise is_aarch64_feature_detected! under simd_aarch64 feature #90271

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

adamgemmell
Copy link
Contributor

Initial implementation, looking for feedback on the approach here. #86941

One point I noticed was that I haven't seen different "since" versions for the same feature - does this mean that other features can't be added to to the simd_aarch64 feature once this is in stable? If so it might need a more specific name.

r? @Amanieu

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2021
@adamgemmell
Copy link
Contributor Author

CI should fail until the stdarch PR is committed and updated here

@Amanieu
Copy link
Member

Amanieu commented Oct 25, 2021

We can have different since versions for the same feature. So adding new target features in the future is not a problem.

@Amanieu Amanieu added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2021
@bors
Copy link
Contributor

bors commented Jan 5, 2022

☔ The latest upstream changes (presumably #92587) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 5, 2022
@adamgemmell adamgemmell force-pushed the dev/feat-detect-stabilise branch from 5fdd506 to d9e776b Compare February 4, 2022 14:59
@@ -6,7 +6,6 @@
all(target_arch = "aarch64", any(target_os = "linux", target_os = "android")),
all(target_arch = "powerpc", target_os = "linux"),
all(target_arch = "powerpc64", target_os = "linux"),
any(target_arch = "x86", target_arch = "x86_64"),
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed for aarch64 as well? You might need to put this behind cfg(bootstrap) so the stage0 compiler still accepts it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True - I mistakenly removed it when making pauth temporarily unstable again.

Copy link
Contributor Author

@adamgemmell adamgemmell Feb 10, 2022

Choose a reason for hiding this comment

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

Actually, it does need to remain there while pauth is unstable. I'll fix it up when rust-lang/stdarch#1259 is merged, and update the submodule at the same time.

@adamgemmell adamgemmell force-pushed the dev/feat-detect-stabilise branch from d9e776b to 93b5bfb Compare February 10, 2022 15:39
@adamgemmell adamgemmell marked this pull request as ready for review February 10, 2022 15:39
@Amanieu
Copy link
Member

Amanieu commented Feb 10, 2022

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Feb 10, 2022

📌 Commit 93b5bfb has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2022
@bors
Copy link
Contributor

bors commented Feb 11, 2022

⌛ Testing commit 93b5bfb with merge e789f3a...

@bors
Copy link
Contributor

bors commented Feb 11, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing e789f3a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 11, 2022
@bors bors merged commit e789f3a into rust-lang:master Feb 11, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 11, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e789f3a): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@ehuss
Copy link
Contributor

ehuss commented Feb 13, 2022

Just a heads up, the stabilization machinery may cause confusion here. The docs say this is still unstable: https://doc.rust-lang.org/nightly/std/arch/macro.is_aarch64_feature_detected.html

This is because the https://doc.rust-lang.org/ docs are built for x86_64. For some reason, using the macro from an incompatible target raises an unstable error. For example, using is_aarch64_feature_detected on x86_64 will display:

error[E0658]: use of unstable library feature 'stdsimd'
 --> src/main.rs:2:8
  |
2 |     if std::arch::is_aarch64_feature_detected!("paca") {
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #27731 <https://github.com/rust-lang/rust/issues/27731> for more information
  = help: add `#![feature(stdsimd)]` to the crate attributes to enable

error:
               is_aarch64_feature_detected can only be used on AArch64 targets.
               You can prevent it from being used in other architectures by
               guarding it behind a cfg(target_arch) as follows:

                   #[cfg(target_arch = "aarch64")] {
                       if is_aarch64_feature_detected(...) { ... }
                   }

 --> src/main.rs:2:8
  |
2 |     if std::arch::is_aarch64_feature_detected!("paca") {
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `std::arch::is_aarch64_feature_detected` (in Nightly builds, run with -Z macro-backtrace for more info)

Is that intentional?

@adamgemmell adamgemmell deleted the dev/feat-detect-stabilise branch February 14, 2022 12:09
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 17, 2022
Fix documentation for is_X_feature_detected!

These are now properly documented for all architectures and the
stability attributes in the docs are now correctly displayed.

This addresses this comment by `@ehuss:` rust-lang#90271 (comment)

cc `@adamgemmell`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2022
Fix documentation for is_X_feature_detected!

These are now properly documented for all architectures and the
stability attributes in the docs are now correctly displayed.

This addresses this comment by `@ehuss:` rust-lang#90271 (comment)

cc `@adamgemmell`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants