NVPTX: Avoid PassMode::Direct for args in C abi by kjetilkjeka · Pull Request #117671 · rust-lang/rust

@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

Nov 7, 2023

@rustbot rustbot added the O-NVPTX

Target: the NVPTX LLVM backend for running rust on GPUs, https://llvm.org/docs/NVPTXUsage.html

label

Nov 7, 2023

davidtwco

@rustbot rustbot 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

Jan 25, 2024

@apiraino apiraino added the S-blocked

Status: Blocked on something else such as an RFC or other implementation work.

label

Jan 30, 2024

@rustbot rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-blocked

Status: Blocked on something else such as an RFC or other implementation work.

labels

Apr 24, 2024

davidtwco

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

May 28, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

May 28, 2024
…ct, r=davidtwco

NVPTX: Avoid PassMode::Direct for args in C abi

Fixes rust-lang#117480

I must admit that I'm confused about `PassMode` altogether, is there a good sum-up threads for this anywhere? I'm especially confused about how "indirect" and "byval" goes together. To me it seems like "indirect" basically means "use a indirection through a pointer", while "byval" basically means "do not use indirection through a pointer".

The return used to keep `PassMode::Direct` for small aggregates. It turns out that `make_indirect` messes up the tests and one way to fix it is to keep `PassMode::Direct` for all aggregates. I have mostly seen this PassMode mentioned for args. Is it also a problem for returns? When experimenting with `byval` as an alternative i ran into [this assert](https://github.com/rust-lang/rust/blob/61a3eea8043cc1c7a09c2adda884e27ffa8a1172/compiler/rustc_codegen_llvm/src/abi.rs#L463C22-L463C22)

I have added tests for the same kind of types that is already tested for the "ptx-kernel" abi. The tests cannot be enabled until something like rust-lang#117458 is completed and merged.

CC: `@RalfJung` since you seem to be the expert on this and have already helped me out tremendously

CC: `@RDambrosio016` in case this influence your work on `rustc_codegen_nvvm`

`@rustbot` label +O-NVPTX

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 28, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117671 (NVPTX: Avoid PassMode::Direct for args in C abi)
 - rust-lang#124251 (Add an intrinsic for `ptr::metadata`)
 - rust-lang#125573 (Migrate `run-make/allow-warnings-cmdline-stability` to `rmake.rs`)
 - rust-lang#125590 (Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable)
 - rust-lang#125598 (Make `ProofTreeBuilder` actually generic over `Interner`)
 - rust-lang#125637 (rustfmt fixes)

r? `@ghost`
`@rustbot` modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 28, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#117671 (NVPTX: Avoid PassMode::Direct for args in C abi)
 - rust-lang#125573 (Migrate `run-make/allow-warnings-cmdline-stability` to `rmake.rs`)
 - rust-lang#125590 (Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable)
 - rust-lang#125598 (Make `ProofTreeBuilder` actually generic over `Interner`)
 - rust-lang#125637 (rustfmt fixes)

r? `@ghost`
`@rustbot` modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

May 28, 2024
Rollup merge of rust-lang#117671 - kjetilkjeka:nvptx_c_abi_avoid_direct, r=davidtwco

NVPTX: Avoid PassMode::Direct for args in C abi

Fixes rust-lang#117480

I must admit that I'm confused about `PassMode` altogether, is there a good sum-up threads for this anywhere? I'm especially confused about how "indirect" and "byval" goes together. To me it seems like "indirect" basically means "use a indirection through a pointer", while "byval" basically means "do not use indirection through a pointer".

The return used to keep `PassMode::Direct` for small aggregates. It turns out that `make_indirect` messes up the tests and one way to fix it is to keep `PassMode::Direct` for all aggregates. I have mostly seen this PassMode mentioned for args. Is it also a problem for returns? When experimenting with `byval` as an alternative i ran into [this assert](https://github.com/rust-lang/rust/blob/61a3eea8043cc1c7a09c2adda884e27ffa8a1172/compiler/rustc_codegen_llvm/src/abi.rs#L463C22-L463C22)

I have added tests for the same kind of types that is already tested for the "ptx-kernel" abi. The tests cannot be enabled until something like rust-lang#117458 is completed and merged.

CC: ``@RalfJung`` since you seem to be the expert on this and have already helped me out tremendously

CC: ``@RDambrosio016`` in case this influence your work on `rustc_codegen_nvvm`

``@rustbot`` label +O-NVPTX

RalfJung

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Jun 12, 2024
…ssmode, r=davidtwco

Nvptx remove direct passmode

This PR does what should have been done in rust-lang#117671. That is fully avoid using the `PassMode::Direct` for `extern "C" fn` for `nvptx64-nvidia-cuda` and enable the compatibility test. `@RalfJung` [pointed me in the right direction](rust-lang#117480 (comment)) for solving this issue.

There are still some ABI bugs after this PR is merged. These ABI tests are created based on what is actually correct, and since they continue passing with even more of them enabled things are improving. I don't have the time to tackle all the remaining issues right now, but I think getting these improvements merged is very valuable in themselves and plan to tackle more of them long term.

This also doesn't remove the use of `PassMode::Direct` for `extern "ptx-kernel" fn`. This was also not trivial to make work. And since the ABI is hidden behind an unstable feature it's less urgent.

I don't know if it's correct to request `@RalfJung` as a reviewer (due to team structures), but he helped me a lot to figure out this stuff. If that's not appropriate then `@davidtwco` would be a good candidate since he know about this topic from rust-lang#117671

r​? `@RalfJung`

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jun 12, 2024
Rollup merge of rust-lang#125980 - kjetilkjeka:nvptx_remove_direct_passmode, r=davidtwco

Nvptx remove direct passmode

This PR does what should have been done in rust-lang#117671. That is fully avoid using the `PassMode::Direct` for `extern "C" fn` for `nvptx64-nvidia-cuda` and enable the compatibility test. `@RalfJung` [pointed me in the right direction](rust-lang#117480 (comment)) for solving this issue.

There are still some ABI bugs after this PR is merged. These ABI tests are created based on what is actually correct, and since they continue passing with even more of them enabled things are improving. I don't have the time to tackle all the remaining issues right now, but I think getting these improvements merged is very valuable in themselves and plan to tackle more of them long term.

This also doesn't remove the use of `PassMode::Direct` for `extern "ptx-kernel" fn`. This was also not trivial to make work. And since the ABI is hidden behind an unstable feature it's less urgent.

I don't know if it's correct to request `@RalfJung` as a reviewer (due to team structures), but he helped me a lot to figure out this stuff. If that's not appropriate then `@davidtwco` would be a good candidate since he know about this topic from rust-lang#117671

r​? `@RalfJung`