-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
try to make rustc-dev work with and without LTO #161535
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 |
|---|---|---|
|
|
@@ -449,27 +449,37 @@ impl Cargo { | |
| let cc = ccacheify(&builder.cc(target)); | ||
| self.command.env(format!("CC_{triple_underscored}"), &cc); | ||
|
|
||
| // Compiling C deps, like jemalloc and llvm-wrapper, should be with | ||
| // the same LTO mode as the Rust code they are linked into. | ||
| // Compiling C deps, like jemalloc and llvm-wrapper, should use the same LTO mode as | ||
| // the Rust code they are linked into. We don't pass the checks cc-rs uses to | ||
| // auto-enable that so we have to do it ourselves. | ||
| // | ||
| // Std's C deps, e.g. compiler_builtins, ship inside rlibs that | ||
| // end users consume directly. Building with `-flto` may break | ||
| // non-LLVM linkers or mismatch on bitcode versions. Therefore we | ||
| // must not build std in LTO mode here. | ||
| // However, some of these files get distributed via rustup, and the user may then use | ||
| // non-LLVM linkers. If we turn on regular LTO (which stores LLVM bitcode and nothing | ||
| // else), only LLVM linkers can deal with those files. Therefore we have to use "fat | ||
| // LTO" and include both machine code and LLVM bitcode. | ||
| // | ||
| // We don't do this for `std` as this may make std bigger and it is generally a | ||
| // non-trivial configuration we do not want to risk. (`std` does have C deps, e.g. | ||
| // compiler_builtins, so this is relevant.) | ||
| let lto_cflag = if matches!(self.mode, Mode::Rustc | Mode::ToolRustcPrivate) | ||
|
Member
Author
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. FWIW, this |
||
| && is_lto_stage(&self.compiler) | ||
| && builder.cc_tool(target).is_like_clang() | ||
|
weihanglo marked this conversation as resolved.
|
||
| { | ||
| match builder.config.rust_lto { | ||
| RustcLto::Thin => Some("-flto=thin"), | ||
| RustcLto::Fat => Some("-flto=full"), | ||
| let lto_cflag = match builder.config.rust_lto { | ||
| RustcLto::Thin => Some("-flto=thin -ffat-lto-objects"), | ||
| RustcLto::Fat => Some("-flto=full -ffat-lto-objects"), | ||
| RustcLto::ThinLocal | RustcLto::Off => None, | ||
| }; | ||
| // Make sure the linker actually uses the fat LTO part. | ||
| if lto_cflag.is_some() { | ||
| self.rustflags.arg("-Clink-args=-Wl,--fat-lto-objects"); | ||
|
Member
Author
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. IIUC, this flag is needed to make the linker even recognize the bitcode in fat LTO objects. It is strange that this is not the default (unlike LTO for "thin" objects which apparently is the default). Does this mean we're also not LTO'ing jemalloc for the distributed Miri, Clippy etc any more?
Member
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'll do a Clippy benchmark after the current one finishes to check.
Member
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. Note this is LLD specific flag and you can let Clang handle it itself. Linkers other than LLD don't need this flag because
Member
Author
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. The flag was needed, we lost LTO otherwise which was visible in perf runs. Don't ask me why, I am throwing spaghetti at the wall here.
Member
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 meant it also works when you pass
Member
Author
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. Ah okay, thanks. We didn't try that. |
||
| } | ||
| lto_cflag | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| // Extend `CXXFLAGS_$TARGET` with our extra flags. | ||
| // Extend `CFLAGS_$TARGET` with our extra flags. | ||
| let env = format!("CFLAGS_{triple_underscored}"); | ||
| let mut cflags = | ||
| builder.cc_unhandled_cflags(target, GitRepo::Rustc, CLang::C).join(" "); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
should be with the same LTO mode as the Rust code they are linked intoJust a note that this doesn't seem very true.
View changes since the review
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.
FWIW, the discussion happens in #t-compiler > Analysing broken jemalloc rlib
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 just reformatted that comment, I don't know if it is correct.
What would be more correct?