LLVM 18 x86 data layout update by maurer · Pull Request #116672 · 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

Conversation83 Commits4 Checks0 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 }})

maurer

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

@rustbot

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Oct 12, 2023

@rustbot

@maurer

@rustbot rustbot added the llvm-main

Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling)

label

Oct 12, 2023

@rust-log-analyzer

This comment has been minimized.

@rustbot

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cjgillot

@rust-log-analyzer

This comment has been minimized.

tgross35

Comment on lines +3189 to +3313

#[cfg(not(bootstrap))]
static_assert_size!(LitKind, 32);
static_assert_size!(Local, 72);
static_assert_size!(MetaItemLit, 40);
// This can be removed after i128:128 is in the bootstrap compiler's target.
#[cfg(not(bootstrap))]
static_assert_size!(MetaItemLit, 48);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think config on bootstrap is too reliable since we could wind up with a bootstrap with either LLVM version. It would be better if there is a way to check the LLVM version here.

Alternatively, a static assertion that size can be either 24 or 32 (or 40/48) with a // FIXME: is probably suitable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't depend on the LLVM version - it depends on the alignment in the spec file. If you refer back to the zulip conversation we had, you'll note that TargetDataLayout will determine the size of these types, which is derived from the pre-munged data_layout, not the one after correction.

So in theory, after this rev, this should always be the case, regardless of LLVM version linked.

All that said, I might be misunderstanding something here, because I'm getting some creader issues in stage2+

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, then I guess those are coming from rustc using the new layout strings (16-byte align) while llvm <18 uses the adjusted version (8byte)? What happens if you update the layout strings, don't do the LLVM<18 fixup, and just suppress the "different default layout strings" error? I think that would force older LLVM to use 16byte alignment (like Clang does), which means all of rust upgrades at once.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just tested, and that works (./x.py test and ./x.py dist both complete locally).

The check still seemed valuable, so I've uploaded a change that makes the bug! conditional check the munged data layout rather than munging it when sending it to LLVM.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't do this, it will cause LLVM to assert. Even if LLVM didn't assert it would break cross-language LTO.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be right. I must have had asserts off locally or something, because this did work with the vendored LLVM locally.

Given that:

  1. We can't adjust rustc's data layout without adjusting LLVM's data layout, that results in bugs.
  2. We can't adjust LLVM's data layout on older LLVM.

Does that just leave the "newer LLVM needs to munge the data_layout early in execution, before TargetDataLayout gets built?

@@ -1585,13 +1585,19 @@ mod size_asserts {
use super::*;
use rustc_data_structures::static_assert_size;
// tidy-alphabetical-start
static_assert_size!(BasicBlockData<'_>, 136);
// This can be removed after i128:128 is in the bootstrap compiler's target.
#[cfg(not(bootstrap))]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the comment in compiler/rustc_ast/src/ast.rs

@tmandry

Seems like this is worth mentioning in release notes. Possibly under compatibility notes?

@rustbot label relnotes

@rustbot rustbot added the relnotes

Marks issues that should be documented in the release notes of the next release.

label

Oct 12, 2023

@tgross35

I was thinking this may be important enough to merit a blog post, so people know the change is coming and what to expect

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nikic nikic mentioned this pull request

Jan 31, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 31, 2024

@matthiaskrgr

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Jan 31, 2024

@matthiaskrgr

…cuviper

Update data layouts in custom target tests for LLVM 18

Apply the data layout changes from rust-lang#116672 to custom target specs as well, as we started validating them since rust-lang#120062.

Fixes rust-lang#120492.

r? @cuviper

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Feb 3, 2024

@matthiaskrgr

…cuviper

Update data layouts in custom target tests for LLVM 18

Apply the data layout changes from rust-lang#116672 to custom target specs as well, as we started validating them since rust-lang#120062.

Fixes rust-lang#120492.

r? @cuviper

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Feb 4, 2024

@rust-timer

feliperodri added a commit to model-checking/kani that referenced this pull request

Feb 8, 2024

Related PRs so far:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Signed-off-by: Felipe R. Monteiro felisous@amazon.com Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: tautschnig tautschnig@users.noreply.github.com Co-authored-by: Qinheping Hu qinhh@amazon.com Co-authored-by: Michael Tautschnig tautschn@amazon.com Co-authored-by: Felipe R. Monteiro felisous@amazon.com

This was referenced

Feb 26, 2024

BurntSushi pushed a commit to BurntSushi/memchr that referenced this pull request

Mar 27, 2024

@mkroening

The old CI test was removed (#77 (comment)) due to rust-lang/rust#116672 causing a mismatch of data layouts with the custom target (src/tests/x86_64-soft_float.json):

error: data-layout for target `x86_64-soft_float-10047705440633310713`, `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`, differs from LLVM target's `x86_64-unknown-none` default layout, `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128`

This PR removes the unused custom target and reintroduces the CI test with the now-available tier 2 target x86_64-unknown-none m-support/x86_64-unknown-none.html). This makes this test much more robust, since we don't have to update a custom target ourselves and can even use stable Rust now.

PR #146

@hzuo hzuo mentioned this pull request

Mar 27, 2024

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Mar 29, 2024

@he32

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request

Apr 7, 2024

@bors

Separate immediate and in-memory ScalarPair representation

Currently, we assume that ScalarPair is always represented using a two-element struct, both as an immediate value and when stored in memory.

This currently works fairly well, but runs into problems with rust-lang/rust#116672, where a ScalarPair involving an i128 type can no longer be represented as a two-element struct in memory. For example, the tuple (i32, i128) needs to be represented in-memory as { i32, [3 x i32], i128 } to satisfy alignment requirements. Using { i32, i128 } instead will result in the second element being stored at the wrong offset (prior to LLVM 18).

Resolve this issue by no longer requiring that the immediate and in-memory type for ScalarPair are the same. The in-memory type will now look the same as for normal struct types (and will include padding filler and similar), while the immediate type stays a simple two-element struct type. This also means that booleans in immediate ScalarPair are now represented as i1 rather than i8, just like we do everywhere else.

The core change here is to llvm_type (which now treats ScalarPair as a normal struct) and immediate_llvm_type (which returns the two-element struct that llvm_type used to produce). The rest is fixing things up to no longer assume these are the same. In particular, this switches places that try to get pointers to the ScalarPair elements to use byte-geps instead of struct-geps.

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request

Apr 27, 2024

@bors

Separate immediate and in-memory ScalarPair representation

Currently, we assume that ScalarPair is always represented using a two-element struct, both as an immediate value and when stored in memory.

This currently works fairly well, but runs into problems with rust-lang/rust#116672, where a ScalarPair involving an i128 type can no longer be represented as a two-element struct in memory. For example, the tuple (i32, i128) needs to be represented in-memory as { i32, [3 x i32], i128 } to satisfy alignment requirements. Using { i32, i128 } instead will result in the second element being stored at the wrong offset (prior to LLVM 18).

Resolve this issue by no longer requiring that the immediate and in-memory type for ScalarPair are the same. The in-memory type will now look the same as for normal struct types (and will include padding filler and similar), while the immediate type stays a simple two-element struct type. This also means that booleans in immediate ScalarPair are now represented as i1 rather than i8, just like we do everywhere else.

The core change here is to llvm_type (which now treats ScalarPair as a normal struct) and immediate_llvm_type (which returns the two-element struct that llvm_type used to produce). The rest is fixing things up to no longer assume these are the same. In particular, this switches places that try to get pointers to the ScalarPair elements to use byte-geps instead of struct-geps.

bjorn3

}
#[no_mangle]
pub fn load(x: &ScalarPair) -> ScalarPair {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that this is testing the rust abi? The C ABI returns ScalarPair quite differently using an sret pointer. #130899 will change the Rust ABI to also return it using an sret pointer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the intent was to test a case where i128 gets returned in an LLVM IR struct. If that situation no longer occurs after that PR, then testing the sret case is fine.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Oct 13, 2024

@he32

This is based on the pkgsrc-wip rust180 package, retaining the main pkgsrc changes as best as I could.

Pkgsrc changes:

Upstream chnages:

Version 1.80.1 (2024-08-08)

Version 1.80.0 (2024-07-25)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.79.0 (2024-06-13)

Language

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Misc

Compatibility Notes

Version 1.78.0 (2024-05-02)

Language

Compiler

Target changes:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.77.0 (2024-03-21)

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

Cargo

Rustdoc

Misc

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

tgross35 added a commit to tgross35/rust that referenced this pull request

Feb 20, 2025

@tgross35

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:

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)

Labels

llvm-main

Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling)

merged-by-bors

This PR was explicitly merged by bors.

perf-regression

Performance regression.

perf-regression-triaged

The performance regression has been triaged.

relnotes

Marks issues that should be documented in the release notes of the next release.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.