Skip to content
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.

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

tests: adjust expectation for f128 abi on Windows #138182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

durin42
Copy link
Contributor

@durin42 durin42 commented Mar 7, 2025

llvm/llvm-project@5ee1c0b updates llvm to match the documented calling convention to pass f128 indirectly.

@rustbot label llvm-main

try-job: dist-x86_64-msvc
try-job: dist-x86_64-mingw
try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
try-job: x86_64-mingw-1
try-job: x86_64-mingw-2

llvm/llvm-project@5ee1c0b updates llvm
to match the documented calling convention to pass f128 indirectly.

@rustbot label llvm-main
@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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. llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) labels Mar 7, 2025
@Mark-Simulacrum
Copy link
Member

r? @tgross35

Looks OK but want to get someone more familiar with i128 to review.

@rustbot rustbot assigned tgross35 and unassigned Mark-Simulacrum Mar 15, 2025
@tgross35
Copy link
Contributor

I meant to send a patch once my LLVM change merged, but we should also update our codegen to match. Could you change

if is_ret && matches!(scalar.primitive(), Primitive::Int(Integer::I128, _)) {
if cx.target_spec().rustc_abi == Some(RustcAbi::X86Softfloat) {
// Use the native `i128` LLVM type for the softfloat ABI -- in other words, adjust nothing.
} else {
// `i128` is returned in xmm0 by Clang and GCC
// FIXME(#134288): This may change for the `-msvc` targets in the future.
let reg = Reg { kind: RegKind::Vector, size: Size::from_bits(128) };
a.cast_to(reg);
}
} else if a.layout.size.bytes() > 8
&& !matches!(scalar.primitive(), Primitive::Float(Float::F128))
{
// Match what LLVM does for `f128` so that `compiler-builtins` builtins match up
// with what LLVM expects.
a.make_indirect();
} else {
a.extend_integer_width_to(32);
}
so i128 and f128 use the same ABI? That probably means a codegen test update.

@@ -37,7 +37,7 @@ pub extern "C" fn second_f64(_: f64, x: f64) -> f64 {
}

// CHECK-LABEL: second_f128
// CHECK: movaps %xmm1, %xmm0
// CHECK: movaps {{(%xmm1|\(%rdx\))}}, %xmm0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add something like // FIXME(llvm21) remove xmm1 once Rust's LLVM has the ABI change so we clean this up later?

Cc @beetrees since iirc you authored this test

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually if we update our codegen to match i128, the options shouldn't be needed since it will always be (%rdx).

@tgross35 tgross35 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 Mar 17, 2025
@tgross35
Copy link
Contributor

You should be able to take the CC change from #137779.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants