c-variadic: use `emit_ptr_va_arg` for mips by folkertdev · Pull Request #152576 · rust-lang/rust

folkertdev · 2026-02-13T13:10:51Z

tracking issue: #44930

After reading the implementation carefully, I believe it really is just emit_ptr_va_arg.

The LLVM implementation can be found here: https://github.com/llvm/llvm-project/blob/289a3292be0c6a3df86bcdf5be7dd05b79a5570c/llvm/lib/Target/Mips/MipsISelLowering.cpp#L2338.

r? workingjubilee

rustbot · 2026-02-13T13:10:56Z

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

folkertdev · 2026-02-13T13:11:26Z

+            bx,
+            addr,
+            target_ty,
+            PassMode::Direct,


Nothing in the LLVM implementation suggests a double load. byval is not handled.

and there is no conditional logic based on the size either

folkertdev · 2026-02-13T13:11:38Z

+            match bx.tcx().sess.target.pointer_width {
+                32 => SlotSize::Bytes4,
+                64 => SlotSize::Bytes8,
+                _ => unreachable!(),
+            },


based on

unsigned ArgSlotSizeInBytes = (ABI.IsN32() || ABI.IsN64()) ? 8 : 4;

...Hm, but

(ABI.IsN32() || ABI.IsN64()) ? 8 : 4;

evaluates to 8 if N32 or N64 is true, no? And N32 is an ILP32 ABI despite having 64-bit registers available, if I understand correctly.

Is this correct anyways for the targets we actually support? A comment might be desired about the frumious "N32" ABI, if so.

It is correct for our current targets, but we can match the LLVM logic more based on

https://github.com/llvm/llvm-project/blob/67e47fb5317cabd82317379baf8e31d61984d43d/llvm/lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp#L64-L78

I've implemented that now.

folkertdev · 2026-02-13T13:11:49Z

+                64 => SlotSize::Bytes8,
+                _ => unreachable!(),
+            },
+            AllowHigherAlign::Yes,


Because the code allows, and corrects for, a higher alignment

if (Align > getMinStackArgumentAlignment()) {

folkertdev · 2026-02-13T13:12:00Z

+            match bx.tcx().sess.target.endian {
+                Endian::Big => ForceRightAdjust::Yes,
+                Endian::Little => ForceRightAdjust::No,
+            },


there is a whole comment about this

// In big-endian mode we must adjust the pointer when the load size is smaller // than the argument slot size. We must also reduce the known alignment to // match. For example in the N64 ABI, we must add 4 bytes to the offset to get // the correct half of the slot, and reduce the alignment from 8 (slot // alignment) down to 4 (type alignment).

Something to this effect seems rather helpful to have in a code comment rather than a review comment? Ditto for align comment, I don't think that's all going to be obvious when somebody reads this code.

ForceRightAdjust could also use some documentation, I needed to dig a bit to figure out its intended purpose. (same for AllowHigherAlign though that one is more obvious)

folkertdev · 2026-02-13T13:18:17Z

Pinging some target maintainers:

@Gelbpunkt @wzssyqa @chenx97 @709924470 @Cyanoxygen @Fearyncess

Mostly this is just a heads-up. You could try to run (with your target) this test to be sure that it all works:

./x test tests/run-make/c-link-to-rust-va-list-fn --target mipsel-unknown-linux-gnu

this likely needs some sort of config in bootstrap.toml, I have

[target.mips64-unknown-linux-gnuabi64]
runner = "qemu-mips64 -L /usr/mips64-linux-gnuabi64"

[target.mipsel-unknown-linux-gnu]
runner = "qemu-mipsel -cpu 24Kf -L /usr/mipsel-linux-gnu"

[target.mips-unknown-linux-gnu]
runner = "qemu-mips -L /usr/mips-linux-gnu"

folkertdev · 2026-04-21T20:23:15Z

r? tgross35

rustbot · 2026-04-21T20:23:20Z

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

tgross35 · 2026-04-21T22:00:11Z

Do we have any asm tests for varargs that could be extended to this target? Unfortunately I don't know much about this area of code and I'm not positive what effects I should actually be looking for.

folkertdev · 2026-04-21T22:06:17Z

We don't, but I could add some.

Really I think what we're looking for is whether the mips implementation subtly deviates from what emit_ptr_va_arg does. E.g. by changing the alignment for some type.

I'll also just extract the non-mips changes, they were combined for convenience but not it probably makes more sense to split them up.

folkertdev

this will need a rebase after the currently in flight PR merges, so it's not quite ready yet, but I added an extra commit that captures what assembly LLVM generates, and then an updated version with the rustc implementation.

on 32-bit mips that leads to a slightly nasty diff no matter what I do, but I believe it is in fact equivalent.

View changes since this review

folkertdev · 2026-04-22T13:31:19Z

-    // mips64el-NEXT: lw  $1, 0($1)
+    // mips64el-NEXT: lw  $2, 0($1)
    // mips64el-NEXT: jr  $ra
-    // mips64el-NEXT: sll $2, $1, 0
+    // mips64el-NEXT: nop


the rust versions stores the value into $2 directly, clang emits this weird "shift by zero" instruction, essentially a move, that has the same effect.

folkertdev · 2026-04-22T13:33:19Z

    // mips: lw  $1, 0($4)
-    // mips-NEXT: addiu   $2, $zero, 4
-    // mips-NEXT: addiu   $1, $1, 7
-    // mips-NEXT: move    $3, $1
-    // mips-NEXT: ins $3, $2, 0, 3
    // mips-NEXT: addiu   $2, $zero, -8
+    // mips-NEXT: addiu   $1, $1, 7
+    // mips-NEXT: and $2, $1, $2
+    // mips-NEXT: addiu   $3, $2, 8
    // mips-NEXT: sw  $3, 0($4)
-    // mips-NEXT: and $1, $1, $2
-    // mips-NEXT: lw  $2, 0($1)
-    // mips-NEXT: addiu   $1, $3, 4
-    // mips-NEXT: sw  $1, 0($4)
-    // mips-NEXT: lw  $3, 0($3)
+    // mips-NEXT: addiu   $3, $zero, 4
+    // mips-NEXT: lw  $2, 0($2)
+    // mips-NEXT: ins $1, $3, 0, 3
+    // mips-NEXT: lw  $3, 0($1)
    // mips-NEXT: jr  $ra
    // mips-NEXT: nop


I stared at this for a long time, and am fairly confident these are equivalent. The equivalence is easier to see on godbolt which compiles this slightly differently (I don't know why, but both should be valid compilations for the target)

https://godbolt.org/z/h7WxPE58n

The original version calculates a pointer to the second word, and writes that back into ap, but the value is never used and ap is later overwritten.

ori $2, $1, 4 # $2 = aligned + 4 addiu $3, $1, 8 # $3 = aligned + 8 sw $2, 0($4) # *ap = aligned + 4 lw $2, 0($1) # $2 = *$1 sw $3, 0($4) # *ap = aligned + 8 lw $3, 4($1) # $3 = *($1 + 4)

versus

addiu $2, $1, 8 # $2 = aligned + 8 sw $2, 0($4) # *ap = aligned + 8 lw $2, 0($1) # $2 = *$1 lw $3, 4($1) # $3 = *($1 + 4)

…ross35 c-variadic: `va_arg` fixes tracking issue: rust-lang#44930 extracts some generic LLVM `va_arg` fixes from rust-lang#152576 and rust-lang#155429. r? tgross35

Rollup merge of #155622 - folkertdev:va-arg-llvm-fixes, r=tgross35 c-variadic: `va_arg` fixes tracking issue: #44930 extracts some generic LLVM `va_arg` fixes from #152576 and #155429. r? tgross35

tgross35

I can barely read the mips assembly but think it serves the purpose of making sure we don't accidentally change implementations. One request here with the naming, and the request above to turn the review notes into comments, then r=me.

(Thanks for separating test addition from doing the update, made it easy to see what changed)

View changes since this review

tgross35 · 2026-04-22T22:21:20Z

Any thought on making this c-variadic.rs and then eventually merging in the ARM version and others? Since many arches need the same test cases I'm not sure it's worth duplicating the setup.

I'd rather keep them separate per-arch, working with these files is already not that fun when they do need updating. We could put them in their own directory

Reasonable enough, +1 to a variadic directory.

How did you update these by the way? As far as I know we don't have anything like LLVM's python script.

Right, so conveniently we do write the whole generated asm file to disk (for each revision), and then it's some vim stuff (retab to get rid of tabs, multiple cursors for adding the NEXT lines). But, pretty tedious still.

Especially because it took a couple of tries to land on the right thing to even test. For any real program, our version gets optimized differently because we provide LLVM with the separate loads early, while LLVM itself only expands va_arg quite late.

Reasonable enough, +1 to a variadic directory.

I'll look at that in the sparc PR

Oof, that's pretty brutal. It would be nice if we could have --bless work something like the LLVM script tbh, but that sounds like a bit of a pain to build.

tgross35 · 2026-04-22T22:27:31Z

+//@ revisions: mips mips64 mips64el
+//@ [mips] compile-flags: -Copt-level=3 --target mips-unknown-linux-gnu
+//@ [mips] needs-llvm-components: mips
+//@ [mips64] compile-flags: -Copt-level=3 --target mipsisa64r6-unknown-linux-gnuabi64
+//@ [mips64] needs-llvm-components: mips
+//@ [mips64el] compile-flags: -Copt-level=3 --target mips64el-unknown-linux-gnuabi64
+//@ [mips64el] needs-llvm-components: mips


Nit, I think we've been using more uppercase revisions for the codegen tests to match CHECK and the NEXT suffixes

really? I though it was because there is some lowercase revision name that doesn't work (windows maybe? some common name like that). But I'm happy to change them

It's just for consistency with filecheck AFAIK, we've talked about auto-uppercasing vs. just using uppercase names before to make things match. But everything is totally inconsistent so also a complete 🤷‍♂️ if you don't feel like changing it

rustbot · 2026-04-22T23:22:30Z

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.

tgross35

@bors r+ rollup

View changes since this review

rust-bors · 2026-04-22T23:46:01Z

📌 Commit e9ab558 has been approved by tgross35

It is now in the queue for this repository.

c-variadic: use `emit_ptr_va_arg` for mips tracking issue: rust-lang#44930 After reading the implementation carefully, I believe it really is just `emit_ptr_va_arg`. The LLVM implementation can be found here: https://github.com/llvm/llvm-project/blob/289a3292be0c6a3df86bcdf5be7dd05b79a5570c/llvm/lib/Target/Mips/MipsISelLowering.cpp#L2338. r? workingjubilee

…uwer Rollup of 10 pull requests Successful merges: - #146544 (mir-opt: Remove the workaround in UnreachableEnumBranching) - #154819 (Fix ICE for inherent associated type mismatches) - #155265 (Improved assumptions relating to isqrt) - #152576 (c-variadic: use `emit_ptr_va_arg` for mips) - #154481 (Mark a function only used in nightly as nightly only) - #155614 (c-variadic: rename `VaList::arg` to `VaList::next_arg`) - #155630 (Make `//@ skip-filecheck` a normal compiletest directive) - #155641 (Remove non-working code for "running" mir-opt tests) - #155652 (Expand `Path::is_empty` docs) - #155656 (rustc_llvm: update opt-level handling for LLVM 23)

Rollup merge of #152576 - folkertdev:mips-va-arg, r=tgross35 c-variadic: use `emit_ptr_va_arg` for mips tracking issue: #44930 After reading the implementation carefully, I believe it really is just `emit_ptr_va_arg`. The LLVM implementation can be found here: https://github.com/llvm/llvm-project/blob/289a3292be0c6a3df86bcdf5be7dd05b79a5570c/llvm/lib/Target/Mips/MipsISelLowering.cpp#L2338. r? workingjubilee

…uwer Rollup of 10 pull requests Successful merges: - rust-lang/rust#146544 (mir-opt: Remove the workaround in UnreachableEnumBranching) - rust-lang/rust#154819 (Fix ICE for inherent associated type mismatches) - rust-lang/rust#155265 (Improved assumptions relating to isqrt) - rust-lang/rust#152576 (c-variadic: use `emit_ptr_va_arg` for mips) - rust-lang/rust#154481 (Mark a function only used in nightly as nightly only) - rust-lang/rust#155614 (c-variadic: rename `VaList::arg` to `VaList::next_arg`) - rust-lang/rust#155630 (Make `//@ skip-filecheck` a normal compiletest directive) - rust-lang/rust#155641 (Remove non-working code for "running" mir-opt tests) - rust-lang/rust#155652 (Expand `Path::is_empty` docs) - rust-lang/rust#155656 (rustc_llvm: update opt-level handling for LLVM 23)

c-variadic: `va_arg` fixes tracking issue: rust-lang/rust#44930 extracts some generic LLVM `va_arg` fixes from rust-lang/rust#152576 and rust-lang/rust#155429. r? tgross35

…uwer Rollup of 10 pull requests Successful merges: - rust-lang/rust#146544 (mir-opt: Remove the workaround in UnreachableEnumBranching) - rust-lang/rust#154819 (Fix ICE for inherent associated type mismatches) - rust-lang/rust#155265 (Improved assumptions relating to isqrt) - rust-lang/rust#152576 (c-variadic: use `emit_ptr_va_arg` for mips) - rust-lang/rust#154481 (Mark a function only used in nightly as nightly only) - rust-lang/rust#155614 (c-variadic: rename `VaList::arg` to `VaList::next_arg`) - rust-lang/rust#155630 (Make `//@ skip-filecheck` a normal compiletest directive) - rust-lang/rust#155641 (Remove non-working code for "running" mir-opt tests) - rust-lang/rust#155652 (Expand `Path::is_empty` docs) - rust-lang/rust#155656 (rustc_llvm: update opt-level handling for LLVM 23)

rustbot assigned workingjubilee Feb 13, 2026

folkertdev commented Feb 13, 2026

View reviewed changes

folkertdev added the F-c_variadic `#![feature(c_variadic)]` label Feb 13, 2026

folkertdev changed the title ~~Mips va arg~~ c-variadic: use emit_ptr_va_arg for mips Feb 13, 2026

folkertdev mentioned this pull request Feb 21, 2026

c-variadic: error when we can't guarantee that the backend does the right thing #152935

Merged

folkertdev force-pushed the mips-va-arg branch from f49fdb1 to 198d4e6 Compare March 17, 2026 11:19

This comment has been minimized.

Sign in to view

rustbot assigned tgross35 and unassigned workingjubilee Apr 21, 2026

folkertdev mentioned this pull request Apr 21, 2026

c-variadic: va_arg fixes #155622

Merged

folkertdev force-pushed the mips-va-arg branch from 198d4e6 to 508c0ab Compare April 22, 2026 13:28

This comment has been minimized.

Sign in to view

folkertdev commented Apr 22, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

tgross35 reviewed Apr 22, 2026

View reviewed changes

tgross35 mentioned this pull request Apr 22, 2026

c-variadic: fix for sparc64 #155660

Merged

folkertdev added 2 commits April 23, 2026 01:09

c-variadic: add mips assembly test

328d053

va_arg: use emit_ptr_va_arg for mips

e9ab558

folkertdev force-pushed the mips-va-arg branch from 508c0ab to e9ab558 Compare April 22, 2026 23:22

tgross35 approved these changes Apr 22, 2026

View reviewed changes

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 Apr 22, 2026

This was referenced Apr 23, 2026

Rollup of 8 pull requests #155672

Closed

Rollup of 10 pull requests #155674

Merged

rust-bors Bot merged commit 6cc506c into rust-lang:main Apr 23, 2026
11 checks passed

rustbot added this to the 1.97.0 milestone Apr 23, 2026

Uh oh!

Conversation

folkertdev commented Feb 13, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

folkertdev commented Feb 13, 2026

Uh oh!

This comment has been minimized.

folkertdev commented Apr 21, 2026

Uh oh!

rustbot commented Apr 21, 2026

Uh oh!

tgross35 commented Apr 21, 2026

Uh oh!

folkertdev commented Apr 21, 2026

Uh oh!

This comment has been minimized.

folkertdev left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

tgross35 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Apr 22, 2026

Uh oh!

tgross35 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-bors Bot commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

folkertdev commented Feb 13, 2026 •

edited by rustbot

Loading

folkertdev left a comment •

edited by rustbot

Loading

tgross35 left a comment •

edited by rustbot

Loading

tgross35 left a comment •

edited by rustbot

Loading