Ensure floats are returned losslessly by the C ABI on 32-bit x86 by beetrees · Pull Request #161950 · rust-lang/rust

beetrees · 2026-08-28T19:31:13Z

The x86 C ABI returns f32 and f64 using the x87 extended precision floating point format (henceforth x86_fp80) on the x87 floating point stack. Currently, LLVM uses regular fld/fstp instructions to convert f32/f64 to and from x86_fp80 when returning from a function with the C ABI, however these instructions perform floating point format conversions that quieten signalling NaNs, which breaks Rust's (and LLVM's) guarantees. This is a long-standing LLVM bug (llvm/llvm-project#66803) but is difficult to fix on the LLVM side as it ties into larger problems with the way LLVM handles x87 registers holding f32/f64, combined with the general lack of developer interest in 32-bit x86 without SSE (although the ABI bug specifically also affects targets with SSE2 enabled). This problem has been tracked on the Rust side in #115567, with #123351 ensuring that Rustic ABIs avoid the x87 stack altogether and #113053 adding notes to the platform support page detailing how the targets are non-complaint (including the tier 1 targets i686-unknown-linux-gnu and i686-pc-windows-msvc).

This PR fixes #115567 by manually converting NaNs to and from x86_fp80 when a function returns f32/f64 (this was briefly discussed in #t-compiler > `x87_f80` is weird @ 💬).

r? @tgross35
cc @RalfJung

try-job: i686-*

rustbot · 2026-08-28T19:31:19Z

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

folkertdev · 2026-08-28T19:43:57Z

@bors delegate try

rust-bors · 2026-08-28T19:43:59Z

✌️ @beetrees, you can now perform try builds on this pull request!

You can now post @bors try to start a try build.

beetrees · 2026-08-28T19:58:54Z

@bors try

Ensure floats are returned losslessly by the C ABI on 32-bit x86 try-job: i686-*

rust-bors · 2026-08-28T22:44:10Z

☀️ Try build successful (CI)
Build commit: a9e9507 (a9e95070267a88d3be44d2d95464f64d39bb751d)
Base parent: 17fd5b8 (17fd5b8a37b6667b6cc137f3cc35f09759768a3b)

RalfJung

I can't review this, but I love it. :) I see that there's a separate path for nnan, so if LLVM can infer nnan all the new code should optimize away. That might be worth a test as well if we don't already have one?

View changes since this review

folkertdev · 2026-08-29T15:11:39Z

+        let fraction = self.zext(fraction, self.type_ix(80));
+
+        let is_nan_res = self.or(exp_and_sign, fraction);
+        let is_nan_res = self.bitcast(is_nan_res, self.type_x86_fp80());


slightly confusing that this works, but apparently it does? (because when stored x86_fp80 would use 12 or 16 bytes).

View changes since the review

The LLVM x86_fp80 type is exactly 80 bits - padding is added when needed by Clang AFAICT:

https://github.com/llvm/llvm-project/blob/05da143c63d333420b99eab198e1a89c97480929/llvm/lib/IR/Type.cpp#L208

folkertdev · 2026-08-29T15:29:07Z

@@ -44,6 +72,12 @@ where
                // float aggregates directly in a floating-point register.
                if fn_abi.ret.layout.is_single_fp_element(cx) {


hmm, is this actually what we want for is_single_fp_element? Should it not also return true on f16 and f128 (or have a clearer name)?

rust/compiler/rustc_abi/src/layout/ty.rs

Lines 158 to 176 in ee4a4a6

pub fn is_single_fp_element<C>(self, cx: &C) -> bool

where

Ty: TyAbiInterface<'a, C>,

C: HasDataLayout,

{

match self.backend_repr {

BackendRepr::Scalar(scalar) => {

matches!(scalar.primitive(), Primitive::Float(Float::F32 | Float::F64))

}

BackendRepr::Memory { .. } => {

if self.fields.count() == 1 && self.fields.offset(0).bytes() == 0 {

self.field(cx, 0).is_single_fp_element(cx)

} else {

false

}

}

_ => false,

}

}

View changes since the review

Looking into it, it definitely isn't what we want, and at least on s390x #[repr(C)] struct Foo(f16) is passed incorrectly.

C https://godbolt.org/z/Ybb1WY4x4

Rust https://godbolt.org/z/xq5jzP1df

The rust version moves ldgr %f0, %r0 from a general into a float register. With the C compilers the value is already there.

I'll try to clean that up.

I've left it as is in this PR as I didn't want to put to many changes in one PR, but is_single_fp_element is definitely not correct here. Both Clang and GCC also match f16 on x86, and they also require the struct to have no padding (which isn't checked here at the moment). is_single_fp_element also doesn't handle #[repr(transparent)] structs containing 1ZST fields. is_single_fp_element is also used in the s390x ABI, where GCC and Clang compilers currently implement slightly different rules: padding is fine as long as the overall size if either 2, 4 bytes or 8 bytes. This appears to be GCC and Clang compilers violating the s390x ABI specification, which says "A structure equivalent to one of the above. A structure is equivalent to a type 𝑇 if and only if it has exactly one member, which is either of type 𝑇 itself or a structure equivalent to type 𝑇.", which implies that padding should actually be ignored.

(EDIT: That's strange, GitHub didn't show me your second comment until after I'd posted mine.)

Oh yeah is_single_fp_element is completely wrong.
Strange that tests/ui/abi/compatibility.rs didn't catch it, it's meant exactly for things like this. Even when I add an f32 test, which will check whether a repr(transparent) wrapper around f32 has the same ABI as f32, nothing fails: #161991

RalfJung · 2026-08-30T06:41:02Z

+    // CHECK: ucomiss [[XMM]], [[XMM]]
+    // CHECK: jp [[NAN_LABEL:.*]]
+    // CHECK: movss dword ptr [esp + [[#OFFSET:]]], [[XMM]]
+    // CHECK: fld dword ptr [esp + [[#OFFSET]]]
+    // CHECK: ret
+    // CHECK: [[NAN_LABEL]]:
+    // CHECK: movd [[BITS:.*]], [[XMM]]
+    // CHECK: mov dword ptr [esp + [[#OFFSET:]]], 0
+    // CHECK: mov e[[SIGN_AND_EXP:.*]], [[BITS]]
+    // CHECK: shl [[BITS]], 8
+    // CHECK: shr e[[SIGN_AND_EXP]], 16
+    // CHECK: mov dword ptr [esp + [[#OFFSET+4]]], [[BITS]]
+    // CHECK: or e[[SIGN_AND_EXP]], 32767
+    // CHECK: mov word ptr [esp + [[#OFFSET+8]]], [[SIGN_AND_EXP]]
+    // CHECK: fld tbyte ptr [esp + [[#OFFSET]]]
+    // CHECK: ret


Would it make sense to extract the NaN branch into a function to not blow up the assembly so much?

View changes since the review

It would definitely be possible. The downsides would be that if the function called in the NaN branch was compiled separately (e.g. compiler-builtins) LLVM wouldn't be able to constant fold NaNs, and it would require leaf functions to do more stack setup, negating some of the benefit (a quick check based on the IR for extern "C" fn f(x: f64) -> f64 { x } shows that calling a function in the NaN branch would save a net 4 instructions in that case). Given that extern "C" functions that return f32/f64 are rare in Rust programs (as opposed to C where extern "C" is the default ABI) I don't think it's worth the extra complexity (part of my aim with this PR was to minimise the extra compiler complexity that goes towards supporting 32-bit x86 as much as possible). For comparison, the VaList setup prelude of a extern "C" fn f(x: ...) { f2(x) } function on x86_64-unknown-linux-gnu takes over 20 instructions.

rustbot · 2026-08-30T17:24:57Z

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

Left some comments about the output, but I think we need an LLVM expert to check whether we're emitting the optimal IR here.

r? @nikic

Perhaps this could also be brought into LLVM in the future behind an optional flag?

Also cc from the other backends @bjorn3 @rust-lang/wg-gcc-backend in case there might be something applicable there.

View changes since this review

tgross35 · 2026-08-31T21:01:58Z

+pub extern "C" fn return_f32(x: f32) -> f32 {
+    // CHECK: movss [[XMM:.*]], dword ptr [{{esp|ebp}} + [[#]]]
+    // CHECK: ucomiss [[XMM]], [[XMM]]
+    // CHECK: jp [[NAN_LABEL:.*]]
+    // CHECK: movss dword ptr [esp + [[#OFFSET:]]], [[XMM]]
+    // CHECK: fld dword ptr [esp + [[#OFFSET]]]
+    // CHECK: ret
+    // CHECK: [[NAN_LABEL]]:
+    // CHECK: movd [[BITS:.*]], [[XMM]]
+    // CHECK: mov dword ptr [esp + [[#OFFSET:]]], 0
+    // CHECK: mov e[[SIGN_AND_EXP:.*]], [[BITS]]
+    // CHECK: shl [[BITS]], 8
+    // CHECK: shr e[[SIGN_AND_EXP]], 16
+    // CHECK: mov dword ptr [esp + [[#OFFSET+4]]], [[BITS]]
+    // CHECK: or e[[SIGN_AND_EXP]], 32767
+    // CHECK: mov word ptr [esp + [[#OFFSET+8]]], [[SIGN_AND_EXP]]
+    // CHECK: fld tbyte ptr [esp + [[#OFFSET]]]
+    // CHECK: ret


Would CHECK-DAG work rather than CHECK to ensure there are no extra instructions?

I've added CHECK-NEXT where possible, but there's there's stack setup at the start and before returns (as well as some .cfi_* directives) that vary by target and therefore would be verbose to check for exhaustively and make the test harder to read, so there's still some plain CHECKs.

tgross35 · 2026-08-31T21:05:02Z

 [`aarch64-apple-darwin`](platform-support/apple-darwin.md) | ARM64 macOS (11.0+, Big Sur+)
 [`aarch64-pc-windows-msvc`](platform-support/windows-msvc.md) | ARM64 Windows MSVC
 [`aarch64-unknown-linux-gnu`](platform-support/aarch64-unknown-linux-gnu.md) | ARM64 Linux (kernel 4.1+, glibc 2.17+)
-[`i686-pc-windows-msvc`](platform-support/windows-msvc.md) | 32-bit MSVC (Windows 10+, Windows Server 2016+, Pentium 4) [^x86_32-floats-return-ABI] [^win32-msvc-alignment]


Pehaps this is worth keeping around but updating that non-LLVM backends may have the bug?

That seems like something to put into the docs for those backends.

rustc_codegen_gcc has the issue, but I don't think we need to worry about mentioning it in the platform support documentation until rustc_codegen_gcc is available on stable. rustc_codegen_cranelift doesn't support 32-bit x86.

rustbot assigned tgross35 Aug 28, 2026

This comment has been minimized.

Sign in to view

beetrees force-pushed the x86-ret-snan-c branch from 9749c87 to 477f3c8 Compare August 28, 2026 19:50

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Aug 28, 2026

Auto merge of #161950 - beetrees:x86-ret-snan-c, r=<try>

a9e9507

Ensure floats are returned losslessly by the C ABI on 32-bit x86 try-job: i686-*

This comment has been minimized.

Sign in to view

RalfJung reviewed Aug 29, 2026

View reviewed changes

Comment thread compiler/rustc_target/src/callconv/x86.rs

Comment thread tests/ui/abi/numbers-arithmetic/return-float.rs Outdated

Comment thread compiler/rustc_target/src/callconv/mod.rs

RalfJung reviewed Aug 29, 2026

View reviewed changes

Comment thread compiler/rustc_codegen_llvm/src/abi.rs

beetrees force-pushed the x86-ret-snan-c branch 3 times, most recently from 68fdb80 to bb9b569 Compare August 29, 2026 14:05

folkertdev reviewed Aug 29, 2026

View reviewed changes

beetrees force-pushed the x86-ret-snan-c branch 2 times, most recently from 26bbbba to 552a4cb Compare August 29, 2026 16:34

This comment has been minimized.

Sign in to view

beetrees force-pushed the x86-ret-snan-c branch from 552a4cb to ec5132b Compare August 29, 2026 16:53

This comment has been minimized.

Sign in to view

beetrees force-pushed the x86-ret-snan-c branch from ec5132b to 0e8b455 Compare August 29, 2026 18:29

folkertdev mentioned this pull request Aug 29, 2026

fix is_single_fp_element for s390x and x86 #161987

Draft

RalfJung reviewed Aug 30, 2026

View reviewed changes

Comment thread tests/ui/abi/numbers-arithmetic/return-float.rs Outdated

RalfJung reviewed Aug 30, 2026

View reviewed changes

beetrees force-pushed the x86-ret-snan-c branch from 0e8b455 to 3381af6 Compare August 30, 2026 11:45

This comment has been minimized.

Sign in to view

beetrees force-pushed the x86-ret-snan-c branch from 3381af6 to ccba80a Compare August 30, 2026 17:24

beetrees force-pushed the x86-ret-snan-c branch 3 times, most recently from bad3872 to 6d19888 Compare August 31, 2026 14:31

tgross35 reviewed Aug 31, 2026

View reviewed changes

rustbot assigned nikic and unassigned tgross35 Aug 31, 2026

beetrees force-pushed the x86-ret-snan-c branch from 6d19888 to 0538d4f Compare September 3, 2026 13:25

Ensure floats are returned losslessly by the C ABI on 32-bit x86

5945bf9

beetrees force-pushed the x86-ret-snan-c branch from 0538d4f to 5945bf9 Compare September 3, 2026 13:34

		@@ -44,6 +72,12 @@ where
		// float aggregates directly in a floating-point register.
		if fn_abi.ret.layout.is_single_fp_element(cx) {

	pub fn is_single_fp_element<C>(self, cx: &C) -> bool
	where
	Ty: TyAbiInterface<'a, C>,
	C: HasDataLayout,
	{
	match self.backend_repr {
	BackendRepr::Scalar(scalar) => {
	matches!(scalar.primitive(), Primitive::Float(Float::F32 \| Float::F64))
	}
	BackendRepr::Memory { .. } => {
	if self.fields.count() == 1 && self.fields.offset(0).bytes() == 0 {
	self.field(cx, 0).is_single_fp_element(cx)
	} else {
	false
	}
	}
	_ => false,
	}
	}

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rustbot commented Aug 28, 2026

Uh oh!

This comment has been minimized.

folkertdev commented Aug 28, 2026

Uh oh!

rust-bors Bot commented Aug 28, 2026

Uh oh!

beetrees commented Aug 28, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Aug 28, 2026

Uh oh!

RalfJung 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

folkertdev 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!

Uh oh!

Uh oh!

folkertdev 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!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

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!

beetrees Aug 30, 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!

This comment has been minimized.

rustbot commented Aug 30, 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!

Uh oh!

Uh oh!

Uh oh!

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!

beetrees commented Aug 28, 2026 •

edited by rustbot

Loading

RalfJung left a comment •

edited by rustbot

Loading

folkertdev Aug 29, 2026 •

edited by rustbot

Loading

folkertdev Aug 29, 2026 •

edited by rustbot

Loading

beetrees Aug 29, 2026 •

edited

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

beetrees Aug 30, 2026 •

edited

Loading

tgross35 left a comment •

edited by rustbot

Loading