Windows x86: Change i128 to return via the vector ABI by tgross35 · Pull Request #134290 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation63 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request
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 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: 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
This comment has been minimized.
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
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.
__rust_i128_mulo and __rust_u128_mulo do change ABI with this PR:
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"?
huh, why do we rely on certain tuples having a fixed meaning in the C ABI?
Good question :) Maybe those should change to a C-like fn(a: &mut i128, b: i128) -> bool at some point?
Area: The compiletest test runner
Area: The testsuite used to check the correctness of rustc
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
Some changes occurred in src/tools/compiletest
cc @jieyouxu
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
bors added a commit to rust-lang-ci/rust that referenced this pull request
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 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: 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
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
📌 Commit a44a20e has been approved by bjorn3,wesleywiser
It is now in the queue for this repository.
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
Zalathar added a commit to Zalathar/rust that referenced this pull request
…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 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: 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
bors added a commit to rust-lang-ci/rust that referenced this pull request
Zalathar added a commit to Zalathar/rust that referenced this pull request
…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 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: 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
bors added a commit to rust-lang-ci/rust that referenced this pull request
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
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%)
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request
…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 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: 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
This was referenced
Feb 15, 2025
| //@ [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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did this so we could could add non-windows targets in the same file in the future.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...which I see you did in #137094 :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…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-timer added a commit to rust-lang-ci/rust that referenced this pull request
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
tgross35 added a commit to tgross35/rust that referenced this pull request
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
i128on 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
i128on 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
i128on 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
[3]: rust-lang/lang-team#255 (comment)
BoxyUwU pushed a commit to BoxyUwU/rustc-dev-guide that referenced this pull request
Labels
This PR was explicitly merged by bors.
Operating system: Windows
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.