-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Stabilize keylocker #140766
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
base: master
Are you sure you want to change the base?
Stabilize keylocker #140766
Conversation
… detection for keylocker
|
Stabilizing target features are somewhat minimal as far as stabilizations go, but still, this is going to need something more in the way of a stabilization report in the description of this PR. We need some narrative about what we're stabilizing here and a review of the evidence about why it's OK for us to stabilize this now. Have a look at our new template for this in rust-lang/rustc-dev-guide#2219. Please think too about who else in the Project has worked on this or related things and who might therefore be interested in this, and please ping those people here. This will also need a PR to the Reference documenting the change, and that should be linked from the stabilization report as well. @rustbot labels +S-waiting-on-documentation Please renominate when the stabilization report is available. |
@rustbot label I-lang-nominated |
url = https://github.com/rust-lang/stdarch.git | ||
url = https://github.com/sayantn/stdarch.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving a note so we remember to pull out this "do not merge" commit.
@@ -257,6 +257,8 @@ declare_features! ( | |||
/// Allows some increased flexibility in the name resolution rules, | |||
/// especially around globs and shadowing (RFC 1560). | |||
(accepted, item_like_imports, "1.15.0", Some(35120)), | |||
// Allows using the `kl` and `widekl` target features and the associated intrinsics | |||
(accepted, keylocker_x86, "1.86.0", Some(134813)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(accepted, keylocker_x86, "1.86.0", Some(134813)), | |
(accepted, keylocker_x86, "CURRENT_RUSTC_VERSION", Some(134813)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, forgot changing it, will update
@sayantn, are these |
These names are used by both gcc and llvm, but Intel uses the names |
Beyond ABI, is there any possibility for trouble when mixing code built with and without these target features? |
The only job of these target features is enabling the use of those 11 intrinsics, or equivalently, 11 instructions. They don't affect normal codegen in any way, so I don't think it will be problematic mixing code built with and without them. The only nontrivial thing I can think of about these is that the |
It looks like the kernel uses
This follows the nomenclature from a project for exhaustively documenting and tracking this CPUID data: In comparing it with the features that we support, most but not all of these match. Here's the LLVM list: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86.td
Yes, I suppose I'm happy enough to follow LLVM here. |
This sounds OK to me, so let's propose to do it. @rfcbot fcp merge |
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. 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! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
This PR stabilizes the feature flag
keylocker_x86
(tracking issue #134813).Public API
The 2
x86
target featureskl
andwidekl
, and the associated intrinsics in stdarch.These target features are very specialized, and are only used to signal the presence of the corresponding CPU instruction. They don't have any nontrivial interaction with the ABI (contrary to something like AVX), and serve the only purpose of enabling 11 stdarch intrinsics, all of which have been implemented and propagated to rustc via a stdarch submodule update.
Also, these were added way back in LLVM12, and as the minimum LLVM required for rustc is LLVM19, we are safe in that front too!
Associated PRs
kl
andwidekl
target features, and the feature gate #134814kl
andwidekl
) intrinsics and runtime feature detection stdarch#1706As all of the required tasks have been done (adding the target features to rustc, implementing their runtime detection in std_detect and implementing the associated intrinsics in core_arch), these target features can be stabilized now.
cc @rust-lang/lang
cc @rust-lang/libs-api for the intrinsics and runtime detection
I don't think anyone else worked on this feature, so no one else to ping, maybe cc @Amanieu. I will send the reference pr soon.
Do not merge now, contains a temporary fix to make the tests pass until the stdarch stabilization PR is merged.