NVPTX: Avoid PassMode::Direct for args in C abi by kjetilkjeka · Pull Request #117671 · rust-lang/rust
rustbot
added
S-waiting-on-review
labels
Nov 7, 2023label
Nov 7, 2023
rustbot
added
S-waiting-on-author
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.labels
Jan 25, 2024label
Jan 30, 2024
rustbot
added
S-waiting-on-author
and removed S-blocked
Status: Blocked on something else such as an RFC or other implementation work.labels
Apr 24, 2024
bors
added
S-waiting-on-bors
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, 2024matthiaskrgr 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, 2024Rollup 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
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, 2024Rollup 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`
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