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

RalfJung · 2026-08-29T06:32:35Z

    C: HasDataLayout + HasTargetSpec,
 {
    if !fn_abi.ret.is_ignore() {
+        // "vectorcall" returns floats in `xmm0`, and soft float also does not use the x87 stack.


vectorcall does not show up in the code below, so this comment is a bit confusing.

I've flipped the condition to make it more obvious what the comment is referring to.

RalfJung · 2026-08-29T06:35:17Z

Shouldn't the FIXME and if be removed as this PR is supposed to fix exactly that?

This FIXME isn't for the ABI issue, this is for the related but separate "i586 targets without SSE have horribly broken floats (#114479)". While this PR does fix the C ABI, it mostly doesn't have any noticeable affect on targets without SSE as LLVM will load f32/f64 into x87 registers (quietening NaNs) whenever it want to load them into a register, so f32/f64 are quietened mid function.

I've updated the test so it now runs on i586 targets while allowing NaNs to be quitened only on them to make it more obvious what's going on.

Oh I see.

I was looking for a pure-Rust test for the change; it shouldn't be necessary to use C and FFI for that, should it?

The "C" (and other ABI) test cases I added in this PR to return-float.rs are a pure-Rust test for this change: the version of return-float.rs in this PR would fail on i686-* targets before this PR (before this PR return-float.rs only tested the "Rust" ABI). After this PR, x87-related float miscompilations will be entirely limited to the non-SSE i586-* targets.

RalfJung · 2026-08-29T06:37:31Z

+    /// 32-bit x86. This is needed as LLVM needs to generate extra code to ensure that signalling
+    /// NaNs are passed losslessly on the x87 floating point stack. Only valid on return types on
+    /// 32-bit x86 with a cast target of either `Reg::f32()` or `Reg::f64()`.
+    pub x87_floating_point_stack: bool,


This should mention that the LLVM-level type is then changed to x86_fp80.

RalfJung · 2026-08-29T07:02:07Z

+        // any initialized bytes within a partially uninitialized value survive the round trip,
+        // freeze the value.
+        let value = if no_undef { value } else { self.freeze(value) };
+        let is_nan = self.fcmp(RealPredicate::RealUNO, value, value);


Wouldn't the ideal test be "is it a signaling NaN"? Or do normal NaNs also get garbled?
Though I can imagine that that's annoying enough to implement that it's not worth it.

View changes since the review

Only signalling NaNs get quietened. I went with just checking for NaN as that is a single-instruction operation, whereas checking if a value is a signalling NaN requires many bit operations (more the larger the float). As the vast majority of values seem likely to be not NaNs at all, it felt more important to prioritise the fast path for non-NaNs.

Makes sense. It's also unlikely LLVM would ever know "this is not a signaling NaN" (so the more complicated condition will also not have the branch eliminated more often).

Please add a comment explaining this.

folkertdev · 2026-08-29T15:06:24Z

+        let (fraction_bits, fraction) =
+            if num_bits != 64 { (23, self.zext(bits, self.type_i64())) } else { (52, bits) };


Is this clearer as

Suggested change

let (fraction_bits, fraction) =

if num_bits != 64 { (23, self.zext(bits, self.type_i64())) } else { (52, bits) };

let (fraction_bits, fraction) = match num_bits {

32 => (f32::MANTISSA_DIGITS - 1, self.zext(bits, self.type_i64())),

64 => (f64::MANTISSA_DIGITS - 1, bits),

_ => bug!(),

};

View changes since the 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:13:06Z

+        let fraction_bits = match num_bits {
+            32 => 23,
+            64 => 52,
+            _ => bug!("attempt to return float on x87 floating point stack with width {num_bits}"),
+        };


again https://doc.rust-lang.org/std/primitive.f64.html#associatedconstant.MANTISSA_DIGITS etc. might be useful here.

View changes since the review

folkertdev · 2026-08-29T15:18:48Z

        dst: PlaceRef<'tcx, Self::Value>,
    );
+    /// Losslessly convert a `f32` or `f64` to a float to be returned on the x87 floating point
+    /// stack. As `MaybeUninit` floats are returned on the floating point stack, `value` being


Suggested change

/// stack. As `MaybeUninit` floats are returned on the floating point stack, `value` being

/// stack. Because `MaybeUninit` floats are also returned on the floating point stack, `value` being

Maybe this comment should also mention what this is for?

View changes since the review

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

rustbot assigned tgross35 Aug 28, 2026