fix `is_single_fp_element` for `s390x` and `x86` by folkertdev · Pull Request #161987 · rust-lang/rust

folkertdev · 2026-08-29T19:46:38Z

In #161950 (comment) we discovered that the old is_single_fp_element is incorrect in a number of ways.

it did not consider f16 or f128
it did not consider transparent wrappers
on x86, it incorrectly accepted over-aligned types
on s390x, it incorrectly accepted single-element unions and arrays

So, in practice each target does something slightly different here, and I've split the function into two.

beetrees · 2026-08-29T20:12:31Z

+        // Match GCC and Clang in allowing trailing padding. This does appear to violate the
+        // specification, but is well-established in both compilers.
+        //
+        // e.g. `#[repr(C, align(4))] struct Foo(f16)` is passed as `Reg::f32()`.


I've done some further research into this. AFAICT it seems that Clang and GCC, while they do use larger loads/stores than needed, do (coincidentally?) end up doing the right thing with the full struct size is <= 8 bytes (specifically, loading e.g. a 8 byte struct as a f16, when the struct is a f16 followed by 6 bytes of padding, will mean the f16 ends up in the right place in the register anyway). The only difference between Clang/GCC's behaviour and the ABI spec is that structs which contain a single f16/f32/f64 that are larger than 8 bytes won't get passed in a floating point register. In summary, the problem is that Clang/GCC don't ignore the trailing padding when they should.

View changes since the review

cc @uweigand in general on this PR but here in particular

(Re-reading the spec again, it does say "[...] load the argument value left-aligned into floating-point register [...]", which is probably why GCC/Clang load all the bytes instead of just the bytes of the floating-point value (of course it doesn't matter whether the padding bytes are loaded or not as they're padding, except when there's so much padding the full size of the values doesn't fit in the register). I'm guessing overaligned structs weren't considered one way or another when writing this part of the spec)

folkertdev · 2026-08-29T22:49:17Z

I'll leave it as a draft because I'm not really sure who to assign here, and I'd like some feedback from the s390x target maintainer anyway.

RalfJung · 2026-08-30T07:07:00Z

+{
+    // Contrary to X86, trailing padding is allowed on s390x.
+
+    layout = layout.peel_transparent_wrappers(cx);


TIL that peel_transparent_wrappers exists. However, its logic can only work for non-1-ZST types. The function should be renamed to reflect that as people might think it handles repr(transparent) for everything, and the doc comment of peel_transparent_wrappers should be clarified to call this out.

View changes since the review

RalfJung · 2026-08-30T07:11:23Z

+    if is_single_fp_element(arg.layout, cx) {
+        // Match GCC and Clang by explicitly passing padding, even though their behavior violates
+        // (our reading of) the specification, which says that:
+        //
+        // > Structures equivalent to a floating point type are passed in floating point registers.
+        // > A structure is equivalent to a floating point type if and only if it has exactly one
+        // > member, which is either of floating point type of itself a structure equivalent to a
+        // > floating point type.
+        //
+        // When the alignment is at most 8 but still overaligns the element, our implementation
+        // (matching GCC and Clang) is compliant but does require suboptimally large loads and
+        // stores.
+        //
+        // When the alignment is higher than 8, we passed the argument indirectly, which violates
+        // the specification but is consistent with GCC and Clang.


@uweigand @cuviper -- looks like we have the choice of either implementing the ABI correctly according to the spec, or implementing the ABI GCC/clang use. The two sadly disagree. What would you prefer we do?

View changes since the review

We need to follow the de-facto ABI, which is that trailing padding is allowed if the total size including padding still remains a power-of-two <= 8 bytes. This is consistently implemented by all compilers on the platform - I think we should update the ABI spec accordingly.

see also rust-lang/rust#161987

folkertdev · 2026-09-08T17:20:51Z

r? beetrees

beetrees · 2026-09-08T17:56:30Z

+            BackendRepr::Memory { .. } => {
+                // Structs, unions and arrays all qualify.
+                if layout.fields.count() == 1 && layout.fields.offset(0).bytes() == 0 {
+                    layout = layout.field(cx, 0);
+                    continue;
+                } else {
+                    false
+                }
+            }


-freg-struct-return ignores all zero-sized fields (compiler explorer).

View changes since the review

beetrees · 2026-09-08T18:02:59Z

+    layout = layout.peel_transparent_wrappers(cx);
+    match layout.backend_repr {
+        BackendRepr::Scalar(scalar) => match scalar.primitive() {
+            Primitive::Float(_) => true,
+            Primitive::Int(_, _) | Primitive::Pointer(_) => false,
+        },
+        BackendRepr::Memory { .. } => {
+            // A single-element array or union does not qualify.
+            if let FieldsShape::Arbitrary { .. } = layout.fields
+                && layout.fields.count() == 1
+                && layout.fields.offset(0).bytes() == 0
+            {
+                is_single_fp_element(layout.field(cx, 0), cx)
+            } else {
+                false
+            }
+        }
+        _ => false,
+    }


Any reason not to do this in a loop like the x86 is_single_fp_element does?

View changes since the review

beetrees · 2026-09-08T18:31:10Z

+            if let FieldsShape::Arbitrary { .. } = layout.fields
+                && layout.fields.count() == 1
+                && layout.fields.offset(0).bytes() == 0
+            {
+                is_single_fp_element(layout.field(cx, 0), cx)
+            } else {
+                false
+            }


Unlike the x86 ABI, the s390x ABI (and Clang/GCC) does not allow empty structs here. Is there any Rust-wide expectation that #[repr(C)] struct A(f32, PhantomData<()>); has the same ABI as #[repr(C)] struct B(f32); (or the C struct B { float f; };)? I haven't been able to find any documentation on the subject. It feels like the definition of a ZST with a "trivial ABI" from the not-yet-merged #157973 would be logical to be ignored in structs, but I'm not sure if there has been any discussion about this. For now, probably worth at least leaving a FIXME here.

View changes since the review

No there's nothing official AFAIK. Only repr(C) structs where all fields have a C equivalent have any guarantees.

But I agree "completely ignore types with trivial ABI" makes sense both for the ABI (rust-lang/unsafe-code-guidelines#623) and for repr(C) layout.

For C and C++, the s390x ABI does not allow empty structs. However, I don't think this necessarily has to impose a restriction on what we can define for the Rust ABI here - for types used across a cross-language boundary, this should not cause issues in practice.

Given that ZST are much more frequently used in Rust than in C, I do agree it makes sense to ignore them in Rust here.

rustbot · 2026-09-08T19:01:40Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

beetrees · 2026-09-08T19:35:11Z

+        // When the alignment is at most 8 but still overaligns the element, our implementation
+        // (matching GCC and Clang) is compliant but does require suboptimally large loads and
+        // stores.


Upon further testing, while it is true that, for example, #[repr(C, align(8))] struct Foo(f32); is passed the same with cast_to(Reg::f32()) and cast_to(Reg::f64()) when the argument gets placed in registers, it is not equivalent if there are no floating point registers left and arguments are passed on the stack, as arguments are right-aligned within their 8-byte slot (I missed this when I was initially comparing them). The comment can probably simplified to something closer to what you originally had (that GCC/Clang don't ignore padding whereas our reading of the spec says the padding should be ignored).

View changes since the review

beetrees reviewed Aug 29, 2026

View reviewed changes

folkertdev force-pushed the single-fp-element branch from 5681e66 to 50861ec Compare August 29, 2026 20:23

beetrees reviewed Aug 29, 2026

View reviewed changes

Comment thread compiler/rustc_abi/src/layout/ty.rs Outdated

folkertdev mentioned this pull request Aug 29, 2026

abi/compatibility test: add simple float type #161991

Draft

folkertdev force-pushed the single-fp-element branch from 50861ec to 5867f93 Compare August 29, 2026 21:31

beetrees reviewed Aug 29, 2026

View reviewed changes

Comment thread compiler/rustc_target/src/callconv/s390x.rs Outdated

folkertdev force-pushed the single-fp-element branch from 5867f93 to 2172749 Compare August 29, 2026 22:02

fix is_single_fp_element for s390x and x86

9ae7ff3

folkertdev force-pushed the single-fp-element branch from 2172749 to 9ae7ff3 Compare August 29, 2026 22:38

RalfJung reviewed Aug 30, 2026

View reviewed changes

abi/compatibility test: add simple float type tests

2992d1d

folkertdev added a commit to folkertdev/abi-cafe that referenced this pull request Aug 30, 2026

add is_single_fp_element reproducers

a31b863

see also rust-lang/rust#161987

folkertdev mentioned this pull request Aug 30, 2026

ABI mismatch on powerpc64 (elfv1) for union containing floats #162011

Open

rustbot assigned beetrees Sep 8, 2026

folkertdev marked this pull request as ready for review September 8, 2026 17:21

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 8, 2026

beetrees requested changes Sep 8, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2026

beetrees reviewed Sep 8, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

folkertdev commented Aug 29, 2026

Uh oh!

beetrees Aug 29, 2026 • 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!

folkertdev Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

beetrees Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

folkertdev commented Aug 29, 2026

Uh oh!

RalfJung Aug 30, 2026 • 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!

RalfJung Aug 30, 2026 • 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!

uweigand Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

folkertdev commented Sep 8, 2026

Uh oh!

beetrees Sep 8, 2026 • 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!

beetrees Sep 8, 2026 • 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!

beetrees Sep 8, 2026 • 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!

RalfJung Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

uweigand Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 8, 2026

Uh oh!

beetrees Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

beetrees Aug 29, 2026 •

edited by rustbot

Loading

beetrees Aug 29, 2026 •

edited

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

beetrees Sep 8, 2026 •

edited by rustbot

Loading

beetrees Sep 8, 2026 •

edited by rustbot

Loading

beetrees Sep 8, 2026 •

edited by rustbot

Loading

beetrees Sep 8, 2026 •

edited

Loading