Return f32 and f64 in XMM0 instead of FP0 on i686 #115919

GuentherVIII · 2023-09-17T19:46:08Z

i686 already uses SSE to do calculations with f32 and f64, but the C calling convention uses the x87 stack to return values. The Rust calling convention does not need to do this, and LLVM makes it easy to use XMM0 instead, which saves move instructions and fixes problems with NaN values.

See #115567, #91399 (comment) and this LLVM comment.

rustbot · 2023-09-17T19:46:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

the8472 · 2023-09-18T20:19:33Z

library/std/src/f32/tests.rs

-// Ignore test on x87 floating point, these platforms do not guarantee NaN
-// payloads are preserved and flush denormals to zero, failing the tests.
-#[cfg(not(target_arch = "x86"))]


I think the tests still should be ignored on pre-SSE x86 targets. We have a bunch of i586 tier2 targets and others might be running tests for them.

Done. Running the i586 tests also required changes to rustc_ty_utils which I'm not happy with. I could not figure out how to properly detect SSE2 support - target.features is empty on i586-unknown-linux-gnu at that point.

GuentherVIII · 2023-09-27T21:11:22Z

@rustbot label +A-abi +A-floating-point +O-x86 -T-libs

chorman0773 · 2023-10-03T02:16:03Z

Can I ask that we not make floating-point ABI even more complicated on x86?
Rustc already has ABI issues with floats and -x87, adding +sse to the list of features replied upon for abi is just going to make that problem worse.

Not to mention my headache trying to catalog all of the different x86 float ABIs.

chorman0773 · 2023-10-03T02:45:22Z

@rust-lang/opsem and probably @rust-lang/lang definitely needs to look at this for the reason in #116344.

When compiling for i586, local #[target_feature] attributes can likely affect whether an f32 or f64 return value is passed in an SSE or x87 register according to the code from this PR from what I can tell. The setting for the callee is not respected for the caller, at least in the existing cases affecting by this. This further constrains what can be promised in #115476.

workingjubilee · 2023-10-03T02:49:39Z

Can I ask that we not make floating-point ABI even more complicated on x86?
Rustc already has ABI issues with floats and -x87, adding +sse to the list of features replied upon for abi is just going to make that problem worse.

Not to mention my headache trying to catalog all of the different x86 float ABIs.

Given that we are also discussing making SSE/SSE2 mandatory for i686 targets with SSE2 as part of their baseline features, I think we should forge ahead, because this actually makes it more like x86-64's ABI.

programmerjake · 2023-10-03T03:05:21Z

compiler/rustc_ty_utils/src/abi.rs

@@ -371,6 +371,8 @@ fn fn_abi_new_uncached<'tcx>(
    let target = &cx.tcx.sess.target;
    let target_env_gnu_like = matches!(&target.env[..], "gnu" | "musl" | "uclibc");
    let win_x64_gnu = target.os == "windows" && target.arch == "x86_64" && target.env == "gnu";
+    let x86_sse2 =
+        target.arch == "x86" && (target.features.contains("sse2") || target.cpu == "pentium4");


I expect you'd want to query the sse2 target feature rather than checking for the pentium4 cpu, since e.g. penryn also supports sse2 but isn't the pentium4. https://rust.godbolt.org/z/j7oPE6Wrf

also, what if there's a soft-float target for an OS kernel where sse2 would be disabled even if the cpu is pentium4 due to the kernel not saving SSE registers?

@GuentherVIII If you handle the aforementioned edge-cases appropriately, I believe we can get this approved. We want to only do this if the target is a 32-bit x86 CPU that has "baseline" features including SSE2, and preferably only with the function also satisfying the appropriate target feature (which we wish to make "always" but for now we can afford to check twice for correctness). This obviously includes Pentium 4 CPUs with no other target feature settings, but it won't include any that, say, are softfloat by default.

so, in other words, only if on x86-32 and SSE2 is implied by the target structs/json ignoring any -C target-features or -C target-cpu or other overrides.

Does rust currently ask LLVM for the target features implied by the target cpu? I could only find the code that creates the list for #[cfg(target_feature)], which takes command-line options into account.

I've changed the implementation to check that list instead of target.features and target.cpu. This has the advantage that the standard library can activate the NAN-returning test precisely when it passes, and deactivating SSE with -C target-features still works.

There's also a second commit introducing yet another target option to make this configurable in the target specification and enabling it in all "i686" targets except i686_unknown_uefi, which disables SSE. The standard

I do not think we should make the ABI dependant on #[target_feature] - unlike -C target-feature, the attribute cannot disable SSE. If someone disables SSE with -C target-feature and re-enables it for select functions with #[target_feature], this shouldn't result in incompatible function ABIs with the same function type.

the issue you'll run into with -C target-feature=-sse is that functions in your binary won't match the ABI of other code (such as std) with SSE2 enabled in the default target features

There's talk about forbidding going below the baseline. If you want less-than-baseline features you need a different target.

https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Float.20ABI.20hell/near/394473707

RalfJung · 2023-10-03T05:25:16Z

Rustc already has ABI issues with floats and -x87, adding +sse to the list of features replied upon for abi is just going to make that problem worse.

I think it just means we want to have both of these in the set of baseline features.

But it means we should maybe block this PR on having the concept of "baseline features" rolled out so people get at least a warning when they disable a feature that should not be disabled?

thomcc · 2023-10-04T02:30:29Z

r? compiler

…tion i686 already uses SSE2 to do calculations with f32 and f64, but the C calling convention uses the x87 stack to return values. The Rust calling convention does not need to do this, and LLVM makes it easy to use XMM0 instead, which saves move instructions and fixes problems with NaN values.

rustbot · 2023-10-08T01:13:43Z

These commits modify compiler targets.
(See the Target Tier Policy.)

Activating SSE2 with -C target-feature doesn't change the Rust ABI anymore, while deactivating it still does.

b-naber · 2023-10-13T10:55:38Z

r? compiler

petrochenkov · 2023-10-13T13:20:53Z

This should probably be blocked on #116584.
@rustbot blocked

bors · 2023-11-08T21:23:10Z

☔ The latest upstream changes (presumably #117716) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2024-07-16T15:14:39Z

Closing because #116584 was closed.

RalfJung · 2024-07-16T15:19:44Z

This got superseded by #123351 which avoids the ABI issue by returning things in-memory.

programmerjake · 2024-07-16T16:31:32Z

This got superseded by #123351 which avoids the ABI issue by returning things in-memory.

returning stuff in xmm0 is still better since it avoids extra load/store, so I think we still want this PR (or similar) but it's not as critical since the corrupting-float-returns bug is now fixed.

rustbot assigned thomcc Sep 17, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 17, 2023

the8472 reviewed Sep 18, 2023

View reviewed changes

GuentherVIII force-pushed the ssereturn branch from 2212887 to 19acbf4 Compare September 20, 2023 00:05

rustbot added A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic O-x86 and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 27, 2023

Muon mentioned this pull request Oct 3, 2023

add notes about non-compliant FP behavior on 32bit x86 targets #113053

Merged

chorman0773 mentioned this pull request Oct 3, 2023

The ABI of float types can be changed by -Ctarget-feature #116344

Open

programmerjake reviewed Oct 3, 2023

View reviewed changes

rustbot assigned b-naber and unassigned thomcc Oct 4, 2023

GuentherVIII force-pushed the ssereturn branch from 19acbf4 to 2b153e7 Compare October 8, 2023 01:13

This comment has been minimized.

Sign in to view

Enable usage of XMM0 per-target

b7dfd48

Activating SSE2 with -C target-feature doesn't change the Rust ABI anymore, while deactivating it still does.

GuentherVIII force-pushed the ssereturn branch from 2b153e7 to b7dfd48 Compare October 8, 2023 09:55

GuentherVIII mentioned this pull request Oct 9, 2023

Remove ability to disable some target features #116584

Closed

rustbot assigned petrochenkov and unassigned b-naber Oct 13, 2023

rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2023

Noratrieb added O-x86_32 Target: x86 processors, 32 bit (like i686-*) and removed O-x86-all labels Oct 25, 2023

beetrees mentioned this pull request Apr 23, 2024

Ensure floats are returned losslessly by the Rust ABI on 32-bit x86 #123351

Merged

petrochenkov closed this Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return f32 and f64 in XMM0 instead of FP0 on i686 #115919

Return f32 and f64 in XMM0 instead of FP0 on i686 #115919

GuentherVIII commented Sep 17, 2023 •

edited

Loading

rustbot commented Sep 17, 2023

the8472 Sep 18, 2023

GuentherVIII Sep 20, 2023

GuentherVIII commented Sep 27, 2023

chorman0773 commented Oct 3, 2023 •

edited

Loading

chorman0773 commented Oct 3, 2023

workingjubilee commented Oct 3, 2023

programmerjake Oct 3, 2023

workingjubilee Oct 3, 2023 •

edited

Loading

programmerjake Oct 3, 2023 •

edited

Loading

GuentherVIII Oct 8, 2023

programmerjake Oct 8, 2023

the8472 Oct 8, 2023

RalfJung commented Oct 3, 2023

thomcc commented Oct 4, 2023

rustbot commented Oct 8, 2023

This comment has been minimized.

b-naber commented Oct 13, 2023

petrochenkov commented Oct 13, 2023

bors commented Nov 8, 2023

petrochenkov commented Jul 16, 2024

RalfJung commented Jul 16, 2024 •

edited

Loading

programmerjake commented Jul 16, 2024

Return f32 and f64 in XMM0 instead of FP0 on i686 #115919

Return f32 and f64 in XMM0 instead of FP0 on i686 #115919

Conversation

GuentherVIII commented Sep 17, 2023 • edited Loading

rustbot commented Sep 17, 2023

the8472 Sep 18, 2023

Choose a reason for hiding this comment

GuentherVIII Sep 20, 2023

Choose a reason for hiding this comment

GuentherVIII commented Sep 27, 2023

chorman0773 commented Oct 3, 2023 • edited Loading

chorman0773 commented Oct 3, 2023

workingjubilee commented Oct 3, 2023

programmerjake Oct 3, 2023

Choose a reason for hiding this comment

workingjubilee Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

programmerjake Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

GuentherVIII Oct 8, 2023

Choose a reason for hiding this comment

programmerjake Oct 8, 2023

Choose a reason for hiding this comment

the8472 Oct 8, 2023

Choose a reason for hiding this comment

RalfJung commented Oct 3, 2023

thomcc commented Oct 4, 2023

rustbot commented Oct 8, 2023

This comment has been minimized.

b-naber commented Oct 13, 2023

petrochenkov commented Oct 13, 2023

bors commented Nov 8, 2023

petrochenkov commented Jul 16, 2024

RalfJung commented Jul 16, 2024 • edited Loading

programmerjake commented Jul 16, 2024

GuentherVIII commented Sep 17, 2023 •

edited

Loading

chorman0773 commented Oct 3, 2023 •

edited

Loading

workingjubilee Oct 3, 2023 •

edited

Loading

programmerjake Oct 3, 2023 •

edited

Loading

RalfJung commented Jul 16, 2024 •

edited

Loading