Refactor the way cg_ssa handles indirect returns (returns via sret) — Take 2 - #160023
Conversation
|
Some changes occurred in compiler/rustc_codegen_llvm/src/builder/autodiff.rs cc @ZuseZ4
|
|
|
This comment has been minimized.
This comment has been minimized.
8a2ec6b to
25ec648
Compare
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Refactor the way cg_ssa handles indirect returns (returns via `sret`) — Take 2
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4cef956 to
d2b0028
Compare
This comment has been minimized.
This comment has been minimized.
d2b0028 to
47c07d8
Compare
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.
This comment has been minimized.
…llow for more correct ABI handling in cg_gcc
Co-authored-by: Jubilee <workingjubilee@gmail.com>
1c68965 to
cab1d5d
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
cab1d5d to
7d0d273
Compare
| // FIXME directly use the llvm intrinsic adjustment functions here | ||
| let llret = self.call(fn_ty, None, None, fn_ptr, &call_args, None, None); | ||
| let llret = | ||
| self.call(fn_ty, None, None, fn_ptr, ReturnSlot::Direct, &call_args, None, None); |
There was a problem hiding this comment.
Ah, this is also a way to address my main concern of "why are we passing another mysterious Option-like thing that needs comments to explain it".
I'm not super-enthused about call in general but this is a tolerable change as a result.
|
@bors r+ rollup |
…ingjubilee Refactor the way cg_ssa handles indirect returns (returns via `sret`) — Take 2 This continues the work that @FractalFir done in rust-lang#144976. **I managed to make this work, so this confirms the original approach works and is ready to merge.** r? workingjubilee (assigning the same reviewer as the original PR) # The problem The current way `cg_ssa` handles `PassMode::Indirect` is fundamentally incompatible with the requirements of the GCC backend. This is currently worked around in a very brittle way, that breaks on ARM. In order to function correctly, `cg_gcc` requires the `sret`(indirect return) pointer to be treated in a special way. # The solution This PR separates the `sret` pointer from all the other arguments, allowing each backend to decide how it wants to handle that pointer. This will allow GCC to do it's own thing, while changing nothing on the LLVM side. Currently, the PR just makes both backends preappend the sret arg, mimicking the previous behaviour. The PR is based on this [zulip suggestion](https://rust-lang.zulipchat.com/#narrow/channel/421156-gsoc/topic/Project.3A.20Bootstrap.20of.20rustc.20with.20rustc_codegen_gcc/near/526487725). # Assumptions I made. The PR makes certain assumptions about the ABI handling. 1. No LLVM intrinsic returns via sret. [According to my sources](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Can.20LLVM.20intrinsics.20return.20via.20PassMode.3A.3AIndirect.3F), this is always true. 2. Tail calls don't currently return via `sret`. From [asking about](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Can.20tail.20calls.20return.20indirectly.28via.20a.20pointer.29.3F/near/532984507), it seems like the tail call handling does not currently support indirect returns. So, I did not add support for that. 3. TLS shims don't perform indirect returns, and asserts and drops also don't perform indirect returns(since they return nothing) - those assumptions seemed reasonable. 4. Inline assembly calls don't use indirect returns. I *assumed* functions like `inline_asm_call` will not need the `sret` handling code. I am not 100% sure about this, tough.
…uwer Rollup of 10 pull requests Successful merges: - #162126 (Rename various resolving functions for consistency, and document them) - #162520 (Refactor `HygieneEncodeContext`) - #154373 (Skip linting unused braces for FunctionArg and MethodArg context for 2024 later ) - #160023 (Refactor the way cg_ssa handles indirect returns (returns via `sret`) — Take 2) - #161482 (Use attribute parser for `#[non_exhaustive]` attribute check) - #161867 (deeper `rustc_builtin_macros` cleanups) - #162099 (small refactor of doc attribute arguments warnings) - #162541 (split `macroless_generic_const_args` in two) - #162549 (The fuchsia team maintains `riscv64gc-unknown-fuchsia`) - #162577 (Fix `i686-pc-windows-msvc` platform support docs and target spec metadata)
…ingjubilee Refactor the way cg_ssa handles indirect returns (returns via `sret`) — Take 2 This continues the work that @FractalFir done in rust-lang#144976. **I managed to make this work, so this confirms the original approach works and is ready to merge.** r? workingjubilee (assigning the same reviewer as the original PR) # The problem The current way `cg_ssa` handles `PassMode::Indirect` is fundamentally incompatible with the requirements of the GCC backend. This is currently worked around in a very brittle way, that breaks on ARM. In order to function correctly, `cg_gcc` requires the `sret`(indirect return) pointer to be treated in a special way. # The solution This PR separates the `sret` pointer from all the other arguments, allowing each backend to decide how it wants to handle that pointer. This will allow GCC to do it's own thing, while changing nothing on the LLVM side. Currently, the PR just makes both backends preappend the sret arg, mimicking the previous behaviour. The PR is based on this [zulip suggestion](https://rust-lang.zulipchat.com/#narrow/channel/421156-gsoc/topic/Project.3A.20Bootstrap.20of.20rustc.20with.20rustc_codegen_gcc/near/526487725). # Assumptions I made. The PR makes certain assumptions about the ABI handling. 1. No LLVM intrinsic returns via sret. [According to my sources](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Can.20LLVM.20intrinsics.20return.20via.20PassMode.3A.3AIndirect.3F), this is always true. 2. Tail calls don't currently return via `sret`. From [asking about](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Can.20tail.20calls.20return.20indirectly.28via.20a.20pointer.29.3F/near/532984507), it seems like the tail call handling does not currently support indirect returns. So, I did not add support for that. 3. TLS shims don't perform indirect returns, and asserts and drops also don't perform indirect returns(since they return nothing) - those assumptions seemed reasonable. 4. Inline assembly calls don't use indirect returns. I *assumed* functions like `inline_asm_call` will not need the `sret` handling code. I am not 100% sure about this, tough.
…uwer Rollup of 11 pull requests Successful merges: - #162520 (Refactor `HygieneEncodeContext`) - #154373 (Skip linting unused braces for FunctionArg and MethodArg context for 2024 later ) - #160023 (Refactor the way cg_ssa handles indirect returns (returns via `sret`) — Take 2) - #160860 (Implement arbitrary casts in custom mir) - #161482 (Use attribute parser for `#[non_exhaustive]` attribute check) - #161867 (deeper `rustc_builtin_macros` cleanups) - #162099 (small refactor of doc attribute arguments warnings) - #162541 (split `macroless_generic_const_args` in two) - #162549 (The fuchsia team maintains `riscv64gc-unknown-fuchsia`) - #162577 (Fix `i686-pc-windows-msvc` platform support docs and target spec metadata) - #162624 (regression test for opaque field projection in closure capture)
Rollup merge of #160023 - antoyo:ssa-indirect-return, r=workingjubilee Refactor the way cg_ssa handles indirect returns (returns via `sret`) — Take 2 This continues the work that @FractalFir done in #144976. **I managed to make this work, so this confirms the original approach works and is ready to merge.** r? workingjubilee (assigning the same reviewer as the original PR) # The problem The current way `cg_ssa` handles `PassMode::Indirect` is fundamentally incompatible with the requirements of the GCC backend. This is currently worked around in a very brittle way, that breaks on ARM. In order to function correctly, `cg_gcc` requires the `sret`(indirect return) pointer to be treated in a special way. # The solution This PR separates the `sret` pointer from all the other arguments, allowing each backend to decide how it wants to handle that pointer. This will allow GCC to do it's own thing, while changing nothing on the LLVM side. Currently, the PR just makes both backends preappend the sret arg, mimicking the previous behaviour. The PR is based on this [zulip suggestion](https://rust-lang.zulipchat.com/#narrow/channel/421156-gsoc/topic/Project.3A.20Bootstrap.20of.20rustc.20with.20rustc_codegen_gcc/near/526487725). # Assumptions I made. The PR makes certain assumptions about the ABI handling. 1. No LLVM intrinsic returns via sret. [According to my sources](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Can.20LLVM.20intrinsics.20return.20via.20PassMode.3A.3AIndirect.3F), this is always true. 2. Tail calls don't currently return via `sret`. From [asking about](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Can.20tail.20calls.20return.20indirectly.28via.20a.20pointer.29.3F/near/532984507), it seems like the tail call handling does not currently support indirect returns. So, I did not add support for that. 3. TLS shims don't perform indirect returns, and asserts and drops also don't perform indirect returns(since they return nothing) - those assumptions seemed reasonable. 4. Inline assembly calls don't use indirect returns. I *assumed* functions like `inline_asm_call` will not need the `sret` handling code. I am not 100% sure about this, tough.
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (51474fd): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: missing data |
This continues the work that @FractalFir done in #144976.
I managed to make this work, so this confirms the original approach works and is ready to merge.
r? workingjubilee
(assigning the same reviewer as the original PR)
The problem
The current way
cg_ssahandlesPassMode::Indirectis fundamentally incompatible with the requirements of the GCC backend. This is currently worked around in a very brittle way, that breaks on ARM.In order to function correctly,
cg_gccrequires thesret(indirect return) pointer to be treated in a special way.The solution
This PR separates the
sretpointer from all the other arguments, allowing each backend to decide how it wants to handle that pointer. This will allow GCC to do it's own thing, while changing nothing on the LLVM side. Currently, the PR just makes both backends preappend the sret arg, mimicking the previous behaviour.The PR is based on this zulip suggestion.
Assumptions I made.
The PR makes certain assumptions about the ABI handling.
sret. From asking about, it seems like the tail call handling does not currently support indirect returns. So, I did not add support for that.inline_asm_callwill not need thesrethandling code. I am not 100% sure about this, tough.