Support u128/i128 c-variadic arguments#155429
Support u128/i128 c-variadic arguments#155429folkertdev wants to merge 2 commits intorust-lang:mainfrom
u128/i128 c-variadic arguments#155429Conversation
|
@bors try jobs=dist-various-1,dist-various-2,aarch64-apple,x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
Support `u128`/`i128` c-variadic arguments try-job: dist-various-1 try-job: dist-various-2 try-job: aarch64-apple try-job: x86_64-mingw-1
This comment has been minimized.
This comment has been minimized.
3f2ff65 to
5e8c8b5
Compare
|
@bors try jobs=dist-various-1,dist-various-2,aarch64-apple,x86_64-mingw-1 |
Support `u128`/`i128` c-variadic arguments try-job: dist-various-1 try-job: dist-various-2 try-job: aarch64-apple try-job: x86_64-mingw-1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5e8c8b5 to
bcf4cfa
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 Test for d3bd26f failed: CI. Failed job:
|
bcf4cfa to
8e7c3b1
Compare
|
@bors try jobs=dist-various-1,dist-various-2,aarch64-apple,x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
Support `u128`/`i128` c-variadic arguments try-job: dist-various-1 try-job: dist-various-2 try-job: aarch64-apple try-job: x86_64-mingw-1
8e7c3b1 to
d765f82
Compare
This comment has been minimized.
This comment has been minimized.
d765f82 to
1b40600
Compare
| crate::cfg_select! { | ||
| target_pointer_width = "32" => { | ||
| // GCC says <https://gcc.gnu.org/onlinedocs/gcc-15.2.0/gcc/_005f_005fint128.html> | ||
| // | ||
| // > There is no support in GCC for expressing an integer constant of type __int128 for targets | ||
| // > with long long integer less than 128 bits wide. | ||
| } | ||
| target_env = "msvc" => { | ||
| // Per https://learn.microsoft.com/en-us/cpp/cpp/data-type-ranges?view=msvc-170, __int128 is | ||
| // not recognized by msvc. | ||
| } | ||
| any( | ||
| target_arch = "x86_64", | ||
| target_arch = "aarch64", | ||
| target_arch = "riscv64", | ||
| target_arch = "loongarch64", | ||
| target_arch = "s390x", | ||
| target_arch = "powerpc64" | ||
| ) => { | ||
| unsafe impl VaArgSafe for i128 {} | ||
| unsafe impl VaArgSafe for u128 {} | ||
| } | ||
| _ => { | ||
| } | ||
| } |
There was a problem hiding this comment.
cc @Amanieu @joshtriplett @workingjubilee thoughts on this as an initial set of targets to support i128 varargs on?
| Primitive::Int(integer, _) => match integer { | ||
| Integer::I8 | Integer::I16 => unreachable!(), | ||
| Integer::I32 | Integer::I64 => { /* fall through */ } | ||
| Integer::I128 => return Align::EIGHT, |
There was a problem hiding this comment.
It has custom behavior, the va_arg implementation for powerpc64 is here
and it uses the implementation here
The logic falls through to the last line for i128, returning 8. For f128 it curiously does use an alignment of 16, so I've just added that already because it would be easy to miss down the line.
1b40600 to
aa85c8d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ccdec2f to
50b74c4
Compare
There was a problem hiding this comment.
This looks big but only really because I added a lot more assembly tests to check that the 128-bit implementation is correct. Due to its alignment it sometimes requires some additonal logic, so I thought it best to lock the behavior in via assembly tests.
r? tgross35
| let overflow_arg_area_v = if layout.layout.align.bytes() > 8 { | ||
| round_pointer_up_to_alignment(bx, overflow_arg_area_v, layout.layout.align.abi) | ||
| } else { | ||
| overflow_arg_area_v | ||
| }; |
There was a problem hiding this comment.
| //@ revisions: WINDOWS_GNU WINDOWS_MSVC | ||
| //@ [WINDOWS_GNU] compile-flags: -Copt-level=3 -Cllvm-args=-x86-asm-syntax=intel | ||
| //@ [WINDOWS_GNU] compile-flags: --target x86_64-pc-windows-gnu | ||
| //@ [WINDOWS_GNU] needs-llvm-components: x86 | ||
| //@ [WINDOWS_MSVC] compile-flags: -Copt-level=3 -Cllvm-args=-x86-asm-syntax=intel | ||
| //@ [WINDOWS_MSVC] compile-flags: --target x86_64-pc-windows-msvc | ||
| //@ [WINDOWS_MSVC] needs-llvm-components: x86 |
There was a problem hiding this comment.
Calling this out explicitly: we do implement the trait even on -msvc targets, because clang provides __int128 there. If MSVC itself ever did support the type (which, from my understanding, is unlikely), clang would kind of have to follow whatever it does, and we would too. Until then we're compatible with clang.
| // Clang and Rustc use a different ABI for i128 on wasm32, and LLVM optimizes differently if we use | ||
| // a mutable reference instead of just a pointer. With this setup we match the equivalent Clang | ||
| // input exactly. | ||
| #[unsafe(no_mangle)] | ||
| unsafe extern "C" fn read_i128(out: *mut i128, ap: *mut VaList<'_>) { |
There was a problem hiding this comment.
just calling attention to this difference. Based on #135532 we actually follow the spec here.
| //@ revisions: POWERPC POWERPC64 POWERPC64LE AIX | ||
| //@ [POWERPC] compile-flags: -Copt-level=3 --target powerpc-unknown-linux-gnu | ||
| //@ [POWERPC] needs-llvm-components: powerpc | ||
| //@ [POWERPC64] compile-flags: -Copt-level=3 --target powerpc64-unknown-linux-gnu | ||
| //@ [POWERPC64] needs-llvm-components: powerpc | ||
| //@ [POWERPC64LE] compile-flags: -Copt-level=3 --target powerpc64le-unknown-linux-gnu | ||
| //@ [POWERPC64LE] needs-llvm-components: powerpc | ||
| //@ [AIX] compile-flags: -Copt-level=3 --target powerpc64-ibm-aix | ||
| //@ [AIX] needs-llvm-components: powerpc |
There was a problem hiding this comment.
included because powerpc has some special behavior for i128, which is only aligned to 8 bytes in c-variadic arguments.
| // assembly. | ||
| // | ||
| // For aarch64-apple-darwin LLVM is able to optimize our output better, because we effectively | ||
| // desugar va_arg early, hence we don't actually match Clang there. |
There was a problem hiding this comment.
I played around with our codegen for a while, but LLVM is too clever. But, these are both tier-1 targets so we test the implementation versus C in CI, and the generated IR does look reasonable.
| // Implement `VaArgSafe` for 128-bit integers on targets where clang provides `__int128`. | ||
| // | ||
| // GCC does not implement `__int128` for any 16-bit/32-bit target: | ||
| // | ||
| // https://gcc.gnu.org/onlinedocs/gcc-15.2.0/gcc/_005f_005fint128.html | ||
| // | ||
| // > There is no support in GCC for expressing an integer constant of type __int128 for targets | ||
| // > with long long integer less than 128 bits wide. | ||
| // | ||
| // Per https://learn.microsoft.com/en-us/cpp/cpp/data-type-ranges?view=msvc-170, MSVC does not | ||
| // define `__int128`. | ||
| // | ||
| // Clang is slightly more permissive: it defines `__int128` on wasm32 (a 32-bit target) and also | ||
| // does provide `__int128` on 64-bit `*-pc-windows-msvc`, and we follow suit. |
There was a problem hiding this comment.
calling this out: the implementation now follows clang: if it defines __int128, we impl the trait.
There was a problem hiding this comment.
cc @Amanieu @joshtriplett if you have objections to that.
|
|
|
may as well @bors try jobs=dist-various-1,dist-various-2,aarch64-apple,x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
Support `u128`/`i128` c-variadic arguments try-job: dist-various-1 try-job: dist-various-2 try-job: aarch64-apple try-job: x86_64-mingw-1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
50b74c4 to
52d37d3
Compare
| ) -> &'ll Value { | ||
| if layout.layout.align.abi > src_align { | ||
| let tmp = bx.alloca(layout.layout.size(), layout.layout.align().abi); | ||
| let tmp = bx.alloca_with_ty(layout); |
There was a problem hiding this comment.
Using a typed alloca solves an issue with the memcpy not being optimized out, see llvm/llvm-project#194158. Normally rust type erases allocas (e.g. [16 x i8]) and that confuses LLVM apparently.
|
💔 Test for 9d4c015 failed: CI. Failed job:
|
52d37d3 to
a34dc03
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try jobs=dist-various-1,dist-various-2,aarch64-apple,x86_64-mingw-1 |
This comment has been minimized.
This comment has been minimized.
Support `u128`/`i128` c-variadic arguments try-job: dist-various-1 try-job: dist-various-2 try-job: aarch64-apple try-job: x86_64-mingw-1
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
On platforms where `clang` defines `__int128`.
a34dc03 to
6f9049b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
View all comments
The restriction on
u128is kind of arbitrary, so let's see what it would take to support it.r? @ghost