-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Apply some fixes to cross-language LTO (especially when targeting MSVC) #53031
Conversation
@bors try |
Apply some fixes to cross-language LTO (especially when targeting MSVC) This PR contains a few fixes that were needed in order to get Firefox compiling with Rust/C++ cross-language ThinLTO on Windows. The commits are self-contained and should be self-explanatory. r? @alexcrichton
@@ -1626,8 +1626,12 @@ fn relevant_lib(sess: &Session, lib: &NativeLibrary) -> bool { | |||
fn is_full_lto_enabled(sess: &Session) -> bool { |
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.
With the comment below, perhaps this function could be renamed to something like are_upstream_rust_objects_already_included
? (albeit a bit wordy)
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.
Sounds good to me.
@@ -1331,6 +1335,11 @@ fn execute_work_item(cgcx: &CodegenContext, | |||
let needs_lto = match cgcx.lto { | |||
Lto::No => false, | |||
|
|||
// If the linker does LTO, we don't have to do it. Note that we | |||
// keep doing full LTO, if it is requested, as not to break the | |||
// assumption that the output will be a single module. |
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.
To confirm, this is a rustc bug, right? This is fixable on our end where fat LTO plus cross-lang-lto shouldn't actually run the LTO passes in rustc?
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, it probably isn't too hard to do just the module merging but not the optimizations. Maybe in another PR, unless you think it's urgent.
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.
Oh nah definitely fine to happen later, just wanted to make sure I understood!
sess.opts.cg.prefer_dynamic && | ||
sess.target.target.options.is_like_msvc { | ||
sess.err("Linker plugin based LTO is not supported together with \ | ||
`-C prefer-dynamic` when targeting MSVC"); |
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.
Could a comment be added above this with a brief explanation as to why the error is being emitted? AFAIK it's all b/c of DLL weirdness, right?
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.
👍
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☀️ Test successful - status-travis |
@rust-timer build 807bbd9 |
Success: Queued 807bbd9 with parent 88e0ff1, comparison URL. |
Perf is ready. Almost nothing is changed. |
Hm, there's still a bit of a regression. It might be coming from allocating the |
0b253da
to
603584d
Compare
@bors try ( |
Apply some fixes to cross-language LTO (especially when targeting MSVC) This PR contains a few fixes that were needed in order to get Firefox compiling with Rust/C++ cross-language ThinLTO on Windows. The commits are self-contained and should be self-explanatory. r? @alexcrichton
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
603584d
to
551ef38
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
551ef38
to
1ecb0a5
Compare
@bors try |
⌛ Trying commit 1ecb0a56fd1211e1357c59cedd22e94bef989596 with merge 63d3f70a598b20ea7e9eb775348456a015daedb5... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Hm, everything passed except for |
Apply some fixes to cross-language LTO (especially when targeting MSVC) This PR contains a few fixes that were needed in order to get Firefox compiling with Rust/C++ cross-language ThinLTO on Windows. The commits are self-contained and should be self-explanatory. r? @alexcrichton
☀️ Test successful - status-appveyor, status-travis |
@michaelwoerister is there a guide somewhere explaining how to use Cross-Language LTO? I‘m Linking a vectorized version of libm in packed simd that gets compiled from C and would like it to be inlined into Rust. I’d also like to set this up for jemalloc. |
No guide yet. You basically:
Note that things are still experimental (although stable enough to build Firefox with it). |
This fixes a regression from rust-lang#53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes rust-lang#53322
Fix warnings about the `native` target-cpu This fixes a regression from #53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes #53322
This fixes a regression from rust-lang#53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes rust-lang#53322
Fix warnings about the `native` target-cpu This fixes a regression from #53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes #53322
This fixes a regression from rust-lang#53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes rust-lang#53322
Fix warnings about the `native` target-cpu This fixes a regression from #53031 where specifying `-C target-cpu=native` is printing a lot of warnings from LLVM about `native` being an unknown CPU. It turns out that `native` is indeed an unknown CPU and we have to perform a mapping to an actual CPU name, but this mapping is only performed in one location rather than all locations we inform LLVM about the target CPU. This commit centralizes the mapping of `native` to LLVM's value of the native CPU, ensuring that all locations we inform LLVM about the `target-cpu` it's never `native`. Closes #53322
This PR contains a few fixes that were needed in order to get Firefox compiling with Rust/C++ cross-language ThinLTO on Windows. The commits are self-contained and should be self-explanatory.
r? @alexcrichton