Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set alignment of i128 to 128 bits for x86 #683

Closed
1 of 3 tasks
tgross35 opened this issue Oct 25, 2023 · 5 comments
Closed
1 of 3 tasks

Set alignment of i128 to 128 bits for x86 #683

tgross35 opened this issue Oct 25, 2023 · 5 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@tgross35
Copy link

tgross35 commented Oct 25, 2023 •
edited
Loading

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 rust-lang/rust#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:

  • Users will immediately get i128 compatibility with:
    • All versions of GCC
    • Clang >= 18
    • Compatibility with Clang < 18 for layout, but not for in-memory passing (since D158169 will fix us but old Clang is broken).
  • i128/u128 build files will not be compatible across the rustc version change where this lands
  • rustc built against LLVM < 18 that isn't from Rust's tree will mostly work with mainline rustc, with the exception of in-memory passing

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 rust-lang/rust#116672

Process

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

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

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.
@tgross35 tgross35 added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Oct 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2023

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

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Oct 25, 2023
@tgross35
Copy link
Author

tgross35 commented Oct 25, 2023 •
edited
Loading

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 tgross35 changed the title Set alignment of i128 to 128 bits Set alignment of i128 to 128 bits for x86 Oct 25, 2023
@tgross35
Copy link
Author

This was originally attempted at rust-lang/rust#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
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Oct 26, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
@apiraino
Copy link
Contributor

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

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Nov 15, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 12, 2023
@tgross35 tgross35 mentioned this issue Dec 13, 2023
Merged
maurer added a commit to maurer/rust that referenced this issue Dec 15, 2023
@maurer
LLVM 18 x86 data layout update
2f13a43 
With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to
16-bytes on x86 based platforms. This will be in LLVM-18. This patch
updates all our spec targets to be 16-byte aligned, and removes the
alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This alignment change was discussed in rust-lang/compiler-team#683

See rust-lang#54341 for additional information about why this is happening and
where this will be useful in the future.

This *does not* stabilize `i128`/`u128` for FFI.
@tgross35 tgross35 mentioned this issue Dec 15, 2023
Closed
maurer added a commit to maurer/rust that referenced this issue Jan 5, 2024
@maurer
LLVM 18 x86 data layout update
01c30a4 
With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to
16-bytes on x86 based platforms. This will be in LLVM-18. This patch
updates all our spec targets to be 16-byte aligned, and removes the
alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This alignment change was discussed in rust-lang/compiler-team#683

See rust-lang#54341 for additional information about why this is happening and
where this will be useful in the future.

This *does not* stabilize `i128`/`u128` for FFI.
nikic pushed a commit to nikic/rust that referenced this issue Jan 17, 2024
@maurer @nikic
LLVM 18 x86 data layout update
5586f4d 
With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to
16-bytes on x86 based platforms. This will be in LLVM-18. This patch
updates all our spec targets to be 16-byte aligned, and removes the
alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This alignment change was discussed in rust-lang/compiler-team#683

See rust-lang#54341 for additional information about why this is happening and
where this will be useful in the future.

This *does not* stabilize `i128`/`u128` for FFI.
cuviper pushed a commit to cuviper/rust that referenced this issue Jan 18, 2024
@maurer @cuviper
LLVM 18 x86 data layout update
253f856 
With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to
16-bytes on x86 based platforms. This will be in LLVM-18. This patch
updates all our spec targets to be 16-byte aligned, and removes the
alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This alignment change was discussed in rust-lang/compiler-team#683

See rust-lang#54341 for additional information about why this is happening and
where this will be useful in the future.

This *does not* stabilize `i128`/`u128` for FFI.
nikic pushed a commit to maurer/rust that referenced this issue Jan 19, 2024
@maurer @nikic
LLVM 18 x86 data layout update
dbff90c 
With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to
16-bytes on x86 based platforms. This will be in LLVM-18. This patch
updates all our spec targets to be 16-byte aligned, and removes the
alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This alignment change was discussed in rust-lang/compiler-team#683

See rust-lang#54341 for additional information about why this is happening and
where this will be useful in the future.

This *does not* stabilize `i128`/`u128` for FFI.
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 20, 2024
@bors
Auto merge of rust-lang#116672 - maurer:128-align, r=nikic
0547c41 
LLVM 18 x86 data layout update

With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to 16-bytes on x86 based platforms. This will be in LLVM-18. This patch updates all our spec targets to be 16-byte aligned, and removes the alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This implements MCP rust-lang/compiler-team#683.

See rust-lang#54341
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jan 21, 2024
@bors
Auto merge of #116672 - maurer:128-align, r=nikic
020c749 
LLVM 18 x86 data layout update

With https://reviews.llvm.org/D86310 LLVM now has i128 aligned to 16-bytes on x86 based platforms. This will be in LLVM-18. This patch updates all our spec targets to be 16-byte aligned, and removes the alignment when speaking to older LLVM.

This results in Rust overaligning things relative to LLVM on older LLVMs.

This implements MCP rust-lang/compiler-team#683.

See #54341
@tgross35 tgross35 mentioned this issue Mar 5, 2024
Open
@jordy25519 jordy25519 mentioned this issue Apr 2, 2024
Open
@tcoratger tcoratger mentioned this issue May 3, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@apiraino @tgross35 @WaffleLapkin @rustbot