-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Stabilize -Cmin-function-alignment
#142824
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
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 |
---|---|---|
|
@@ -811,7 +811,7 @@ mod desc { | |
pub(crate) const parse_wasm_c_abi: &str = "`spec`"; | ||
pub(crate) const parse_mir_include_spans: &str = | ||
"either a boolean (`yes`, `no`, `on`, `off`, etc), or `nll` (default: `nll`)"; | ||
pub(crate) const parse_align: &str = "a number that is a power of 2 between 1 and 2^29"; | ||
pub(crate) const parse_align: &str = "a number that is a power of 2 between 1 and 8192"; | ||
} | ||
|
||
pub mod parse { | ||
|
@@ -1925,6 +1925,12 @@ pub mod parse { | |
return false; | ||
} | ||
|
||
// Limit the alignment to 8192 (i.e. 0x2000, or 1 << 13) bytes. It is the highest function | ||
// alignment that works on all target platforms. COFF does not support higher alignments. | ||
if bytes > 8192 { | ||
return false; | ||
} | ||
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. Why should it be limited on all platforms? Can't we error when the alignment exceeds the maximum that the actual target we are compiling for supports? Maybe someone genuinely needs to align to 16k on ELF for whatever reason? 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. Well, so as we've discovered, alignment is just a mess between clang and llvm. For instance For 8192, we know it'll work everywhere, and the logic for when to accept/reject an alignment value is clear. This flag aligns all functions to the minimum, so I have a hard time seeing a realistic scenario where aligning all functions to 16k is a reasonable thing to do (the limit on individual functions will be handled separately). 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. So I think the options are:
I've picked the most conservative one (again, with the option to relax the limits if the need ever arises), but if there is consensus now on one of the other options that's also fine. 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. It is my understanding that on Linux kernels we still nominally support, overaligning beyond the page size, typically 4096, is not going to work properly, so it is arguable that even 8192 is too high. 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. IMO for the long term, hard-coding to EDIT: or rather, what is a suitable upper bound in this case in terms of stability guarantees? As in, if we stabilize |
||
|
||
let Ok(align) = Align::from_bytes(bytes) else { | ||
return false; | ||
}; | ||
|
@@ -2014,6 +2020,8 @@ options! { | |
"perform LLVM link-time optimizations"), | ||
metadata: Vec<String> = (Vec::new(), parse_list, [TRACKED], | ||
"metadata to mangle symbol names with"), | ||
min_function_alignment: Option<Align> = (None, parse_align, [TRACKED], | ||
"align all functions to at least this many bytes. Must be a power of 2"), | ||
no_prepopulate_passes: bool = (false, parse_no_value, [TRACKED], | ||
"give an empty list of passes to the pass manager"), | ||
no_redzone: Option<bool> = (None, parse_opt_bool, [TRACKED], | ||
|
@@ -2327,8 +2335,6 @@ options! { | |
"gather metadata statistics (default: no)"), | ||
metrics_dir: Option<PathBuf> = (None, parse_opt_pathbuf, [UNTRACKED], | ||
"the directory metrics emitted by rustc are dumped into (implicitly enables default set of metrics)"), | ||
min_function_alignment: Option<Align> = (None, parse_align, [TRACKED], | ||
"align all functions to at least this many bytes. Must be a power of 2"), | ||
mir_emit_retag: bool = (false, parse_bool, [TRACKED], | ||
"emit Retagging MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \ | ||
(default: no)"), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
error: incorrect value `3` for codegen option `min-function-alignment` - a number that is a power of 2 between 1 and 8192 was expected | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
//@ revisions: too-high not-power-of-2 | ||
// | ||
//@ [too-high] compile-flags: -Cmin-function-alignment=16384 | ||
//@ [not-power-of-2] compile-flags: -Cmin-function-alignment=3 | ||
|
||
//~? ERROR a number that is a power of 2 between 1 and 8192 was expected |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
error: incorrect value `16384` for codegen option `min-function-alignment` - a number that is a power of 2 between 1 and 8192 was expected | ||
|