-
Notifications
You must be signed in to change notification settings - Fork 13.3k
LLVM 18 x86 data layout update #116672
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3304,9 +3304,13 @@ mod size_asserts { | |
static_assert_size!(Impl, 136); | ||
static_assert_size!(Item, 136); | ||
static_assert_size!(ItemKind, 64); | ||
static_assert_size!(LitKind, 24); | ||
// This can be removed after i128:128 is in the bootstrap compiler's target. | ||
#[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); | ||
Comment on lines
+3308
to
+3313
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I've just tested, and that works ( The check still seemed valuable, so I've uploaded a change that makes the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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:
Does that just leave the "newer LLVM needs to munge the |
||
static_assert_size!(Param, 40); | ||
static_assert_size!(Pat, 72); | ||
static_assert_size!(Path, 24); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1672,13 +1672,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 commentThe reason will be displayed to describe this comment to others. Learn more. Same as the comment in |
||
static_assert_size!(BasicBlockData<'_>, 144); | ||
static_assert_size!(LocalDecl<'_>, 40); | ||
static_assert_size!(SourceScopeData<'_>, 72); | ||
static_assert_size!(Statement<'_>, 32); | ||
static_assert_size!(StatementKind<'_>, 16); | ||
static_assert_size!(Terminator<'_>, 104); | ||
static_assert_size!(TerminatorKind<'_>, 88); | ||
// This can be removed after i128:128 is in the bootstrap compiler's target. | ||
#[cfg(not(bootstrap))] | ||
static_assert_size!(Terminator<'_>, 112); | ||
// This can be removed after i128:128 is in the bootstrap compiler's target. | ||
#[cfg(not(bootstrap))] | ||
static_assert_size!(TerminatorKind<'_>, 96); | ||
static_assert_size!(VarDebugInfo<'_>, 88); | ||
// tidy-alphabetical-end | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -10,8 +10,8 @@ pub fn target() -> Target { | |||
Target { | ||||
llvm_target: "x86_64-pc-windows-msvc".into(), | ||||
pointer_width: 64, | ||||
data_layout: "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" | ||||
.into(), | ||||
data_layout: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe win x86 targets are missing here bridiver@defeeb9#diff-82540e5fa4a277a476dc2b2e408d3633344032cb745bf7d2013fd27e6bc62247 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They should be there now
|
||||
"e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128".into(), | ||||
arch: "x86_64".into(), | ||||
options: base, | ||||
} | ||||
|
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 failed for me on macos x64 with llvmorg-18-init-9505-g10664813 from chromium . Expected was 32, but actual was still 24
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.
https://github.com/rust-lang/rust/pull/116672/files#diff-00567f9be31678eec46651e6157f0eaaa47d1f344eb61b8a63f87edfa7bf6b14R3169 also failed and was still 40 instead of 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.
hmmm... seeing the error on the asserts mod.rs as well. Maybe I'm doing something wrong...
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.
Not sure if you have seen but there is some more discussion about this pattern in #116672 (comment)
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.
Also, feel free to drop into the Zulip discussion since you are taking a look at this https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/LLVM.20alignment.20of.20i128
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.
yes, but in this case I'm building with a version of llvm 18 that has the alignment change so the new value should be correct, right? I'm using the chromium rust build script that this change was originally made to fix
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 see what I did wrong here, I removed
#[cfg(not(bootstrap))]
because I thought it was no longer needed since I was building against llvm 18, but on further inspection it looks like the problem is a mismatch between the bootstrap llvm version and the output rustc llvm versionThere 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 guess that isn't quite right, but @maurer explained the problem and now I understand what's going on