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

The logic here is shared between ABIs (extern "C", extern "Rust", etc.), so changing how ZSTs are handled here breaks compatibility with C.

We can carve out an exception for rustic ABIs specifically but I don't think that's relevant to this PR.

Are you sure? The logic in this file should not be used for "Rust".

The logic in this file is only for non-Rustic ABIs: Rustic ABIs use a separate code path that currently doesn't perform any target-specific adjustments on s390x.

Ah, I thought that only happened later on.

Still, this is not the right place to have custom, incompatible behavior for ZSTs.

The current behaviour is consistent with our current behaviour of not ignoring ZSTs in arguments if C doesn't: once #157973 is merged we'll probably make it so that only ZSTs with a non-trivial ABI matter, but that's for a future, more general PR.

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

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

rustbot · 2026-09-13T15:07:48Z

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 · 2026-09-13T16:56:39Z

@rustbot ready

beetrees · 2026-09-13T18:40:20Z


+    /// Finds the one field that is not a ZST.
+    /// Returns `None` if there are multiple non-ZST fields or only ZST-fields.
+    pub fn non_zst_field<C>(&self, cx: &C) -> Option<(FieldIdx, Self)>


Having non_zst_field and non_1zst_field with very similar names feels like an easy footgun for future ABI PRs. Maybe rename non_zst_field to non_zst_field_ignore_alignment (or similar) to make the distinction more obvious?

View changes since the review

beetrees · 2026-09-13T18:43:02Z

+#[repr(C)]
+struct Wrapper<T>(T);
+
+// CHECK: define void @plain_f16(half noundef %x)


I think these should be CHECK-LABEL, not CHECK?

View changes since the review

beetrees · 2026-09-13T18:52:04Z

+                Primitive::Float(Float::F32 | Float::F64) => true,
+                Primitive::Float(Float::F16 | Float::F128) => false,


While it doesn't implement VaArgSafe yet, f16 is passed in floating point registers.

Suggested change

Primitive::Float(Float::F32 | Float::F64) => true,

Primitive::Float(Float::F16 | Float::F128) => false,

Primitive::Float(Float::F16 | Float::F32 | Float::F64) => true,

Primitive::Float(Float::F128) => false,

View changes since the review

folkertdev · 2026-09-13T23:18:34Z

@rustbot ready

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 2 times, most recently from 2172749 to 9ae7ff3 Compare August 29, 2026 22:38

RalfJung reviewed Aug 30, 2026

View reviewed changes

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

RalfJung reviewed Aug 30, 2026

View reviewed changes

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

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

folkertdev force-pushed the single-fp-element branch from 2992d1d to 2df6a25 Compare September 13, 2026 15:07

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 13, 2026

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2026

beetrees requested changes Sep 13, 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 13, 2026

add non_zst_field helper

aadf13c

folkertdev force-pushed the single-fp-element branch from 2df6a25 to 0501e2b Compare September 13, 2026 19:38

beetrees requested changes Sep 13, 2026

View reviewed changes

Comment thread tests/codegen-llvm/x86-abi/single-fp-element.rs Outdated

folkertdev and others added 2 commits September 14, 2026 01:17

fix is_single_fp_element for s390x and x86

ee524a2

abi/compatibility test: add simple float type tests

33871b9

folkertdev force-pushed the single-fp-element branch from 0501e2b to 33871b9 Compare September 13, 2026 23:17

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 13, 2026

		Primitive::Float(Float::F32 \| Float::F64) => true,
		Primitive::Float(Float::F16 \| Float::F128) => false,

Uh oh!

Uh oh!

Conversation

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

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

folkertdev commented Sep 8, 2026

Uh oh!

Uh oh!

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!

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

Uh oh!

Uh oh!

rustbot commented Sep 13, 2026

Uh oh!

folkertdev commented Sep 13, 2026

Uh oh!

beetrees Sep 13, 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 13, 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 13, 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!

Uh oh!

folkertdev commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

folkertdev commented Aug 29, 2026 •

edited by rustbot

Loading

beetrees Aug 29, 2026 •

edited by rustbot

Loading

beetrees Aug 29, 2026 •

edited

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

beetrees Sep 8, 2026 •

edited by rustbot

Loading

beetrees Sep 13, 2026 •

edited by rustbot

Loading

beetrees Sep 13, 2026 •

edited by rustbot

Loading

beetrees Sep 13, 2026 •

edited by rustbot

Loading