fix sparc64 ABI for aggregates with floating point members

[psumbera]

Fixes #86163

[rust-highfive]

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

[psumbera]

I have created this with some suggestions from @eddyb.

[nagisa]

This should have a regression test which tests both the LLVM-IR output and the assembly. One with #[no_core] and compiler-flags: --target=sparc64-... so that it runs on all hosts would be ideal.

See src/test/codegen/sparc-struct-abi.rs for an example of an LLVM-IR test and src/test/assembly for the assembly ones.

[psumbera]

@nagisa isn't extending src/test/codegen/sparc-struct-abi.rs enough? I don't know how to check these easily in assembler...

[nagisa]

I would say that LLVM IR is a pretty poor proxy for verifying the ABI is what we want it to be. Conventionally we would have a run-pass test which would call some functions defined in C, like this.

However since nobody is really able to run tests on a SPARC and the CI does not cover the architecture, such a run-pass test would be pretty much always skipped (though adding one would still be appreciated!)

An assembly test therefore is the next best thing, I feel. As for how to construct such a test, I would start with emitting some assembly from clang for an equivalent call and then FileChecking the ABI instructions around… Kinda like this (taking the code from the issue):

// compiler-flags: -Copt-level=3 --target=sparc...

#[repr(C)]
pub struct Franta {
        a:f32,
        b:f32,
        c:f32,
        d:f32,
}

// CHECK-LABEL: callee
#[no_mangle]
pub extern "C" fn callee(arg: Franta) {
    // CHECK: fmovs   %f?, %f0
    // CHECK: call use
    use(arg.a); 
    // CHECK: fmovs   %f?, %f0
    // CHECK: call use
    use(arg.b); 
    // CHECK: fmovs   %f?, %f0
    // CHECK: call use
    use(arg.c);
    // CHECK: fmovs   %f?, %f0
    // CHECK: call use
    use(arg.d);
}

extern "C" {
    fn opaque_callee(arg: Franta, intarg: i32);
    fn use(arg: f32);
}

// CHECK-LABEL: caller
// CHECK: mov       0x3, %o2
// CHECK: fmovs     %{{.*}}, %f0
// CHECK: fmovs     %{{.*}}, %f1
// CHECK: fmovs     %{{.*}}, %f2
// CHECK: fmovs     %{{.*}}, %f3
// CHECK: call {{.*}}
pub unsafe extern "C" fn caller() {
    opaque_callee(Franta { a: 1.0, b: 2.0, c: 3.0, d: 4.0 }, 3);
}

Do note that I pretty much wrote this entire thing into the github's comment field from memory so there may be mistakes in the code above etc, but should give you a good starting point, hopefully?

[psumbera]

I think I that asm testing cannot reliable work. One time it uses fmovs instruction and another time st instruction.

Clang code below:

void calle(struct Franta a) {
  tst_use(a.a);
  tst_use(a.b);
  tst_use(a.c);
  tst_use(a.d);
}

generates:

calle:                                  ! @calle
! %bb.0:
        save %sp, -208, %sp
        st %f0, [%fp+2031]
        add %fp, 2031, %i0
        or %i0, 4, %i0
        st %f1, [%i0]
        st %f2, [%fp+2039]
        st %f3, [%fp+2043]
        ld [%fp+2031], %f1
        call tst_use
        stx %i0, [%fp+2023]
        ldx [%fp+2023], %i0                     ! 8-byte Folded Reload
        call tst_use
        ld [%i0], %f1
        call tst_use
        ld [%fp+2039], %f1
        call tst_use
        ld [%fp+2043], %f1
        ret
        restore

Where Rust code:

pub unsafe extern "C" fn callee(arg: Franta) {
    tst_use(arg.a);
    tst_use(arg.b);
    tst_use(arg.c);
    tst_use(arg.d);
}

generates:

callee:
        .cfi_startproc
        save %sp, -192, %sp
        .cfi_def_cfa_register %fp
        .cfi_window_save
        .cfi_register %o7, %i7
        st %f3, [%fp+2043]
        st %f2, [%fp+2039]
        st %f1, [%fp+2035]
        fmovs %f0, %f1
        call tst_use
        nop
        call tst_use
        ld [%fp+2035], %f1
        call tst_use
        ld [%fp+2039], %f1
        call tst_use
        ld [%fp+2043], %f1
        ret
        restore

[psumbera]

E.g. Following works. But I think it can change with next LLVM update:

// assembly-output: emit-asm
// needs-llvm-components: sparc
// compile-flags: --target=sparcv9-sun-solaris
#![crate_type = "lib"]

#[repr(C)]
pub struct Franta {
        a:f32,
        b:f32,
        c:f32,
        d:f32,
}

// CHECK-LABEL: callee
#[no_mangle]
pub unsafe extern "C" fn callee(arg: Franta) {
    // CHECK: st %f3, [%fp+2043]
    // CHECK: st %f2, [%fp+2039]
    // CHECK: st %f1, [%fp+2035]
    // CHECK: fmovs %f0, %f1
    tst_use(arg.a);
    tst_use(arg.b);
    tst_use(arg.c);
    tst_use(arg.d);
}

extern "C" {
    fn opaque_callee(arg: Franta, intarg: i32);
    fn tst_use(arg: f32);
}

// CHECK-LABEL: caller
// CHECK: ld [%i1], %f0
// CHECK: ld [%i2], %f1
// CHECK: ld [%i3], %f2
// CHECK: ld [%i0], %f3
// CHECK: call {{.*}}
// CHECK: mov     3, %o2
pub unsafe extern "C" fn caller() {
    opaque_callee(Franta { a: 1.0, b: 2.0, c: 3.0, d: 4.0 }, 3);
}

[nagisa]

I think it is fine if some tests change in output occasionally after a change as major as LLVM bump, so long as it is easy to tell that the test output did not become wrong. There are a couple ways to reduce the frequency of such changes too – first is using the highest optimization level available (-Copt-level=3), another is writing the FileCheck rules in such a way that the test is resilient to offsets on the stack changing, in some ways, for example. Adjusting the test to the following code makes me reasonably confident that maintaining this test won't be an issue. Here's what I came up with:

The code

// assembly-output: emit-asm
// needs-llvm-components: sparc
// compile-flags: --target=sparcv9-sun-solaris
#![crate_type = "lib"]
#![feature(no_core, lang_items)]
#![no_core]

#[lang = "sized"]
pub trait Sized {}
#[lang = "copy"]
pub trait Copy {}

#[repr(C)]
pub struct Franta {
    a: f32,
    b: f32,
    c: f32,
    d: f32,
}

// NB: due to delay slots the `ld` following the call is actually executed before the call.
#[no_mangle]
pub unsafe extern "C" fn callee(arg: Franta) {
    // CHECK-LABEL: callee:
    // CHECK: st %f3, [[PLACE_D:.*]]
    // CHECK: st %f2, [[PLACE_C:.*]]
    // CHECK: st %f1, [[PLACE_B:.*]]
    // CHECK: st %f0, [[PLACE_A:.*]]
    // CHECK: call tst_use
    // CHECK-NEXT: ld [[PLACE_A]], %f1
    // CHECK: call tst_use
    // CHECK-NEXT: ld [[PLACE_B]], %f1
    // CHECK: call tst_use
    // CHECK-NEXT: ld [[PLACE_C]], %f1
    // CHECK: call tst_use
    // CHECK-NEXT: ld [[PLACE_D]], %f1
    clobber();
    tst_use(arg.a);
    tst_use(arg.b);
    tst_use(arg.c);
    tst_use(arg.d);
}

extern "C" {
    fn opaque_callee(arg: Franta, intarg: i32);
    fn tst_use(arg: f32);
    fn clobber();
}

#[no_mangle]
pub unsafe extern "C" fn caller() {
    // CHECK-LABEL: caller:
    // CHECK: ld [{{.*}}], %f0
    // CHECK: ld [{{.*}}], %f1
    // CHECK: ld [{{.*}}], %f2
    // CHECK: ld [{{.*}}], %f3
    // CHECK: call opaque_callee
    // CHECK: mov     3, %o2
    opaque_callee(Franta { a: 1.0, b: 2.0, c: 3.0, d: 4.0 }, 3);
}

[nagisa]

Can you please squash the commits into 1 or 2 so that the commit history is meaningful once merged into the main branch?

[nagisa]

@bors r+ thanks!

[bors]

📌 Commit edbdda3 has been approved by nagisa

[matthiaskrgr]

failed in a rollup: #91232 (comment)
@bors r-

[psumbera]

I have removed last problematic test (where was the collision with readnone). It seemed that -Cno-prepopulate-passes would add more complexity and previous tests would need to be rewritten (and became more complex).

[nagisa]

@bors r+

[bors]

📌 Commit bde8b05e2779120b349aba30effcd2896c2cb12f has been approved by nagisa

[bors]

⌛ Testing commit bde8b05e2779120b349aba30effcd2896c2cb12f with merge 40df9dfd7f06199f71890877ad3fed3b5777ed39...

[bors]

💔 Test failed - checks-actions

[nagisa]

Hm, this is a confusing error given that we specifically specify the optimization level flag, but you could switch some of the CHECKs to CHECK-DAGs to permit the st instructions to appear in an arbitrary order.

(EDIT: that said is it just me or is the or business in

            add %fp, 2031, %i0 
            or %i0, 4, %i0 
            st %f1, [%i0] 
            st %f3, [%fp+2043] 
            st %f2, [%fp+2039] 
            st %f0, [%fp+2031] 

really really sketchy?)

[psumbera]

I have added -Copt-level=3 as you have suggested earlier. I was able to reproduce the problem when I have added -Copt-level=0. Hopefully with level 3 on Linux test system we will get correct results...

[nagisa]

@bors r+

[bors]

📌 Commit 128ceec has been approved by nagisa

[bors]

⌛ Testing commit 128ceec with merge a2b7b78...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing a2b7b78 to master...

[rust-timer]

Finished benchmarking commit (a2b7b78): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

• Small improvement in instruction counts (up to -0.7% on incr-unchanged builds of deeply-nested-async)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

[glaubitz]

@psumbera Thanks for fixing this!