-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add TargetOptions::min_global_align
, with s390x at 16-bit
#44440
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
Conversation
The SystemZ `LALR` instruction provides PC-relative addressing for globals, but only to *even* addresses, so other compilers make sure that such globals are always 2-byte aligned. In Clang, this is modeled with `TargetInfo::MinGlobalAlign`, and `TargetOptions::min_global_align` now serves the same purpose for rustc. In Clang, the only targets that set this are SystemZ, Lanai, and NVPTX, and the latter two don't have targets in rust master.
r? @alexcrichton or @japaric |
// which can force it to be smaller. Rust doesn't support this yet. | ||
if let Some(min) = ccx.sess().target.target.options.min_global_align { | ||
match ty::layout::Align::from_bits(min, min) { | ||
Ok(min) => align = cmp::max(align, min.abi() as machine::llalign), |
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.
There's a .min
method on Align
, you should use that.
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.
We want the greater of the two, so it would be Align::max
. But this would require converting the incoming machine::llalign
to Align
, comparing, then back to llalign
for the LLVM call. I don't see the point of that.
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.
Well, llalign
should have never existed etc. etc. I'll just have to remember fixing it on my branch where it's gone, but the rebase should catch it because of type mismatches and whatnot.
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.
Maybe it's also pointless to convert min_global_align
to Align
. I just figured a little sanity checking of that value would be good.
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.
That conversion is fine, since it will be in there sooner or later! In the future I plan to have librustc_back
and a lot of librustc/ty/layout.rs
be in the same crate so even the target specification would use it.
📌 Commit c606e97 has been approved by |
Add `TargetOptions::min_global_align`, with s390x at 16-bit The SystemZ `LALR` instruction provides PC-relative addressing for globals, but only to *even* addresses, so other compilers make sure that such globals are always 2-byte aligned. In Clang, this is modeled with `TargetInfo::MinGlobalAlign`, and `TargetOptions::min_global_align` now serves the same purpose for rustc. In Clang, the only targets that set this are SystemZ, Lanai, and NVPTX, and the latter two don't have targets in rust master. Fixes #44411. r? @eddyb
☀️ Test successful - status-appveyor, status-travis |
The SystemZ
LALR
instruction provides PC-relative addressing for globals,but only to even addresses, so other compilers make sure that such
globals are always 2-byte aligned. In Clang, this is modeled with
TargetInfo::MinGlobalAlign
, andTargetOptions::min_global_align
nowserves the same purpose for rustc.
In Clang, the only targets that set this are SystemZ, Lanai, and NVPTX, and
the latter two don't have targets in rust master.
Fixes #44411.
r? @eddyb