-
Notifications
You must be signed in to change notification settings - Fork 13k
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
rustc: Rewrite platform intrinsics crate #57048
rustc: Rewrite platform intrinsics crate #57048
Conversation
This commit rewrites (updates) the `rustc_platform_intrinsics` crate in this repository. This crate was previously procedurally generated from a number of JSON files and was intended to be the basis for how we bind SIMD in Rust. This crate is quite old at this point and is actually only used very little. The `stdsimd` crate and `std::arch` modules are the "new" way for stabilizing SIMD intrinsics. Despite this, however, the crate isn't useless! Most LLVM intrinsics are called directly in the `stdsimd` crate using the `link_llvm_intrinsics` feature, but not all intrinsics can be expressed directly in Rust. Instead some intrinsics, notably those that return LLVM aggregates, need to be compiler-defined to ensure they're declared with the correct signature. A good example of this is the x86 rdrand/rdseed family of LLVM intrinsics, all of which return an aggregate of some form. The stdsimd functions which provide these x86 intrinsics link to functions defined by this crate, so we need to keep those! Adding all that together, this commit performs the following tasks: * Deletes the now-outdated `src/etc/platform-intrinsics` infrastructure * Deletes all `src/librustc_platform_intrinsics/*` intrinsics that are otherwise compatible to be defined in Rust itself. This includes everything that doesn't deal with aggregates basically. Empty architecture files are removed. * The style of `src/librustc_platform_intrinsics/` was updated slightly to make it a bit easier to handwrite. The `x86` rdrand/rdseed intrinsics and some `aarch64` intrinsics which return aggregates have all survived to continue to be defined by this crate. Closes rust-lang#41885
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
5d75a46
to
42cd022
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
42cd022
to
19e73b3
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
We can definitely make something work out here with a special ABI like the |
All of these LLVM intrinsics return aggregates, so they need to be compiler defined!
19e73b3
to
54cfe35
Compare
@nagisa indeed! And whomever implements that will have the please of deleting this whole crate! |
r? @nagisa |
What LLVM version is this targeting? I think the used addcarry/subborrow signatures are only valid since LLVM 8, previously those used an out parameter rather than an aggregate return. It's generally okay to keep using old signatures on new LLVM versions, because LLVM will autoupgrade them. But of course, it doesn't work the other way around ^^ |
In general all |
@bors r+ Some back-story. This crate, I believe, was intended to become some sort of a public stable API for SIMD and other instructions. Since we now have decided on stdsimd approach, it seems fine to me to remove most of this crate. In fact, stdsimd could also replace uses of e.g. rdrand intrinsic with "unadjusted" (yes, it works, cc @gnzlbg) as well. Unless that happens, however, this crate cannot be removed entirely quite yet. |
📌 Commit 54cfe35 has been approved by |
Oh nice! If unadjusted works I'll update stdsimd to stop using platform-intrinsics and then I'll send a PR to completely delete this crate. Thanks for checking! |
Please ping @eddyb and @sunfish here. I can’t remember how the cretonne
integration was supposed to work but one option was to migrate stdsimd to
use this (there were other options as well).
About removing this, I don’t think it’s a great loss right now. Even if we
end up going back in this direction in the future, it might be worth it to
build something directly tailored towards mapping stdsimd to both llvm and
cretonne, or maybe we can just do the mapping in stdsimd itself somehow .
|
bjorn3/rustc_codegen_cranelift#171 is the conversation you mean @gnzlbg I think. |
As suggested [here]! [here]: rust-lang/rust#57048 (comment)
As suggested [here]! [here]: rust-lang/rust#57048 (comment)
As suggested [here]! [here]: rust-lang/rust#57048 (comment)
I've reopened a new version of this at #57416 |
This was originally attempted in rust-lang#57048 but it was realized that we could fully remove the crate via the `"unadjusted"` ABI on intrinsics. This means that all intrinsics in stdsimd are implemented directly against LLVM rather than using the abstraction layer provided here. That ends up meaning that this crate is no longer used at all. This crate developed long ago to implement the SIMD intrinsics, but we didn't end up using it in the long run. In that case let's remove it!
rustc: Remove platform intrinsics crate This was originally attempted in #57048 but it was realized that we could fully remove the crate via the `"unadjusted"` ABI on intrinsics. This means that all intrinsics in stdsimd are implemented directly against LLVM rather than using the abstraction layer provided here. That ends up meaning that this crate is no longer used at all. This crate developed long ago to implement the SIMD intrinsics, but we didn't end up using it in the long run. In that case let's remove it!
This was originally attempted in rust-lang#57048 but it was realized that we could fully remove the crate via the `"unadjusted"` ABI on intrinsics. This means that all intrinsics in stdsimd are implemented directly against LLVM rather than using the abstraction layer provided here. That ends up meaning that this crate is no longer used at all. This crate developed long ago to implement the SIMD intrinsics, but we didn't end up using it in the long run. In that case let's remove it!
This commit rewrites (updates) the
rustc_platform_intrinsics
cratein this repository. This crate was previously procedurally generated
from a number of JSON files and was intended to be the basis for how we
bind SIMD in Rust. This crate is quite old at this point and is actually
only used very little. The
stdsimd
crate andstd::arch
modules arethe "new" way for stabilizing SIMD intrinsics.
Despite this, however, the crate isn't useless! Most LLVM intrinsics are
called directly in the
stdsimd
crate using thelink_llvm_intrinsics
feature, but not all intrinsics can be expressed directly in Rust.
Instead some intrinsics, notably those that return LLVM aggregates, need
to be compiler-defined to ensure they're declared with the correct
signature.
A good example of this is the x86 rdrand/rdseed family of LLVM
intrinsics, all of which return an aggregate of some form. The stdsimd
functions which provide these x86 intrinsics link to functions defined
by this crate, so we need to keep those!
Adding all that together, this commit performs the following tasks:
src/etc/platform-intrinsics
infrastructuresrc/librustc_platform_intrinsics/*
intrinsics that areotherwise compatible to be defined in Rust itself. This includes
everything that doesn't deal with aggregates basically. Empty
architecture files are removed.
src/librustc_platform_intrinsics/
was updated slightlyto make it a bit easier to handwrite. The
x86
rdrand/rdseedintrinsics and some
aarch64
intrinsics which return aggregates haveall survived to continue to be defined by this crate.
Closes #41885