rust-lang / compiler-team

A home for compiler team planning documents, meeting minutes, and other such things.
https://rust-lang.github.io/compiler-team/
Apache License 2.0
388 stars 69 forks source link

Set alignment of `i128` to 128 bits for x86 #683

Closed tgross35 closed 1 year ago

tgross35 commented 1 year ago

Context

i128 and u128 have long had the incorrect alignment on x86_64. Correct alignment is 128 bits; however, Rust currently aligns these types to 32 bits due to a bug in LLVM. Clang has gotten around this LLVM bug by manually setting the alignment. This means that Rust cannot safely share an i128 with C, which is the reason for it raising the improper_ctypes_definitions lint.

There is also a second issue in LLVM where i128s are passed incorrectly at function calls when they spill out of registers. Clang did not have a workaround for this, meaning that code compiled by Clang and code compiled by GCC are not compatible when 128-bit integers need to be passed in memory. See https://github.com/rust-lang/rust/issues/54341 for additional context on these problems.

Just recently, the alignment issue was fixed in LLVM (https://reviews.llvm.org/D86310) as was the function call issue (https://reviews.llvm.org/D158169). These fixes should be released in LLVM 18, likely in 2024-Q1.

Proposal

Manually set the alignment of i128 to the correct 16 bytes with LLVM < 18 now, to match the workaround that Clang currently has, and pull D158196 into Rust's LLVM tree. This only needs to be done for x86 targets, essentially a hardcoded version of the layout string changes in https://reviews.llvm.org/D86310#change-9bvPKJH10jZC.

This means the following:

We can drop this workaround when support for LLVM 17 ends

Alternatives

Just wait for LLVM 18

This means that the same version of rustc would produce incompatible code when built with different LLVM versions.

Don't pull D158196

Adding D158196 means that rustc+LLVM17 will be 100% compatible with rustc+LLVM18, GCC, and Clang18. Not adding it would mean that we have a two step ABI change and a mix of compatibility.

Mentors or Reviewers

@nikic has been helping with the LLVM side @maurer has a WIP change at https://github.com/rust-lang/rust/pull/116672

Process

The main points of the Major Change Process are as follows:

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

rustbot commented 1 year ago

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

tgross35 commented 1 year ago

See also ongoing discussion at https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/LLVM.20alignment.20of.20i128

cc @joshtriplett

tgross35 commented 1 year ago

This was originally attempted at https://github.com/rust-lang/rust/pull/38871 but was not merged since we preferred a fix in LLVM. Now that LLVM has the fix, this MCP proposes making Rust change to match so we are consistent across all LLVM versions.

WaffleLapkin commented 1 year ago

@rustbot second

apiraino commented 1 year ago

@rustbot label -final-comment-period +major-change-accepted