Stabilise std::is_aarch64_feature_detected #86941
Comments
jonas-schievink
added
A-simd
…=Amanieu Remove the aarch64 `crypto` target_feature The subfeatures `aes` or `sha2` should be used instead. This can't yet be done for ARM targets as some LLVM intrinsics still require `crypto`. Also update the runtime feature detection tests in `library/std` to mirror the updates in `stdarch`. This also helps rust-lang#86941 r? `@Amanieu`
…=Amanieu Remove the aarch64 `crypto` target_feature The subfeatures `aes` or `sha2` should be used instead. This can't yet be done for ARM targets as some LLVM intrinsics still require `crypto`. Also update the runtime feature detection tests in `library/std` to mirror the updates in `stdarch`. This also helps rust-lang#86941 r? ``@Amanieu``
|
One possible issue I've noticed is that I can't see a way to view https://doc.rust-lang.org/std/macro.is_aarch64_feature_detected.html under a different platform like you can with other crates on docs.rs. Until we can I'm not sure it's possible to make the documentation for the macro easily visible. |
|
The documentation on doc.rust-lang.org is for all platforms at the same time. |
|
The issue is there's different output for multiple different platforms. I'll try removing the doc comment on the error macro in there to see if that helps. It's not useful on the docs page anyway and should just be a normal comment |
|
Is the general ARM one different enough to not just stabilize it at the same time as well? |
|
@rfcbot merge |
Amanieu
added
the
T-libs-api
|
@rfcbot merge |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
|
Have the unresolved questions here been addressed? Also, are we stabilizing any corresponding |
One issue is that we can't remove the I don't see a problem with stabilising the other ARM features though (this seems to be an option based on how the macro is written). Just that no-one has asked for it yet. I'm not sure how well it works on FreeBSD, nor if there's any more ARMv8 features that affect aarch32 execution (though neither of these arguably block stabilisation). |
Amanieu seems happy with Personally I'm happy keeping PAuth as one feature as LLVM does as it would then match target_features.
These should be mostly up to date. Since the minimum LLVM version has moved up quickly I reckon a few could be uncommented - such as PAuth |
|
To clarify, we are only stabilizing feature detection, not the stdarch intrinsics or |
|
@rfcbot concern move to a module? Should we move the macro out of the root of |
|
The (stable) x86/x86_64 variant is in the crate root too. Moving only the aarch64 variant would be confusing. |
|
After some internal discussion my opinion on splitting Would it make sense to split them preemptively for runtime feature detection? Or would the above scenario be ok? |
|
I would rather preemptively split the features. We also need to figure out how the two features map to LLVM's pointer authentication support. |
|
PAuth demonstrates some rather interesting properties that could apply to other features too, so it's worth examining, and perhaps using this as a model for other cases that arise. Notably:
A likely example application could involve the compiler using So, as a general policy, at least for runtime detection, I would argue for splitting features down to the point that they're unlikely to be split further in the future, as we've done here. A useful guide for is "unlikely to be split further" is the hardware ID registers, and those are what we used for VIXL. Most Linux CPU features map 1:1 onto minimum values of fields in those registers. The compile-time detection — not strictly this issue but related — is made complicated by interactions with LLVM. As a user, I would prefer those to be split in the same way, but I don't fully understand the interactions with LLVM, so it may not be possible. I know that Rust has a mapping function so we can deal with renamings, but dealing with combinations may be trickier. |
|
I agree that we should be consistent in locating this; I don't think it does any harm in the root, since it has a sufficiently specific name that it won't conflict with anything. |
|
I checked LLVM's code: the I agree that we should try to have fine-grained features where possible. The main issue with exposing finer-grained features than LLVM is that there may not be a precise way to map |
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
rfcbot
added
to-announce
|
Sorry for the silence on this issue for a while. The implementations should be mostly up to date now. I'll summarise my open PRs for this in one place: Runtime detection macro:
Target feature:
See also #90620 for the |
…h, r=Amanieu Split `pauth` target feature Per discussion on rust-lang#86941 we'd like to split `pauth` into `paca` and `pacg` in order to better support possible future environments that only have the keys available for address or generic authentication. At the moment LLVM has the one `pauth` target_feature while Linux presents separate `paca` and `pacg` flags for feature detection. Because the use of [target_feature](https://rust-lang.github.io/rfcs/2045-target-feature.html) will "allow the compiler to generate code under the assumption that this code will only be reached in hosts that support the feature", it does not make sense to simply translate `paca` into the LLVM feature `pauth`, as it will generate code as if `pacg` is available. To accommodate this we error if only one of the two features is present. If LLVM splits them in the future we can remove this restriction without making a breaking change. r? `@Amanieu`
…h, r=Amanieu Split `pauth` target feature Per discussion on rust-lang#86941 we'd like to split `pauth` into `paca` and `pacg` in order to better support possible future environments that only have the keys available for address or generic authentication. At the moment LLVM has the one `pauth` target_feature while Linux presents separate `paca` and `pacg` flags for feature detection. Because the use of [target_feature](https://rust-lang.github.io/rfcs/2045-target-feature.html) will "allow the compiler to generate code under the assumption that this code will only be reached in hosts that support the feature", it does not make sense to simply translate `paca` into the LLVM feature `pauth`, as it will generate code as if `pacg` is available. To accommodate this we error if only one of the two features is present. If LLVM splits them in the future we can remove this restriction without making a breaking change. r? ``@Amanieu``
…, r=Amanieu Stabilise `is_aarch64_feature_detected!` under `simd_aarch64` feature Initial implementation, looking for feedback on the approach here. rust-lang#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`
…h, r=Amanieu Split `pauth` target feature Per discussion on rust-lang#86941 we'd like to split `pauth` into `paca` and `pacg` in order to better support possible future environments that only have the keys available for address or generic authentication. At the moment LLVM has the one `pauth` target_feature while Linux presents separate `paca` and `pacg` flags for feature detection. Because the use of [target_feature](https://rust-lang.github.io/rfcs/2045-target-feature.html) will "allow the compiler to generate code under the assumption that this code will only be reached in hosts that support the feature", it does not make sense to simply translate `paca` into the LLVM feature `pauth`, as it will generate code as if `pacg` is available. To accommodate this we error if only one of the two features is present. If LLVM splits them in the future we can remove this restriction without making a breaking change. r? ```@Amanieu```
|
Is there anything else needed for this issue, or can it be closed? |
|
#90271 (comment) @ehuss continuing discussion here. Looks like the error macro (in The same is also true for the x86 version - so you get an unstable warning when using Both should clearly be fixed I think - I'll have a patch up shortly. |
|
@ehuss also mentioned that the docs still show the error_macro docs for none-x86 platforms. Yet to find the reason but everything's fine with |
|
Maybe some issue due to std_detect being a separate crate from std which causes |
|
When a dependency is built for rustdoc, |
|
That makes sense rust-lang/cargo#8811 So, with beta cutoff next week, we could:
The third option seems to be more invasive so I'm going to go with the second for now until someone says otherwise. |
Cutoff is Friday morning this week.
It does not. Adding comments to the error macros might help a little, but I imagine will still show up as unstable so it might still be confusing. Unfortunately I don't have much time to help here. It's a bit of a thorny issue that can be tricky to fix. |
|
I opened rust-lang/stdarch#1283 which should fix this. |
workingjubilee
added
O-AArch64
Fold aarch64 feature +fp into +neon Arm's FEAT_FP and Feat_AdvSIMD describe the same thing on AArch64: The Neon unit, which handles both floating point and SIMD instructions. Moreover, a configuration for AArch64 must include both or neither. Arm says "entirely proprietary" toolchains may omit floating point: https://developer.arm.com/documentation/102374/0101/Data-processing---floating-point In the Programmer's Guide for Armv8-A, Arm says AArch64 can have both FP and Neon or neither in custom implementations: https://developer.arm.com/documentation/den0024/a/AArch64-Floating-point-and-NEON In "Bare metal boot code for Armv8-A", enabling Neon and FP is just disabling the same trap flag: https://developer.arm.com/documentation/dai0527/a In an unlikely future where "Neon and FP" become unrelated, we can add "[+-]fp" as its own feature flag. Until then, we can simplify programming with Rust on AArch64 by folding both into "[+-]neon", which is valid as it supersets both. "[+-]neon" is retained for niche uses such as firmware, kernels, "I just hate floats", and so on. I am... pretty sure no one is relying on this. An argument could be made that, as we are not an "entirely proprietary" toolchain, we should not support AArch64 without floats at all. I think that's a bit excessive. However, I want to recognize the intent: programming for AArch64 should be simplified where possible. For x86-64, programmers regularly set up illegal feature configurations because it's hard to understand them, see rust-lang#89586. And per the above notes, plus the discussion in rust-lang#86941, there should be no real use cases for leaving these features split: the two should in fact always go together. Fixes rust-lang#95002. Fixes rust-lang#95122.
…sa,Amanieu Fold aarch64 feature +fp into +neon Arm's FEAT_FP and Feat_AdvSIMD describe the same thing on AArch64: The Neon unit, which handles both floating point and SIMD instructions. Moreover, a configuration for AArch64 must include both or neither. Arm says "entirely proprietary" toolchains may omit floating point: https://developer.arm.com/documentation/102374/0101/Data-processing---floating-point In the Programmer's Guide for Armv8-A, Arm says AArch64 can have both FP and Neon or neither in custom implementations: https://developer.arm.com/documentation/den0024/a/AArch64-Floating-point-and-NEON In "Bare metal boot code for Armv8-A", enabling Neon and FP is just disabling the same trap flag: https://developer.arm.com/documentation/dai0527/a In an unlikely future where "Neon and FP" become unrelated, we can add "[+-]fp" as its own feature flag. Until then, we can simplify programming with Rust on AArch64 by folding both into "[+-]neon", which is valid as it supersets both. "[+-]neon" is retained for niche uses such as firmware, kernels, "I just hate floats", and so on. I am... pretty sure no one is relying on this. An argument could be made that, as we are not an "entirely proprietary" toolchain, we should not support AArch64 without floats at all. I think that's a bit excessive. However, I want to recognize the intent: programming for AArch64 should be simplified where possible. For x86-64, programmers regularly set up illegal feature configurations because it's hard to understand them, see rust-lang#89586. And per the above notes, plus the discussion in rust-lang#86941, there should be no real use cases for leaving these features split: the two should in fact always go together. - Fixes rust-lang#95002. - Fixes rust-lang#95064. - Fixes rust-lang#95122.
|
Following up on this: what are the remaining blockers to merge this? |
|
I believe all of the concerns have been addressed so this is just waiting on a stabilization PR. |
|
https://doc.rust-lang.org/std/arch/macro.is_aarch64_feature_detected.html I stabilised this back in 1.60 https://blog.rust-lang.org/2022/04/07/Rust-1.60.0.html Likewise with aarch64_target_feature (#90620), apologies for not closing the issues. |
This issue constitutes the stabilisation report as per the rustc dev guide
Summary
I'm hoping to stabilise std::is_aarch64_feature_detected which allows for runtime feature detection on aarch64 platforms. It was recently updated to include all features supported by both linux and LLVM 12, though the detection for some features isn't on non-linux platforms yet.
Documentation
The documentation for it is in this file. There's doc comments for individual features in the macro but I don't see these exposed anywhere, just on a hidden enum. Perhaps this could be improved. I've also got a small patch to mention it in the Rust Reference here.
Tests
There are tests for it in stdarch as well as rust. These are currently all up to date.
Unresolved questions
The only backwards-compatible question left to resolve is whether to split
pauthintopaca(address authentication) andpacg(generic authentication) as linux does. The current behaviour (together) matches LLVM (and the ARMv8-A feature FEAT_PAuth).The x86 macro was stabilised under the
simd_x86feature - does this mean I should create a newsimd_aarch64feature to stabilise this and other aarch64 stdsimd work under?See also:
#48556 for the tracking issue for the stdsimd feature.
#90620 for stabilising the
target_featureattribute on aarch64 platforms.The text was updated successfully, but these errors were encountered: