c-variadic: use emit_ptr_va_arg for va_arg on sparc - #160660
Conversation
3ec8bc2 to
dc6688c
Compare
| Arch::Sparc => { | ||
| std::assert_matches!(stability, CVariadicStatus::Unstable { .. }); | ||
| emit_ptr_va_arg( | ||
| bx, | ||
| addr, | ||
| target_ty, | ||
| PassMode::Direct, | ||
| SlotSize::Bytes4, | ||
| AllowHigherAlign::No, | ||
| ForceRightAdjust::No, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Sparc is still gated by the c_variadic_experimental_arch feature.
The arguments to emit_ptr_va_arg here are based on the LLVM implementation. Many targets have an implementation of va_arg in Clang, but for sparc it is really LLVM that expands va_arg.
- it just loads from a pointer regardless of the size of the argument, hence unconditional
PassMode::Direct - The pointer is increased by just the
VTsize in bytes. Due to argument promotion, the smallest type that can actually be read usingva_argis 4 bytes, and no attempt is made to align to something higher than that. SoSlotSize::Bytes4. AllowHigherAlign::Yeswould align e.g. ani128to a 16-byte boundary. This function does not take the alignment into account at all. So,AllowHigherAlign::No- This setting is true on BE targets with a slot size of 8, where a 4-byte value could be either in the low or high bytes. Despite being a BE target, the setting is not relevant for sparc because all values that could be passed divide cleanly into 4-byte slots.
|
r? @khyperia rustbot has assigned @khyperia. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
cc target maintainer @jonathanpallant |
|
As context, I'd like to just stabilize c-variadic functions for this target, and for that we don't want to rely on LLVM's |
|
looks good in principle! but sorry, I have absolutely no clue on sparc/etc. and don't feel confident on this myself: @rustbot reroll |
|
r? codegen |
dc6688c to
fda15d9
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. |
|
The LLVM PR has now merged, and the implementation here now matches it |
| // f128 is passed indirectly. | ||
| let pass_mode = match layout.layout.backend_repr() { | ||
| BackendRepr::Scalar(scalar) => match scalar.primitive() { | ||
| Primitive::Float(Float::F128) => PassMode::Indirect, | ||
| _ => PassMode::Direct, | ||
| }, | ||
| _ => PassMode::Direct, | ||
| }; |
There was a problem hiding this comment.
f128 does not yet implement VaArgSafe, so this is unreachable in practice, but:
- that's in the works implement
VaArgSafeforf128#161424 - this way we properly match the LLVM implementation
|
@bors r+ (I didn't realize how small this was at first) |
c-variadic: use `emit_ptr_va_arg` for `va_arg` on `sparc` I've built GCC for the target and validated pretty extensively that this works. Using our helper does generate worse code for `i64` unfortunately, I've reported that as llvm/llvm-project#214594. But, given that this is a tier-3 target etc. I don't think it makes sense to go out of our way to do better, we can just wait for LLVM to resolve that issue.
c-variadic: use `emit_ptr_va_arg` for `va_arg` on `sparc` I've built GCC for the target and validated pretty extensively that this works. Using our helper does generate worse code for `i64` unfortunately, I've reported that as llvm/llvm-project#214594. But, given that this is a tier-3 target etc. I don't think it makes sense to go out of our way to do better, we can just wait for LLVM to resolve that issue.
…uwer Rollup of 6 pull requests Successful merges: - #157808 (sanitizers: Implement support for the sanitize ignorelist) - #160660 (c-variadic: use `emit_ptr_va_arg` for `va_arg` on `sparc`) - #162237 (make -Ctarget-feature warnings more explicitly FCWs) - #160111 (Generalize Decodable impl for arrays to all types) - #162174 (Update internal docs & tests to use `!` rather than `Infallible` in relation to `Try`) - #162226 (Clean up the AST visitor)
…uwer Rollup of 7 pull requests Successful merges: - #157808 (sanitizers: Implement support for the sanitize ignorelist) - #160660 (c-variadic: use `emit_ptr_va_arg` for `va_arg` on `sparc`) - #162237 (make -Ctarget-feature warnings more explicitly FCWs) - #160111 (Generalize Decodable impl for arrays to all types) - #162174 (Update internal docs & tests to use `!` rather than `Infallible` in relation to `Try`) - #162226 (Clean up the AST visitor) - #162281 (rustc-dev-guide subtree update)
Rollup merge of #160660 - folkertdev:sparc-va-arg, r=saethlin c-variadic: use `emit_ptr_va_arg` for `va_arg` on `sparc` I've built GCC for the target and validated pretty extensively that this works. Using our helper does generate worse code for `i64` unfortunately, I've reported that as llvm/llvm-project#214594. But, given that this is a tier-3 target etc. I don't think it makes sense to go out of our way to do better, we can just wait for LLVM to resolve that issue.
…uwer Rollup of 7 pull requests Successful merges: - rust-lang/rust#157808 (sanitizers: Implement support for the sanitize ignorelist) - rust-lang/rust#160660 (c-variadic: use `emit_ptr_va_arg` for `va_arg` on `sparc`) - rust-lang/rust#162237 (make -Ctarget-feature warnings more explicitly FCWs) - rust-lang/rust#160111 (Generalize Decodable impl for arrays to all types) - rust-lang/rust#162174 (Update internal docs & tests to use `!` rather than `Infallible` in relation to `Try`) - rust-lang/rust#162226 (Clean up the AST visitor) - rust-lang/rust#162281 (rustc-dev-guide subtree update)
I've built GCC for the target and validated pretty extensively that this works. Using our helper does generate worse code for
i64unfortunately, I've reported that as llvm/llvm-project#214594. But, given that this is a tier-3 target etc. I don't think it makes sense to go out of our way to do better, we can just wait for LLVM to resolve that issue.