-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LLVM 18 x86 data layout update #116672
LLVM 18 x86 data layout update #116672
Conversation
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
These commits modify compiler targets. |
@rustbot label: +llvm-main |
This comment has been minimized.
This comment has been minimized.
These commits modify compiler targets. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
r? @nikic |
This comment has been minimized.
This comment has been minimized.
#[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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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+
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- We can't adjust rustc's data layout without adjusting LLVM's data layout, that results in bugs.
- 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))] |
There was a problem hiding this comment.
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
Seems like this is worth mentioning in release notes. Possibly under compatibility notes? @rustbot label relnotes |
I was thinking this may be important enough to merit a blog post, so people know the change is coming and what to expect |
8b79f04
to
046503e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Pack u128 in the compiler to mitigate new alignment This is based on rust-lang#116672, adding a new `#[repr(packed(8))]` wrapper on `u128` to avoid changing any of the compiler's size assertions. This is needed in two places: * `SwitchTargets`, otherwise its `SmallVec<[u128; 1]>` gets padded up to 32 bytes. * `LitKind::Int`, so that entire `enum` can stay 24 bytes. * This change definitely has far-reaching effects though, since it's public.
…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`
…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``
…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```
Rollup merge of rust-lang#120529 - nikic:llvm-18-datalayout-fixes, r=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```
Related PRs so far: - rust-lang/rust#119869 - rust-lang/rust#120080 - rust-lang/rust#120128 - rust-lang/rust#119369 - rust-lang/rust#116672 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>
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`): ```console 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`](https://doc.rust-lang.org/nightly/rustc/platfor 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
Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.77.0 (2024-03-21) ========================== - [Reveal opaque types within the defining body for exhaustiveness checking.] (rust-lang/rust#116821) - [Stabilize C-string literals.] (rust-lang/rust#117472) - [Stabilize THIR unsafeck.] (rust-lang/rust#117673) - [Add lint `static_mut_refs` to warn on references to mutable statics.] (rust-lang/rust#117556) - [Support async recursive calls (as long as they have indirection).] (rust-lang/rust#117703) - [Undeprecate lint `unstable_features` and make use of it in the compiler.] (rust-lang/rust#118639) - [Make inductive cycles in coherence ambiguous always.] (rust-lang/rust#118649) - [Get rid of type-driven traversal in const-eval interning] (rust-lang/rust#119044), only as a [future compatiblity lint] (rust-lang/rust#122204) for now. - [Deny braced macro invocations in let-else.] (rust-lang/rust#119062) Compiler -------- - [Include lint `soft_unstable` in future breakage reports.] (rust-lang/rust#116274) - [Make `i128` and `u128` 16-byte aligned on x86-based targets.] (rust-lang/rust#116672) - [Use `--verbose` in diagnostic output.] (rust-lang/rust#119129) - [Improve spacing between printed tokens.] (rust-lang/rust#120227) - [Merge the `unused_tuple_struct_fields` lint into `dead_code`.] (rust-lang/rust#118297) - [Error on incorrect implied bounds in well-formedness check] (rust-lang/rust#118553), with a temporary exception for Bevy. - [Fix coverage instrumentation/reports for non-ASCII source code.] (rust-lang/rust#119033) - [Fix `fn`/`const` items implied bounds and well-formedness check.] (rust-lang/rust#120019) - [Promote `riscv32{im|imafc}-unknown-none-elf` targets to tier 2.] (rust-lang/rust#118704) - Add several new tier 3 targets: - [`aarch64-unknown-illumos`] (rust-lang/rust#112936) - [`hexagon-unknown-none-elf`] (rust-lang/rust#117601) - [`riscv32imafc-esp-espidf`] (rust-lang/rust#119738) - [`riscv32im-risc0-zkvm-elf`] (rust-lang/rust#117958) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Implement `From<&[T; N]>` for `Cow<[T]>`.] (rust-lang/rust#113489) - [Remove special-case handling of `vec.split_off (0)`.](rust-lang/rust#119917) Stabilized APIs --------------- - [`array::each_ref`] (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_ref) - [`array::each_mut`] (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_mut) - [`core::net`] (https://doc.rust-lang.org/stable/core/net/index.html) - [`f32::round_ties_even`] (https://doc.rust-lang.org/stable/std/primitive.f32.html#method.round_ties_even) - [`f64::round_ties_even`] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.round_ties_even) - [`mem::offset_of!`] (https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html) - [`slice::first_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk) - [`slice::first_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk_mut) - [`slice::split_first_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk) - [`slice::split_first_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk_mut) - [`slice::last_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk) - [`slice::last_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk_mut) - [`slice::split_last_chunk`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk) - [`slice::split_last_chunk_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk_mut) - [`slice::chunk_by`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by) - [`slice::chunk_by_mut`] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by_mut) - [`Bound::map`] (https://doc.rust-lang.org/stable/std/ops/enum.Bound.html#method.map) - [`File::create_new`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.create_new) - [`Mutex::clear_poison`] (https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.clear_poison) - [`RwLock::clear_poison`] (https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.clear_poison) Cargo ----- - [Extend the build directive syntax with `cargo::`.] (rust-lang/cargo#12201) - [Stabilize metadata `id` format as `PackageIDSpec`.] (rust-lang/cargo#12914) - [Pull out as `cargo-util-schemas` as a crate.] (rust-lang/cargo#13178) - [Strip all debuginfo when debuginfo is not requested.] (rust-lang/cargo#13257) - [Inherit jobserver from env for all kinds of runners.] (rust-lang/cargo#12776) - [Deprecate rustc plugin support in cargo.] (rust-lang/cargo#13248) Rustdoc ----- - [Allows links in markdown headings.] (rust-lang/rust#117662) - [Search for tuples and unit by type with `()`.] (rust-lang/rust#118194) - [Clean up the source sidebar's hide button.] (rust-lang/rust#119066) - [Prevent JS injection from `localStorage`.] (rust-lang/rust#120250) Misc ---- - [Recommend version-sorting for all sorting in style guide.] (rust-lang/rust#115046) 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. - [Add more weirdness to `weird-exprs.rs`.] (rust-lang/rust#119028)
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.
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.
} | ||
|
||
#[no_mangle] | ||
pub fn load(x: &ScalarPair) -> ScalarPair { |
There was a problem hiding this comment.
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.
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