Skip to content

Match LLVM ABI in extern "C" functions for f128 on Windows #128388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Match LLVM ABI in extern "C" functions for f128 on Windows #128388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions compiler/rustc_target/src/abi/call/x86_win64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::abi::call::{ArgAbi, FnAbi, Reg};
use crate::abi::Abi;
use crate::abi::{Abi, Float, Primitive};

// Win64 ABI: https://docs.microsoft.com/en-us/cpp/build/parameter-passing

Expand All @@ -18,8 +18,12 @@ pub fn compute_abi_info<Ty>(fn_abi: &mut FnAbi<'_, Ty>) {
// FIXME(eddyb) there should be a size cap here
// (probably what clang calls "illegal vectors").
}
Abi::Scalar(_) => {
if a.layout.size.bytes() > 8 {
Abi::Scalar(scalar) => {
// Match what LLVM does for `f128` so that `compiler-builtins` builtins match up
// with what LLVM expects.
if a.layout.size.bytes() > 8
&& !matches!(scalar.primitive(), Primitive::Float(Float::F128))
{
a.make_indirect();
} else {
a.extend_integer_width_to(32);
Expand Down
4 changes: 3 additions & 1 deletion library/std/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ fn main() {
// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
("arm64ec", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86", "windows") => false,
("x86_64", "windows") => false,
Copy link
Contributor

@tgross35 tgross35 Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the ABI bug not present on 32-bit x86, i.e. x86 tests on windows would pass if symbols were available?

(Unfortunately the symbols aren't yet available even with the builtins update, #128358)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you can add a couple try-job: <ci job name>s to the bottom of the PR for any jobs that should be run as a test.

Copy link
Contributor Author

@beetrees beetrees Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 32-bit x86 Windows f16 is passed on the stack and returned in XMM registers and f128 is passed on the stack and returned indirectly. Both LLVM and GCC use this ABI, which is the same as the ABI used on other operating systems.

I've added some try-jobs to the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for confirming. That should be testable once builtins is working then.

// x86 has ABI bugs that show up with optimizations. This should be partially fixed with
// the compiler-builtins update. <https://github.com/rust-lang/rust/issues/123885>
("x86" | "x86_64", _) => false,
Expand Down Expand Up @@ -122,6 +122,8 @@ fn main() {
("nvptx64", _) => false,
// ABI unsupported <https://github.com/llvm/llvm-project/issues/41838>
("sparc", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86_64", "windows") => false,
// 64-bit Linux is about the only platform to have f128 symbols by default
(_, "linux") if target_pointer_width == 64 => true,
// Same as for f16, except MacOS is also missing f128 symbols.
Expand Down
39 changes: 39 additions & 0 deletions tests/assembly/x86_64-windows-float-abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//@ assembly-output: emit-asm
//@ compile-flags: -O
//@ only-windows
//@ only-x86_64

#![feature(f16, f128)]
#![crate_type = "lib"]

// CHECK-LABEL: second_f16
// CHECK: movaps %xmm1, %xmm0
// CHECK-NEXT: retq
#[no_mangle]
pub extern "C" fn second_f16(_: f16, x: f16) -> f16 {
x
}

// CHECK-LABEL: second_f32
// CHECK: movaps %xmm1, %xmm0
// CHECK-NEXT: retq
#[no_mangle]
pub extern "C" fn second_f32(_: f32, x: f32) -> f32 {
x
}

// CHECK-LABEL: second_f64
// CHECK: movaps %xmm1, %xmm0
// CHECK-NEXT: retq
#[no_mangle]
pub extern "C" fn second_f64(_: f64, x: f64) -> f64 {
x
}

// CHECK-LABEL: second_f128
// CHECK: movaps %xmm1, %xmm0
// CHECK-NEXT: retq
#[no_mangle]
pub extern "C" fn second_f128(_: f128, x: f128) -> f128 {
x
}
Loading