Skip to content
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

Make Rust ABI return types up to two pointers in registers #124373

Closed

Conversation

GoldsteinE
Copy link
Contributor

Currently functions like

fn returns_two_u64s() -> [u64; 2] {
    [1, 2]
}

or

fn returns_single_u128_wrapped_in_array() -> [u128; 1] {
    [42]
}

pass their return values via stack on "Rust" x86_64 ABI (but notably not on "C" ABI).

This PR allows aggregates up to two pointer sizes to be passed in registers, so these functions would be treated as if they returned (u64, u64) (treating the second one as if it returned u128 would be cleaner, but I haven’t yet figured out how to do this and it doesn’t seem to make a difference).

This is a perf-motivated change, so I’d really like to see rustc-perf on this change. I’ll try to also do some benchmarks locally.

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 25, 2024
@rust-log-analyzer

This comment has been minimized.

@GoldsteinE
Copy link
Contributor Author

Oops, that’s kinda weird. I’ll try to reproduce it locally.

@GoldsteinE
Copy link
Contributor Author

This version is incomplete, but should probably “work” I think?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be codegen tests that demonstrate what this actually does.

@zetanumbers
Copy link
Contributor

Are there no tests for such optimizations?

@@ -786,7 +786,7 @@ fn fn_abi_adjust_for_abi<'tcx>(
// an LLVM aggregate type for this leads to bad optimizations,
// so we pick an appropriately sized integer type instead.
arg.cast_to(Reg { kind: RegKind::Integer, size });
} else if size <= data_pointer_size * 2 && size.bytes() % 2 == 0 {
} else if size == data_pointer_size * 2 && size.bytes() % 2 == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no comment or anything explaining this subtlety -- please add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my original idea on how this should look got shot down by CI, so I'll look into this some more and add comments to the final version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so it would be useful to understand why the original assumption you made is not compatible with codegen and explain it here. It also may suggest a deeper bug in your implementation, though it's not certain unless you look into it.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#16 exporting to docker image format
#16 sending tarball 31.2s done
#16 DONE 34.3s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
--- stderr -------------------------------
warning: variable does not need to be mutable
##[warning]  --> alloc_input.rs:16:56
   |
16 |         pub unsafe extern "C" fn variadic_fn(n: usize, mut args: ...) -> usize {
   |                                                        |
   |                                                        help: remove this `mut`
   |
   = note: `#[warn(unused_mut)]` on by default
   = note: `#[warn(unused_mut)]` on by default

thread 'rustc' panicked at /checkout/tests/ui-fulldeps/stable-mir/check_abi.rs:95:5:
assertion `left matches right` failed
  left: Cast { pad_i32: false, cast: CastTarget { prefix: [None, None, None, None, None, None, None, None], rest: Uniform { unit: Reg { kind: Integer, size: Size(8 bytes) }, total: Size(16 bytes), is_consecutive: false }, attrs: ArgAttributes { regular: , arg_ext: None, pointee_size: Size(0 bytes), pointee_align: None } } }
 right: PassMode::Indirect { .. }
warning: 1 warning emitted
------------------------------------------


@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2024
@GoldsteinE
Copy link
Contributor Author

Okay, I’ve got it now. Doing it this way produces a load of [2 x i40], which looks like it has size 10, but it actually has size 16, causing UB. I think I know how to do it properly now.

@erikdesjardins
Copy link
Contributor

erikdesjardins commented Apr 26, 2024

Note that this was previously implemented in #76986 / #77434, and previously reverted in #94570 due to #85265.

@GoldsteinE
Copy link
Contributor Author

Interesting, I’ll need to verify that these cases are handled.

@GoldsteinE
Copy link
Contributor Author

Unfortunately, I wasn’t able to circumvent #85265. I’ll tinker with ABI some more and open a new PR if I find something useful.

@GoldsteinE GoldsteinE closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants