Windows x86: Change i128 to return via the vector ABI

[tgross35]

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]

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]

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]

@bors try

[bors]

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

[bors]

💔 Test failed - checks-actions

[tgross35]

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]

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]

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]

__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]

__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]

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

[tgross35]

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

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[tgross35]

@bors try

[tgross35]

@bors ping

[bors]

😪 I'm awake I'm awake

[bors]

⌛ Trying commit d5bc586 with merge 31dc001...

[bors]

☀️ Try build successful - checks-actions
Build commit: a35b226 (a35b22680f28823a88b3520a52f7839f35a8fef2)

[tgross35]

@bjorn3 I saw the mention in the abi-cafe issue, are you able to double check the cranelift changes here?

[bjorn3]

I don't have a Windows machine to test it on locally. Also I assumed that the __rust_u/i128_mulo change would land first. After that I think it would be possible to do a try build here and then in cg_clif's CI pull in the cg_clif changes and use rustup-toolchain-install-master to run against the try build.

[tgross35]

Oh, I was thinking they had to land the other way around. I’ll get that one ready to go then, but unfortunately updating compiler-builtins in r-l/rust is temporarily blocked on something like #13527 (or me reverting the libm submodule updates so the diagnostics don’t show up, but I’m hoping to not need that).

[bjorn3]

I think you missed a digit in the issue number.

[tgross35]

Sorry, #135278 to fix new libm traits showing up in diagnostics.

[tgross35]

I don't have a Windows machine to test it on locally. Also I assumed that the __rust_u/i128_mulo change would land first. After that I think it would be possible to do a try build here and then in cg_clif's CI pull in the cg_clif changes and use rustup-toolchain-install-master to run against the try build.

The mulo changes landed.

@bors try

[bors]

⌛ Trying commit 6acd4e6 with merge f168ac9...

[bors]

☀️ Try build successful - checks-actions
Build commit: f168ac9 (f168ac95ecc8e1903caa79ddf2515e8672e838ab)

[tgross35]

@bjorn3 what exactly needs to happen in the cg_cranelift repo to test this? I don't see any obvious point that the toolchain should be changed.

Also unsure whether I should have run a dist job.

[bjorn3]

#134338 didn't break anything in cg_clif's CI and the cg_clif changes in this PR LGTM.

@bors r=bjorn3,wesleywiser

[bors]

📌 Commit 2f13d43 has been approved by bjorn3,wesleywiser

It is now in the queue for this repository.

[matthiaskrgr]

@bors r- rollup=iffy
#135613 (comment)