Skip to content

c-variadic: use emit_ptr_va_arg for mips #152576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rust-bors merged 2 commits into from
Apr 23, 2026
+156 −4
Merged

c-variadic: use emit_ptr_va_arg for mips #152576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
328d053
c-variadic: add mips assembly test
folkertdev Apr 22, 2026
e9ab558
va_arg: use `emit_ptr_va_arg` for mips
folkertdev Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions compiler/rustc_codegen_llvm/src/va_arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,14 +1171,29 @@ pub(super) fn emit_va_arg<'ll, 'tcx>(
AllowHigherAlign::Yes,
ForceRightAdjust::No,
),
Arch::Mips | Arch::Mips32r6 | Arch::Mips64 | Arch::Mips64r6 => emit_ptr_va_arg(
bx,
addr,
target_ty,
PassMode::Direct,
Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Feb 13, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Feb 13, 2026

Choose a reason for hiding this comment

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

and there is no conditional logic based on the size either

match &target.llvm_abiname {
LlvmAbi::N32 | LlvmAbi::N64 => SlotSize::Bytes8,
LlvmAbi::O32 => SlotSize::Bytes4,
other => bug!("unexpected LLVM ABI {other}"),
},
AllowHigherAlign::Yes,
Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Feb 13, 2026

Choose a reason for hiding this comment

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

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

if (Align > getMinStackArgumentAlignment()) {

// In big-endian mode the actual value is stored in the right side of the slot, meaning
// that when the value is smaller than a slot, we need to adjust the pointer we read
// to somewhere in the middle of the slot.
match bx.tcx().sess.target.endian {
Endian::Big => ForceRightAdjust::Yes,
Endian::Little => ForceRightAdjust::No,
},
Comment on lines +1188 to +1191
Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Feb 13, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 21, 2026

Choose a reason for hiding this comment

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

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)

),

Arch::Bpf => bug!("bpf does not support c-variadic functions"),
Arch::SpirV => bug!("spirv does not support c-variadic functions"),

Arch::Mips | Arch::Mips32r6 | Arch::Mips64 | Arch::Mips64r6 => {
// FIXME: port MipsTargetLowering::lowerVAARG.
bx.va_arg(addr.immediate(), bx.cx.layout_of(target_ty).llvm_type(bx.cx))
}
Arch::Sparc | Arch::Avr | Arch::M68k | Arch::Msp430 => {
// Clang uses the LLVM implementation for these architectures.
bx.va_arg(addr.immediate(), bx.cx.layout_of(target_ty).llvm_type(bx.cx))
Expand Down
137 changes: 137 additions & 0 deletions tests/assembly-llvm/c-variadic-mips.rs
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Apr 22, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Apr 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@folkertdev folkertdev Apr 22, 2026

Choose a reason for hiding this comment

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

Reasonable enough, +1 to a variadic directory.

I'll look at that in the sparc PR

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 22, 2026

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
//@ add-minicore
//@ assembly-output: emit-asm
//
//@ 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
#![feature(c_variadic, no_core, lang_items, intrinsics, rustc_attrs, asm_experimental_arch)]
#![no_core]
#![crate_type = "lib"]

// Check that the assembly that rustc generates matches what clang emits.

extern crate minicore;
use minicore::*;

#[lang = "va_arg_safe"]
pub unsafe trait VaArgSafe {}

unsafe impl VaArgSafe for i32 {}
unsafe impl VaArgSafe for i64 {}
unsafe impl VaArgSafe for f64 {}
unsafe impl<T> VaArgSafe for *const T {}

#[repr(transparent)]
struct VaListInner {
ptr: *const c_void,
}

#[repr(transparent)]
#[lang = "va_list"]
pub struct VaList<'a> {
inner: VaListInner,
_marker: PhantomData<&'a mut ()>,
}

#[rustc_intrinsic]
#[rustc_nounwind]
pub const unsafe fn va_arg<T: VaArgSafe>(ap: &mut VaList<'_>) -> T;

#[unsafe(no_mangle)]
unsafe extern "C" fn read_f64(ap: &mut VaList<'_>) -> f64 {
// CHECK-LABEL: read_f64
//
// MIPS: lw $1, 0($4)
// MIPS-NEXT: addiu $2, $zero, -8
// MIPS-NEXT: addiu $1, $1, 7
// MIPS-NEXT: and $1, $1, $2
// MIPS-NEXT: addiu $2, $1, 8
// MIPS-NEXT: sw $2, 0($4)
// MIPS-NEXT: ldc1 $f0, 0($1)
// MIPS-NEXT: jr $ra
// MIPS-NEXT: nop
//
// MIPS64: ld $1, 0($4)
// MIPS64-NEXT: daddiu $2, $1, 8
// MIPS64-NEXT: sd $2, 0($4)
// MIPS64-NEXT: ldc1 $f0, 0($1)
// MIPS64-NEXT: jrc $ra
//
// MIPS64EL: ld $1, 0($4)
// MIPS64EL-NEXT: daddiu $2, $1, 8
// MIPS64EL-NEXT: sd $2, 0($4)
// MIPS64EL-NEXT: ldc1 $f0, 0($1)
// MIPS64EL-NEXT: jr $ra
// MIPS64EL-NEXT: nop
va_arg(ap)
}

#[unsafe(no_mangle)]
unsafe extern "C" fn read_i32(ap: &mut VaList<'_>) -> i32 {
// CHECK-LABEL: read_i32
//
// MIPS: lw $1, 0($4)
// MIPS-NEXT: addiu $2, $1, 4
// MIPS-NEXT: sw $2, 0($4)
// MIPS-NEXT: lw $2, 0($1)
// MIPS-NEXT: jr $ra
// MIPS-NEXT: nop
//
// MIPS64: ld $1, 0($4)
// MIPS64-NEXT: daddiu $2, $1, 8
// MIPS64-NEXT: sd $2, 0($4)
// MIPS64-NEXT: lw $2, 4($1)
// MIPS64-NEXT: jrc $ra
//
// MIPS64EL: ld $1, 0($4)
// MIPS64EL-NEXT: daddiu $2, $1, 8
// MIPS64EL-NEXT: sd $2, 0($4)
// MIPS64EL-NEXT: lw $2, 0($1)
// MIPS64EL-NEXT: jr $ra
// MIPS64EL-NEXT: nop
va_arg(ap)
}

#[unsafe(no_mangle)]
unsafe extern "C" fn read_i64(ap: &mut VaList<'_>) -> i64 {
// CHECK-LABEL: read_i64
//
// MIPS: lw $1, 0($4)
// 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: 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
//
// MIPS64: ld $1, 0($4)
// MIPS64-NEXT: daddiu $2, $1, 8
// MIPS64-NEXT: sd $2, 0($4)
// MIPS64-NEXT: ld $2, 0($1)
// MIPS64-NEXT: jrc $ra
//
// MIPS64EL: ld $1, 0($4)
// MIPS64EL-NEXT: daddiu $2, $1, 8
// MIPS64EL-NEXT: sd $2, 0($4)
// MIPS64EL-NEXT: ld $2, 0($1)
// MIPS64EL-NEXT: jr $ra
// MIPS64EL-NEXT: nop
va_arg(ap)
}

#[unsafe(no_mangle)]
unsafe extern "C" fn read_ptr(ap: &mut VaList<'_>) -> *const u8 {
// MIPS: read_ptr = read_i32
// MIPS64: read_ptr = read_i64
// MIPS64EL: read_ptr = read_i64
va_arg(ap)
}
Loading