-
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
va_args implementation for AAPCS. #73655
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
r? @nikic (I guess) |
Implement the va args in codegen for AAPCS, this will be used as the default va_args implementation for AArch64 rather than the va_args llvm-ir as it currently is. Copyright (c) 2020, Arm Limited.
d3dc641
to
fc52b47
Compare
@bjorn3 I addressed your comment but force pushed, sorry about that @dlrobertson - Bringing this to your attention seeing so you commented on the issue regarding this |
@JamieCunliffe Thanks for this! I'm not super familiar with the AAPCS64 spec, but on a cursory glance it looks great. |
For reference, this is the clang implementation of aapcs va_arg handling: https://github.com/llvm/llvm-project/blob/9548697df9c6f65a3dba4e8de4d015650e73101c/clang/lib/CodeGen/TargetInfo.cpp#L5795 I had a bunch of questions here, but then I realized that Rust's va_arg intrinsic only supports integer and float types, so we don't need handling for any of the "interesting" cases, like homogeneous aggregates. My remaining question here would be why we can assume a little endian target. |
src/librustc_codegen_llvm/va_arg.rs
Outdated
// the offset again. | ||
|
||
if layout.align.abi.bytes() > 8 { | ||
assert!(layout.align.abi.bytes() <= 16); |
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.
As a mild preference, I would align to the given alignment here, even if 16 is currently the only possibility.
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.
I think the assert was unnecessary and I was being a little over cautious, according to the AAPCS 16 is the given alignment for this (https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#the-va-arg-macro).
if (alignof(type) > 8)
offs = (offs + 15) & -16; // round up
It should only be done on the general purpose registers though and I had missed that in the above check so I have fixed that.
Tested locally and things look good!
Yeah IIRC we wouldn't be able to rely on the |
Lack of actual hardware for testing this, I didn't have any hardware available internally to be able to run this so we decided it was best to have the assert. |
Also the alignment should only be done on general register types as per the AAPCS so fixed that issue. Copyright (c) 2020, Arm Limited.
Okay, sounds reasonable. @bors r+ rollup=always |
📌 Commit 8b58eb9 has been approved by |
va_args implementation for AAPCS. Implement the va args in codegen for AAPCS, this will be used as the default va_args implementation for AArch64 rather than the va_args llvm-ir as it currently is. This should fix the following issues: rust-lang#56475 rust-lang#72579
va_args implementation for AAPCS. Implement the va args in codegen for AAPCS, this will be used as the default va_args implementation for AArch64 rather than the va_args llvm-ir as it currently is. This should fix the following issues: rust-lang#56475 rust-lang#72579
…arth Rollup of 9 pull requests Successful merges: - rust-lang#73655 (va_args implementation for AAPCS.) - rust-lang#73893 (Stabilize control-flow-guard codegen option) - rust-lang#74237 (compiletest: Rewrite extract_*_version functions) - rust-lang#74454 (small coherence cleanup) - rust-lang#74528 (refactor and reword intra-doc link errors) - rust-lang#74568 (Apply rust-lang#66379 to `*mut T` `as_ref`) - rust-lang#74570 (Use forge links for prioritization procedure) - rust-lang#74589 (Update books) - rust-lang#74635 (Fix tooltip position if the documentation starts with a code block) Failed merges: r? @ghost
Implement the va args in codegen for AAPCS, this will be used as the
default va_args implementation for AArch64 rather than the va_args
llvm-ir as it currently is.
This should fix the following issues:
#56475
#72579