rustc: Link LLVM directly into rustc again (take two) #67077
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@alexcrichton: I can't reproduce the old PR's CI failure locally. Could we run a try-build to see if it's still an issue? |
|
@bors try |
bors
added a commit
that referenced
this pull request
Dec 6, 2019
rustc: Link LLVM directly into rustc again (take two) This is a continuation of PR #65703
|
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 - checks-azure |
bors
added
S-waiting-on-author
|
When not merging this PR, it would be possible to rewrite https://github.com/getditto/rust-bitcode to only recompile the codegen backend, instead of a whole rust compiler by using |
|
I'd recommend enabling the try builder as part of this PR itself, you can edit the configuration for builders on PRs so that way you can test through a PR if you're unable to test it locally. |
|
☔ The latest upstream changes (presumably #67091) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I've managed to reproduce the issue locally via |
Aaron1011
force-pushed
the
build-llvm-in-binary
branch
from
3bd9a88
to
137d0fd
Compare
|
@alexcrichton: I believe I've fixed the issue. It didn't show up locally because I was statically linking LLVM, instead of dynamicly linking against |
|
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 |
|
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 |
|
It looks like this actually passed, but failed due to the weird CI change. @alexcrichton: I think this is ready to merge |
|
Ok great! Want to back out the CI changes? Additionally, there's another call to |
Aaron1011
force-pushed
the
build-llvm-in-binary
branch
from
c2ebf81
to
de82e53
Compare
|
@alexcrichton: The existing call to The new call to |
|
I've added a comment to the existing call explaining what it does. |
|
I just noticed that this might interact with how we do ThinLTO on the LLVM code which gives us something like a 10% performance boost. Is that still possible now? |
|
@michaelwoerister: I'm not quite sure what you mean - are you saying we currently get a 10% performance boost, or that this PR might give us a 10% performance boost? This PR does not affect how LLVM itself is built (e.g. whether it's statically or dynamically linked into |
|
We should probably at least to a perf run, it looks like neither this nor the previous PR had one. @bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit ab73c4c6bd3886f05e616610e07ee79695665ab3 with merge 643b73eedf9eb3b55040ec8e87b78b191900f6a0... |
|
📌 Commit 150d328 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 150d328 with merge e02eae73182f4e83beeb13a16f1c41fdece59c2c... |
|
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 - checks-azure |
bors
added
S-waiting-on-review
rustllvm relies on the `LLVMRustStringWriteImpl` symbol existing, but this symbol was previously defined in a *downstream* crate (rustc_codegen_llvm, which depends on rustc_llvm. While this somehow worked under the old 'separate bootstrap step for codegen' scheme, it meant that rustc_llvm could not actually be built by itself, since it relied linking to the downstream rustc_codegen_llvm crate. Now that librustc_codegen_llvm is just a normal crate, we actually try to build a standalone rustc_llvm when we run tests. This commit moves `LLVMRustStringWriteImpl` into rustc_llvm (technically the rustllvm directory, which has its contents built by rustc_llvm). This ensures that we can build each crate in the graph by itself, without requiring that any downstream crates be linked in as well.
|
@alexcrichton: The new build configuration seems to have exposed a weird implicit dependency of rustllvm on rustc_codegen_llvm. I've pushed a commit that should fix the issue. |
|
@bors: r+ Nice catch! |
|
📌 Commit 47e932b has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Dec 13, 2019
rustc: Link LLVM directly into rustc again (take two) This is a continuation of PR #65703
|
☀️ Test successful - checks-azure |
mati865
reviewed
Dec 18, 2019
| // not for MSVC or macOS | ||
| if builder.config.llvm_static_stdcpp && | ||
| !target.contains("freebsd") && | ||
| !target.contains("windows") && |
There was a problem hiding this comment.
This line changed msvc to windows breaking windows-gnu target.
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jan 2, 2020
Previously, we relied fully on Cargo to detect that the compiler had changed and it needed to rebuild the standard library (or later "components"). This used to not quite be the case prior to moving to LLVM be a separate cargo invocation; subsequent compiles would recompile std and friends if LLVM had changed (rust-lang#67077 is the PR that changes things here). This PR moves us to clearing out libstd when it is being compiled if the rustc we're using has changed. We fairly harshly limit the cases in which we do this (e.g., ignoring dry run mode, and so forth, as well as rustdoc invocations). This is primarily because when we're not using the compiler directly, so clearing out in other cases is likely to lead to bugs, particularly as our deletion scheme is pretty blunt today (basically removing more than is needed, i.e., not just the rustc artifacts). In practice, this targeted fix does fix the known bug, though it may not fully resolve the problem here. It's also not clear that there is a full fix hiding here that doesn't involve a more major change (like -Zbinary-dep-depinfo was). As a drive-by fix, don't delete the compiler before calling Build::copy, as that also deletes the compiler.
bors
added a commit
that referenced
this pull request
Jan 8, 2020
Clear out target directory if compiler has changed Previously, we relied fully on Cargo to detect that the compiler had changed and it needed to rebuild the standard library (or later "components"). This used to not quite be the case prior to moving to LLVM be a separate cargo invocation; subsequent compiles would recompile std and friends if LLVM had changed (#67077 is the PR that changes things here). This PR moves us to clearing out libstd when it is being compiled if the rustc we're using has changed. We fairly harshly limit the cases in which we do this (e.g., ignoring dry run mode, and so forth, as well as rustdoc invocations). This is primarily because when we're not using the compiler directly, so clearing out in other cases is likely to lead to bugs, particularly as our deletion scheme is pretty blunt today (basically removing more than is needed, i.e., not just the rustc artifacts). In practice, this targeted fix does fix the known bug, though it may not fully resolve the problem here. It's also not clear that there is a full fix hiding here that doesn't involve a more major change (like -Zbinary-dep-depinfo was). As a drive-by fix, don't delete the compiler before calling Build::copy, as that also deletes the compiler.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of PR #65703