rust_for_linux: -Zregparm= commandline flag for X86 (#116972) by azhogin · Pull Request #130432 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation87 Commits4 Checks12 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Command line flag -Zregparm=<N> for X86 (32-bit) for rust-for-linux: #116972
Implemented in the similar way as fastcall/vectorcall support (args are marked InReg if fit).
r? @pnkfelix
rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
As this affects call ABI, doesn't this need to be a target option rather than compiler flag? Otherwise @RalfJung will be very sad.
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 -C soft-float, the problem that if this is a compiler flag, then it's easy to have code compiled with -Z regparam call code that is not -Z regparam even though they appear to be of the same concrete target, then dragons get summoned?
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.
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.
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.
56 tasks
I get an ICE building core, e.g.
rustc --edition=2021 --target i686-unknown-linux-gnu -Zregparm=3 --crate-type rlib library/core/src/lib.rs --sysroot=/dev/null
query stack during panic:
#0 [fn_abi_of_instance] computing call ABI of `slice::ascii::<impl at library/core/src/slice/ascii.rs:9:1: 9:10>::is_ascii`
#1 [eval_to_allocation_raw] const-evaluating + checking `ascii::ascii_char::<impl at library/core/src/ascii/ascii_char.rs:588:1: 588:30>::fmt::HEX_DIGITS`
end of query stack
The new tests pass though (by the way, since we pass --target, do we need only-x86? I noticed one of the sets was ignored)
Backtrace gives this:
17: 0x7a29058f5943 - fill_inregs<rustc_middle::ty::Ty, rustc_middle::ty::layout::LayoutCx>
at /home/jubilee/rust/rustc/compiler/rustc_target/src/callconv/x86.rs:178:17
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 extern "Rust" handling from the flag, you may wish to pull before you continue working. And feel free to drop/edit/squash/whatever those commits, naturally (though you may wish to branch off before you do so, as to have the extern "Rust" code around).
@ojeda Good catch for the only-x86! Yeah, it's completely unnecessary.
Let's try again, without the scope-creep into extern "Rust"? It builds library/core now with -Zregparm=3, at least.
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 workingjubilee@gmail.com
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.
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.
I tested core compilation with this change (and its ok):
// For types generating PassMode::Cast, PassMode::Indirect(meta_attrs) and PassMode::Pair,
// InRegs will not be set.
// Maybe, this is a FIXME
let has_incompatibles = fn_abi.args.iter().any(
|arg| matches!(arg.mode,
PassMode::Pair { .. }
| PassMode::Indirect { attrs: _, meta_attrs: Some(_), on_stack: _ }
| PassMode::Cast { .. })
);
if has_incompatibles && rust_abi {
return;
}
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 am just not sure if -Zregparm flag without Rust cc has valuable meaning for RfL.
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.
📌 Commit 37dc4ec has been approved by workingjubilee,pnkfelix
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
fmease added a commit to fmease/rust that referenced this pull request
…jubilee,pnkfelix
rust_for_linux: -Zregparm= 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).
bors added a commit to rust-lang-ci/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#130432 - azhogin:azhogin/regparm, r=workingjubilee,pnkfelix
rust_for_linux: -Zregparm= 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).
95 tasks
antoyo pushed a commit to antoyo/rust that referenced this pull request
…jubilee,pnkfelix
rust_for_linux: -Zregparm= 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).
Reviewers
Labels
Area: Concerning the application binary interface (ABI)
Area: Command-line interface (CLI) to the compiler
Relevant for the Rust-for-Linux project
Target: x86 processors, 32 bit (like i686-*) (IA-32)
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.