Skip to content

c-variadic: use emit_ptr_va_arg for va_arg on sparc - #160660

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
folkertdev:sparc-va-arg
Sep 4, 2026
Merged

c-variadic: use emit_ptr_va_arg for va_arg on sparc#160660
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
folkertdev:sparc-va-arg

Conversation

@folkertdev

@folkertdev folkertdev commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
  • I did not use an LLM to create a change in this PR.
  • I used an LLM to create a change in this PR, and I have explained below how it was used.

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.

@folkertdev folkertdev added the F-c_variadic `#![feature(c_variadic)]` label Aug 6, 2026
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. labels Aug 6, 2026
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Aug 6, 2026
Comment on lines +1236 to +1247
Arch::Sparc => {
std::assert_matches!(stability, CVariadicStatus::Unstable { .. });
emit_ptr_va_arg(
bx,
addr,
target_ty,
PassMode::Direct,
SlotSize::Bytes4,
AllowHigherAlign::No,
ForceRightAdjust::No,
)
}

@folkertdev folkertdev Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

https://github.com/llvm/llvm-project/blob/5194e33faad9d84fd8d21c0305621a17e0d8a49f/llvm/lib/Target/Sparc/SparcISelLowering.cpp#L2768-L2790

  • it just loads from a pointer regardless of the size of the argument, hence unconditional PassMode::Direct
  • The pointer is increased by just the VT size in bytes. Due to argument promotion, the smallest type that can actually be read using va_arg is 4 bytes, and no attempt is made to align to something higher than that. So SlotSize::Bytes4.
  • AllowHigherAlign::Yes would align e.g. an i128 to 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.

View changes since the review

@folkertdev
folkertdev marked this pull request as ready for review August 7, 2026 12:46
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 7, 2026
@rustbot

rustbot commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

r? @khyperia

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 75 candidates
  • Random selection from 19 candidates

@folkertdev

Copy link
Copy Markdown
Contributor Author

cc target maintainer @jonathanpallant

@folkertdev

Copy link
Copy Markdown
Contributor Author

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 va_arg.

@khyperia

Copy link
Copy Markdown
Member

looks good in principle! but sorry, I have absolutely no clue on sparc/etc. and don't feel confident on this myself:

@rustbot reroll

@rustbot rustbot assigned adwinwhite and unassigned khyperia Aug 10, 2026
@adwinwhite

Copy link
Copy Markdown
Contributor

r? codegen

@rustbot rustbot assigned saethlin and unassigned adwinwhite Aug 10, 2026
@rustbot

rustbot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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.

@folkertdev

Copy link
Copy Markdown
Contributor Author

The LLVM PR has now merged, and the implementation here now matches it

https://github.com/llvm/llvm-project/blob/d6b2176126d716110eea8bf90c611eb6833f10ba/clang/lib/CodeGen/Targets/Sparc.cpp#L99-L116

Comment on lines +1239 to +1246
// 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,
};

@folkertdev folkertdev Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

f128 does not yet implement VaArgSafe, so this is unreachable in practice, but:

View changes since the review

@saethlin

saethlin commented Sep 4, 2026

Copy link
Copy Markdown
Member

@bors r+

(I didn't realize how small this was at first)

@rust-bors

rust-bors Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📌 Commit fda15d9 has been approved by saethlin

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 10. This pull request will be tested once the tree is reopened.

Reason for tree closure: broken external dependency

@rust-bors rust-bors Bot 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 4, 2026
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.
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 4, 2026
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.
rust-bors Bot pushed a commit that referenced this pull request Sep 4, 2026
…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)
rust-bors Bot pushed a commit that referenced this pull request Sep 4, 2026
…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)
@rust-bors
rust-bors Bot merged commit a63c406 into rust-lang:main Sep 4, 2026
13 checks passed
@rustbot rustbot added this to the 1.100.0 milestone Sep 4, 2026
rust-bors Bot pushed a commit that referenced this pull request Sep 4, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-run-make Area: port run-make Makefiles to rmake.rs F-c_variadic `#![feature(c_variadic)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants