Skip to content

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. This change makes us do that on all versions of LLVM, not just starting with LLVM 21.

@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

@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.

@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.

@durin42 durin42 force-pushed the llvm-21-fp128-windows branch from 697c81b to 8399166 Compare March 27, 2025 16:51
@durin42 durin42 changed the title tests: adjust expectation for f128 abi on Windows rustc_target: update x86_win64 to match the documented calling convention for f128 Mar 27, 2025
@tgross35
Copy link
Contributor

tgross35 commented Apr 1, 2025

@bors try

r=me once that passes

@bors
Copy link
Collaborator

bors commented Apr 1, 2025

⌛ Trying commit 8399166 with merge b83a6ea...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 1, 2025
rustc_target: update x86_win64 to match the documented calling convention for f128

llvm/llvm-project@5ee1c0b updates llvm to match the documented calling convention to pass f128 indirectly. This change makes us do that on all versions of LLVM, not just starting with LLVM 21.

`@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
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 1, 2025

💔 Test failed - checks-actions

@tgross35
Copy link
Contributor

tgross35 commented Apr 1, 2025

The test crash seems to indicate some kind of ABI mismatch. I am guessing this happens during the libcall to __eqtf2 or __unordtf2, but I am unsure why this would be since we don't do anything special with that ABI https://github.com/rust-lang/compiler-builtins/blob/f1b9055b0d09370a474ce5e9b9d78ac4758cedf8/compiler-builtins/src/float/cmp.rs#L181-L189.

@beetrees
Copy link
Contributor

beetrees commented Apr 2, 2025

Only LLVM 21+ uses the new indirect ABI; Rust currently ships with LLVM 20 which will use the old ABI when generating calls to functions in compiler-builtins.

@tgross35
Copy link
Contributor

tgross35 commented Apr 2, 2025

Ah right, I guess we can't switch yet without breaking things. @durin42 sorry for all the confusion here but I guess your original PR with {{(%xmm1|\(%rdx\))}} and FIXME(llvm21) was the best way to get ToT CI working.

If you want to put that into a separate PR, we can leave this one around until the LLVM21 update happens.

@durin42 durin42 force-pushed the llvm-21-fp128-windows branch from 8399166 to dd3de06 Compare April 10, 2025 19:27
llvm/llvm-project@5ee1c0b updates llvm
to match the documented calling convention to pass f128 indirectly.

@rustbot label llvm-main
@durin42 durin42 force-pushed the llvm-21-fp128-windows branch from dd3de06 to d7e7f8b Compare April 10, 2025 19:29
@durin42
Copy link
Contributor Author

durin42 commented Apr 10, 2025

I've updated this PR to go back to the test-only edit, with a FIXME added.

@tgross35
Copy link
Contributor

@bors try

@bors
Copy link
Collaborator

bors commented Apr 10, 2025

⌛ Trying commit d7e7f8b with merge e3493ce...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2025
rustc_target: update x86_win64 to match the documented calling convention for f128

llvm/llvm-project@5ee1c0b updates llvm to match the documented calling convention to pass f128 indirectly. This change makes us do that on all versions of LLVM, not just starting with LLVM 21.

`@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
@bors
Copy link
Collaborator

bors commented Apr 10, 2025

☀️ Try build successful - checks-actions
Build commit: e3493ce (e3493cef53b7d8d1e2429715806de32e08f372ab)

@tgross35
Copy link
Contributor

Sorry about the back and forth, thanks for the changes!

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 11, 2025

📌 Commit d7e7f8b has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2025
@durin42
Copy link
Contributor Author

durin42 commented Apr 11, 2025 via email

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#137447 (add `core::intrinsics::simd::{simd_extract_dyn, simd_insert_dyn}`)
 - rust-lang#138182 (rustc_target: update x86_win64 to match the documented calling convention for f128)
 - rust-lang#138682 (Allow drivers to supply a list of extra symbols to intern)
 - rust-lang#138904 (Test linking and running `no_std` binaries)
 - rust-lang#138998 (Don't suggest the use of  `impl Trait` in closure parameter)
 - rust-lang#139447 (doc changes: debug assertions -> overflow checks)
 - rust-lang#139469 (Introduce a `//@ needs-crate-type` compiletest directive)
 - rust-lang#139564 (Deeply normalize obligations in `BestObligation` folder)
 - rust-lang#139574 (bootstrap: improve `channel` handling)
 - rust-lang#139600 (Update `compiler-builtins` to 0.1.153)
 - rust-lang#139641 (Allow parenthesis around inferred array lengths)
 - rust-lang#139654 (Improve `AssocItem::descr`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 63df6f1 into rust-lang:master Apr 11, 2025
7 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 11, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2025
Rollup merge of rust-lang#138182 - durin42:llvm-21-fp128-windows, r=tgross35

rustc_target: update x86_win64 to match the documented calling convention for f128

llvm/llvm-project@5ee1c0b updates llvm to match the documented calling convention to pass f128 indirectly. This change makes us do that on all versions of LLVM, not just starting with LLVM 21.

`@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
@workingjubilee workingjubilee added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

8 participants