Windows x86: Change i128 to return via the vector ABI #134290

tgross35 · 2024-12-14T01:27:09Z

Clang and GCC both return i128 in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar i128s using the vector ABI, which makes our i128 compatible with C.

In the future, Clang may change to return i128 on the stack for its -msvc targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW.

Link: #134288 (does not fix) [1]

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

rustbot · 2024-12-14T01:27:18Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
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

tgross35 · 2024-12-14T01:28:52Z

This should make us consistent for now, even if Clang isn't yet doing the best thing on MSVC.

The third commit's diff shows how the tests change.

Cc @beetrees @wesleywiser

tgross35 · 2024-12-14T01:35:10Z

@bors try

Windows x86: Change i128 to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: rust-lang#134288 (does not fix) try-job: x86_64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2

bors · 2024-12-14T01:36:21Z

⌛ Trying commit 706db2b with merge 4c26894...

compiler/rustc_target/src/callconv/x86_win64.rs

bors · 2024-12-14T02:32:21Z

💔 Test failed - checks-actions

tgross35 · 2024-12-14T03:08:20Z

I validated the codegen on a non-windows machine - it looks like the default filecheck labels get applied based on the host rather than the target? That’s interesting.

bjorn3 · 2024-12-14T06:09:21Z

Please double check that this doesn't break ABI compat between cg_llvm and cg_clif. And

rust/compiler/rustc_codegen_cranelift/src/abi/mod.rs

Line 128 in 4a204be

// Pass i128 arguments by-ref on Windows.

will almost certainly need to be adjusted too.

tgross35 · 2024-12-14T20:25:00Z

Please double check that this doesn't break ABI compat between cg_llvm and cg_clif. And

rust/compiler/rustc_codegen_cranelift/src/abi/mod.rs

Line 128 in 4a204be

// Pass i128 arguments by-ref on Windows.

will almost certainly need to be adjusted too.

That link looks to be for libcalls but those have to already be correct on cranelift, right? We have a hack to use Clang/GCC's return ABI in builtins https://github.com/rust-lang/compiler-builtins/blob/a09218f1c4d23ffbd97d68f0fefb5feed2469dc5/src/macros.rs#L583C9-L583C30.

What is the best way to verify Cranelift's ABI? If asm tests can run with both backends then I can add that (also does this get covered in one of the CI jobs?)

bjorn3 · 2024-12-14T21:26:09Z

__rust_i128_mulo and __rust_u128_mulo do change ABI with this PR:

rust/compiler/rustc_codegen_cranelift/src/codegen_i128.rs

Line 94 in 4a204be

if is_signed { "__rust_i128_mulo" } else { "__rust_u128_mulo" },

Adjusting the lib_call function directly would also allow getting rid of the if condition at

rust/compiler/rustc_codegen_cranelift/src/codegen_i128.rs

Line 36 in 4a204be

if fx.tcx.sess.target.is_like_windows {

which currently handles the adjustments for non-rust intrinsics.

tgross35 · 2024-12-14T21:41:05Z

__rust_i128_mulo and __rust_u128_mulo do change ABI with this PR:

rust/compiler/rustc_codegen_cranelift/src/codegen_i128.rs

Line 94 in 4a204be

if is_signed { "__rust_i128_mulo" } else { "__rust_u128_mulo" },

I don't think these should change since they return an aggregate type https://github.com/rust-lang/compiler-builtins/blob/a09218f1c4d23ffbd97d68f0fefb5feed2469dc5/src/int/mul.rs#L131, and the change here is only for scalars. Testing with this PR the IR signature for that is:

define void @__rust_i128_mulo(ptr dead_on_unwind noalias nocapture noundef writable writeonly sret([32 x i8]) align 16 dereferenceable(32) %_0, ptr noalias nocapture noundef readonly align 16 dereferenceable(16) %a, ptr noalias nocapture noundef readonly align 16 dereferenceable(16) %b) unnamed_addr #0`

I can make the adjustments to lib_call. But does this handle all extern calls in Cranelift, or is there a separate place that I should also update for non-libcall extern "C"?

workingjubilee · 2024-12-14T21:54:35Z

huh, why do we rely on certain tuples having a fixed meaning in the C ABI?

tgross35 · 2024-12-14T22:11:20Z

Good question :) Maybe those should change to a C-like fn(a: &mut i128, b: i128) -> bool at some point?

rustbot · 2024-12-15T08:19:03Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

tgross35 · 2024-12-15T08:25:07Z

@bors try

tgross35 · 2024-12-15T08:42:39Z

@bors ping

bors · 2024-12-15T08:42:42Z

😪 I'm awake I'm awake

bors · 2024-12-15T08:43:52Z

⌛ Trying commit d5bc586 with merge 31dc001...

Windows x86: Change i128 to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: rust-lang#134288 (does not fix) try-job: x86_64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2

tgross35 · 2025-01-27T12:20:04Z

Nopt had an additional load/store that caused the patterns to not match. Enabled opt-level=1 for the test, passes locally.

@bors r=bjorn3,wesleywiser

bors · 2025-01-27T12:20:07Z

📌 Commit a44a20e has been approved by bjorn3,wesleywiser

It is now in the queue for this repository.

…bjorn3,wesleywiser Windows x86: Change i128 to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: rust-lang#134288 (does not fix) [1] try-job: x86_64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2

Rollup of 9 pull requests Successful merges: - rust-lang#133151 (Trim extra whitespace in fn ptr suggestion span) - rust-lang#133929 (Remove -Zinline-in-all-cgus and clean up tests/codegen-units/) - rust-lang#134290 (Windows x86: Change i128 to return via the vector ABI) - rust-lang#135886 (Document purpose of closure in from_fn.rs more clearly) - rust-lang#136012 (Document powf and powi values that are always 1.0) - rust-lang#136104 (Add mermaid graphs of NLL regions and SCCs to polonius MIR dump) - rust-lang#136117 (Subtree update of `rust-analyzer`) - rust-lang#136143 (Update books) - rust-lang#136153 (Locate asan-odr-win with other sanitizer tests) r? `@ghost` `@rustbot` modify labels: rollup

…bjorn3,wesleywiser Windows x86: Change i128 to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: rust-lang#134288 (does not fix) [1] try-job: x86_64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2

Rollup of 7 pull requests Successful merges: - rust-lang#133151 (Trim extra whitespace in fn ptr suggestion span) - rust-lang#134290 (Windows x86: Change i128 to return via the vector ABI) - rust-lang#135886 (Document purpose of closure in from_fn.rs more clearly) - rust-lang#136012 (Document powf and powi values that are always 1.0) - rust-lang#136104 (Add mermaid graphs of NLL regions and SCCs to polonius MIR dump) - rust-lang#136143 (Update books) - rust-lang#136153 (Locate asan-odr-win with other sanitizer tests) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2025-01-28T06:11:16Z

⌛ Testing commit a44a20e with merge 66d6064...

bors · 2025-01-28T08:51:55Z

☀️ Test successful - checks-actions
Approved by: bjorn3,wesleywiser
Pushing 66d6064 to master...

rust-timer · 2025-01-28T10:08:14Z

Finished benchmarking commit (66d6064): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.0%, secondary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-2.0%	[-3.1%, -0.8%]	2
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-2.0%	[-3.1%, -0.8%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 772.501s -> 773.228s (0.09%)
Artifact size: 328.24 MiB -> 328.20 MiB (-0.01%)

…orn3,wesleywiser Windows x86: Change i128 to return via the vector ABI Clang and GCC both return `i128` in xmm0 on windows-msvc and windows-gnu. Currently, Rust returns the type on the stack. Add a calling convention adjustment so we also return scalar `i128`s using the vector ABI, which makes our `i128` compatible with C. In the future, Clang may change to return `i128` on the stack for its `-msvc` targets (more at [1]). If this happens, the change here will need to be adjusted to only affect MinGW. Link: rust-lang#134288 (does not fix) [1] try-job: x86_64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2

RalfJung · 2025-02-16T18:57:18Z

tests/codegen/i128-x86-callconv.rs

+//@ [MSVC] compile-flags: --target x86_64-pc-windows-msvc
+//@ [MINGW] compile-flags: --target x86_64-pc-windows-gnu
+//@ [MSVC] filecheck-flags: --check-prefix=WIN
+//@ [MINGW] filecheck-flags: --check-prefix=WIN


What is the point of this --check-prefix=WIN? The test can just use the CHECK prefix for annotations that should be checked in all revisions.

I just did this so we could could add non-windows targets in the same file in the future.

...which I see you did in #137094 :)

Ah, great, I did exactly that in #137094. :) However I had to rediscover this as it wasn't clear at all from the test. filecheck-flags: --check-prefix is a super neat trick for making these tests nicer, but I have no idea how anyone is supposed to know about that... it doesn't seem mentioned in the dev guide either?

Tbh I only thought to try it because I noticed LLVM's tests always set --check-prefix, seems like it's barely used in our suite since bootstrap does it for revisions automatically. I think this hasn't been done much in the past because #![no_core] is pretty awkward to use, but since minicore was added recently this is much better.

…r=tgross35 x86_win64 ABI: do not use xmm0 with softfloat ABI This adjusts rust-lang#134290 to not apply the new logic to targets marked as "softfloat". That fixes most instances of the issue brought up [here](rust-lang#116558 (comment)). r? `@tgross35`

Rollup merge of rust-lang#137094 - RalfJung:softfloat-means-no-simd, r=tgross35 x86_win64 ABI: do not use xmm0 with softfloat ABI This adjusts rust-lang#134290 to not apply the new logic to targets marked as "softfloat". That fixes most instances of the issue brought up [here](rust-lang#116558 (comment)). r? `@tgross35`

Rust's 128-bit integers have historically been incompatible with C [1]. However, there have been a number of changes in Rust and LLVM that mean this is no longer the case: * Incorrect alignment of `i128` on x86 [1]: adjusting Rust's alignment proposed at rust-lang/compiler-team#683, implemented at rust-lang#116672. * LLVM version of the above: resolved in LLVM, including ABI fix. Present in LLVM18 (our minimum supported version). * Incorrect alignment of `i128` on 64-bit PowerPC, SPARC, and MIPS [2]: Rust's data layouts adjusted at rust-lang#132422, rust-lang#132741, rust-lang#134115. * LLVM version of the above: done in LLVM 20 llvm/llvm-project#102783. * Incorrect return convention of `i128` on Windows: adjusted to match GCC and Clang at rust-lang#134290. At [3], the lang team considered it acceptable to remove `i128` from `improper_ctypes_definitions` if the LLVM version is known to be compatible. Time has elapsed since then and we have dropped support for LLVM versions that do not have the x86 fixes, meaning a per-llvm-version lint should no longer be necessary. The PowerPC, SPARC, and MIPS changes only came in LLVM 20 but since Rust's datalayouts have also been updated to match, we will be using the correct alignment regardless of LLVM version. Closes: rust-lang#134288 Closes: rust-lang#128950 [1]: rust-lang#54341 [2]: rust-lang#128950 [3]: rust-lang/lang-team#255 (comment)

x86_win64 ABI: do not use xmm0 with softfloat ABI This adjusts rust-lang/rust#134290 to not apply the new logic to targets marked as "softfloat". That fixes most instances of the issue brought up [here](rust-lang/rust#116558 (comment)). r? `@tgross35`

rustbot assigned SparrowLii Dec 14, 2024

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. labels Dec 14, 2024

workingjubilee reviewed Dec 14, 2024

View reviewed changes

compiler/rustc_target/src/callconv/x86_win64.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bors 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 Dec 14, 2024

tgross35 force-pushed the windows-i128-callconv branch from 706db2b to d5bc586 Compare December 15, 2024 08:18

rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 15, 2024

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 Jan 27, 2025

Zalathar mentioned this pull request Jan 28, 2025

Rollup of 9 pull requests #136165

Closed

Zalathar mentioned this pull request Jan 28, 2025

Rollup of 7 pull requests #136169

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 28, 2025

bors merged commit 66d6064 into rust-lang:master Jan 28, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 28, 2025

tgross35 deleted the windows-i128-callconv branch January 28, 2025 08:52

tgross35 mentioned this pull request Jan 28, 2025

Rust's i128 return ABI does not agree with Clang and GCC on Windows targets #134288

Open

This was referenced Feb 15, 2025

The extern "C" ABI of SIMD vector types depends on target features (tracking issue for abi_unsupported_vector_types future-incompatibility lint) #116558

Open

x86_win64 ABI: do not use xmm0 with softfloat ABI #137094

Merged

RalfJung reviewed Feb 16, 2025

View reviewed changes

tgross35 mentioned this pull request Feb 17, 2025

x86-64: Some extern "C" functions use U64x2 on a softfloat target, which makes no sense rust-lang/compiler-builtins#758

Closed

RalfJung mentioned this pull request Feb 18, 2025

remove win64_128bit_abi_hack rust-lang/compiler-builtins#759

Merged

tgross35 mentioned this pull request Feb 20, 2025

Update compiler-builtins to 0.1.147 #137297

Merged

tgross35 mentioned this pull request Feb 20, 2025

Remove i128 and u128 from improper_ctypes_definitions #137306

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows x86: Change i128 to return via the vector ABI #134290

Windows x86: Change i128 to return via the vector ABI #134290

tgross35 commented Dec 14, 2024 •

edited

Loading

rustbot commented Dec 14, 2024

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024

bors commented Dec 14, 2024

This comment has been minimized.

bors commented Dec 14, 2024

tgross35 commented Dec 14, 2024

bjorn3 commented Dec 14, 2024

tgross35 commented Dec 14, 2024 •

edited

Loading

bjorn3 commented Dec 14, 2024

tgross35 commented Dec 14, 2024 •

edited

Loading

workingjubilee commented Dec 14, 2024

tgross35 commented Dec 14, 2024 •

edited

Loading

rustbot commented Dec 15, 2024

tgross35 commented Dec 15, 2024

tgross35 commented Dec 15, 2024

bors commented Dec 15, 2024

bors commented Dec 15, 2024

tgross35 commented Jan 27, 2025

bors commented Jan 27, 2025

bors commented Jan 28, 2025

bors commented Jan 28, 2025

rust-timer commented Jan 28, 2025

RalfJung Feb 16, 2025

tgross35 Feb 17, 2025

tgross35 Feb 17, 2025

RalfJung Feb 17, 2025

tgross35 Feb 20, 2025

Windows x86: Change i128 to return via the vector ABI #134290

Windows x86: Change i128 to return via the vector ABI #134290

Conversation

tgross35 commented Dec 14, 2024 • edited Loading

rustbot commented Dec 14, 2024

tgross35 commented Dec 14, 2024 • edited Loading

tgross35 commented Dec 14, 2024

bors commented Dec 14, 2024

This comment has been minimized.

bors commented Dec 14, 2024

tgross35 commented Dec 14, 2024

bjorn3 commented Dec 14, 2024

tgross35 commented Dec 14, 2024 • edited Loading

bjorn3 commented Dec 14, 2024

tgross35 commented Dec 14, 2024 • edited Loading

workingjubilee commented Dec 14, 2024

tgross35 commented Dec 14, 2024 • edited Loading

rustbot commented Dec 15, 2024

tgross35 commented Dec 15, 2024

tgross35 commented Dec 15, 2024

bors commented Dec 15, 2024

bors commented Dec 15, 2024

tgross35 commented Jan 27, 2025

bors commented Jan 27, 2025

bors commented Jan 28, 2025

bors commented Jan 28, 2025

rust-timer commented Jan 28, 2025

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading

tgross35 commented Dec 14, 2024 •

edited

Loading