-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
rust_for_linux: -Zregparm=<N> commandline flag for X86 (#116972) #130432
Conversation
This comment has been minimized.
This comment has been minimized.
bb7d64f
to
cfed516
Compare
This comment has been minimized.
This comment has been minimized.
cfed516
to
d93f345
Compare
This comment has been minimized.
This comment has been minimized.
d93f345
to
c937a70
Compare
This comment has been minimized.
This comment has been minimized.
c937a70
to
659c7cc
Compare
This comment has been minimized.
This comment has been minimized.
659c7cc
to
c2813de
Compare
☔ The latest upstream changes (presumably #129970) made this pull request unmergeable. Please resolve the merge conflicts. |
c2813de
to
d6a6c57
Compare
These commits modify compiler targets. Some changes occurred in compiler/rustc_codegen_gcc |
As this affects call ABI, doesn't this need to be a target option rather than compiler flag? Otherwise @RalfJung will be very sad. |
Am I understanding correctly that like |
Yes, exactly. |
As a nightly flag I don't mind having this experimentally, but the docs should call out very clearly that all code that is linked together needs to use the same value for this flag. But this can't be stabilized in that form. |
Although IIRC target options don't have a stable format either, so we may as well make this part of target options and implement this the "correct" way to make using it correctly less footgunny from the get-go instead of having the flag that's asking for fireworks? Since I imagine if RfL wants this flag then surely they'll want to actually use it. Especially if this is limited to a specific target architecture(?). |
Yeah if this can be made a target option that would probably be better. I just didn't want to block experimentation on these concerns. |
Right, that's fair and fine by me as well. |
89063aa
to
caecc18
Compare
Np. I have added __m128 & __m256 test cases (vector types are skipped in the same way as float types). |
I get an ICE building rustc --edition=2021 --target i686-unknown-linux-gnu -Zregparm=3 --crate-type rlib library/core/src/lib.rs --sysroot=/dev/null
The new tests pass though (by the way, since we pass |
Backtrace gives this:
Panic is here, it seems: unreachable!("x86 shouldn't be passing arguments by {:?}", arg.mode) Error: internal error: entered unreachable code: x86 shouldn't be passing arguments by Pair(ArgAttributes { regular: NonNull | NoUndef, arg_ext: None, pointee_size: Size(0 bytes), pointee_align: Some(Align(1 bytes)) }, ArgAttributes { regular: NoUndef, arg_ext: None, pointee_size: Size(0 bytes), pointee_align: None }) I had a feeling that was unwarrantedly confident, but the error branch was preexisting, so it wasn't obvious how we'd start to hit it. Hmm... |
oh, because we're now making the Rust ABI go through it. |
@azhogin I have pushed a fix for the test omission and reverted @ojeda Good catch for the Let's try again, without the scope-creep into |
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.
Thank you for your work on this, @azhogin. This needs a slight doc update if we keep it in its current state, but I'm fully content with this if it satisfies the extern "C"
ABI requirements of the kernel.
It now seems obvious that @nbdd0121 was right: the goals of ABI compatibility and performance optimization are different. I realize it might be confusing, but c'est la vie. We shouldn't land this PR with extern "Rust"
support for -Zregparm
, unless we really want to be here for a few more weeks.
We also haven't even started to discuss other possibilities... like making extern "Rust"
always be "fastcall-like" on x86.
Co-authored-by: Jubilee <[email protected]>
Could it be better to just skip signatures (for Rust cc) with PassMode:: (Pair/Cast/Indirect with meta_attrs) ? Yes, it is still a temporary solution, but allows to support most of "Rust" calling conv signatures. I am just not sure if -Zregparm flag without Rust cc has valuable meaning for RfL.
|
The most core need here is to be able to match the ABI of the kernel's C code as it is normally built, so that Rust's presence does not force deoptimizing that C code. This patch should address that in its current state, even if it does not affect the Rust calling convention at all. Landing this patch first will allow us to still implement whatever Rust-ABI optimizations we want. But there is no need to rush such, and we should in fact scrutinize it more closely. This is not the first time the Rust and C ABIs have had the oddity of the C ABI being actually preferable for something, and it won't be the last. Especially not as a temporary situation. |
I think it has (well, assuming it actually has a positive effect for codegen for the Rust side too -- we will need to see if that is true or not), but like @workingjubilee says, it is more important to at least be able to build with the right ABI. With the docs that you added now clarifying where it applies, I think it is clear enough for users. |
Cool. I believe this will serialize with
So let's unblock those. @bors r=workingjubilee,pnkfelix |
…jubilee,pnkfelix rust_for_linux: -Zregparm=<N> commandline flag for X86 (rust-lang#116972) Command line flag `-Zregparm=<N>` for X86 (32-bit) for rust-for-linux: rust-lang#116972 Implemented in the similar way as fastcall/vectorcall support (args are marked InReg if fit).
Rollup of 6 pull requests Successful merges: - rust-lang#129935 (make unsupported_calling_conventions a hard error) - rust-lang#130432 (rust_for_linux: -Zregparm=<N> commandline flag for X86 (rust-lang#116972)) - rust-lang#131697 (`rt::Argument`: elide lifetimes) - rust-lang#131954 (shave 150ms off bootstrap) - rust-lang#131982 (Represent `hir::TraitBoundModifiers` as distinct parts in HIR) - rust-lang#132017 (Update triagebot.toml) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 6 pull requests Successful merges: - rust-lang#130432 (rust_for_linux: -Zregparm=<N> commandline flag for X86 (rust-lang#116972)) - rust-lang#131697 (`rt::Argument`: elide lifetimes) - rust-lang#131807 (Always specify `llvm_abiname` for RISC-V targets) - rust-lang#131954 (shave 150ms off bootstrap) - rust-lang#132015 (Move const trait tests from `ui/rfcs/rfc-2632-const-trait-impl` to `ui/traits/const-traits`) - rust-lang#132017 (Update triagebot.toml) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#130432 - azhogin:azhogin/regparm, r=workingjubilee,pnkfelix rust_for_linux: -Zregparm=<N> commandline flag for X86 (rust-lang#116972) Command line flag `-Zregparm=<N>` for X86 (32-bit) for rust-for-linux: rust-lang#116972 Implemented in the similar way as fastcall/vectorcall support (args are marked InReg if fit).
Command line flag
-Zregparm=<N>
for X86 (32-bit) for rust-for-linux: #116972Implemented in the similar way as fastcall/vectorcall support (args are marked InReg if fit).