Move LLVM bitcode destination #70458
Closed
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
|
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 |
|
What's the motivation behind this? Wouldn't this cause more work for linkers? |
|
By default will slow things down slightly, because it's storing uncompressed bitcode instead of bitcode. But the plan is to change Cargo to use the |
|
@nnethercote for perf data, could you collect data for embedding bitcode by default and not embedding bitcode by default? I'm curious to see how bad this is as-is and how good it's gonna get when Cargo passes the flag. It's also worth pointing out that before landing I think we'll want a stable |
|
I will definitely collect perf data once I have this working a little better. Here is the old perf data showing the no-bitcode case; hopefully I will get similar results when I re-measure. |
nnethercote
force-pushed
the
mv-llvm-bitcode-destination
branch
from
a5c176d
to
61700c5
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 |
bjorn3
reviewed
Mar 29, 2020
bjorn3
reviewed
Mar 29, 2020
|
I think this will make cg_clif "just work ™️" when |
nnethercote
force-pushed
the
mv-llvm-bitcode-destination
branch
from
61700c5
to
016b8f8
Compare
|
@alexcrichton: New code is up, it's much better than what I had before. It passes all tests on my Linux box. There is one unresolved issue relating to bitcode marker sections, marked with a "njn:" comment in the code. Plus we still have to work out the story with wasm. |
|
Let's see the change in instruction counts for First, the change as is, with uncompressed bitcode replacing compressed bitcode. It's a mix of regressions and improvements, with the largest regressions being bigger than the largest improvements. Next, the change with |
|
As for disk space, I measured the size of the 66 rlibs produced for fix-stacks, a moderate-sized Rust program.
|
I'll take a look at this today
I'm actually curious, why is this slower? I thought the only thing we're doing is avoiding compression, so I would expect this to also be faster across the board (just bigger on disk) |
|
Compile times are way more sensitive to I/O performance than I expected. https://github.com/bjorn3/rustc_codegen_cranelift/issues/865#issuecomment-574747252 is an instance where using an SSD is 2x as fast as using an NFS mount. (This is with cg_llvm, not just cg_clif) |
|
Ok it's not exactly the prettiest thing in the world, but I've pushed up an implementation that supports wasm. Like most things along these lines it'll look a lot nicer once we get official LLD support for a feature like this because then we can just delete all of the code :) |
|
I've filed an issue for upstream support here on wasm |
bjorn3
reviewed
Mar 30, 2020
|
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 |
Here is the first part of the Cachegrind diff for |
|
@rust-lang/compiler members: we have an interesting flags situation here that needs your input. Let me give an overview. (EDIT: the original proposal was for a Currently, when rustc produces an rlib it contains both object code ( So this PR is about removing this inefficiency. For backwards compatibility, we will preserve the existing behaviour of producing both object code and bitcode. However, we will instead store the bitcode uncompressed, in a special section in the object code. This is a standard format that clang uses and there is already machinery in the compiler to do it, which is used for This change from compressed bitcode to uncompressed bitcode causes a mixture of performance improvements and regressions, and we don't want to land it by itself. The other key part is that we introduce a new boolean flag The tricky part is that we want to land the rustc changes and the Cargo changes in tandem so that we don't have a period of performance regression, and that requires promoting the Thoughts? |
|
I'd generally prefer an overridable option |
The flag parsing code strongly guides us towards boolean flags that default to false. If you use If you use
All of these feel clumsy. Any preferences? |
This is the standard ( |
nnethercote
force-pushed
the
mv-llvm-bitcode-destination
branch
from
fd6f71f
to
60441e2
Compare
|
New code is up, it addresses all the comments. There are two new commits in the sequence:
And the flag is still |
How do you do that? Can you enable all of them? |
|
If the builders are linux based, it's generally very easy, you just add some lines to https://github.com/rust-lang/rust/blob/master/src/ci/azure-pipelines/try.yml#L28 here, copying from this list here: https://github.com/rust-lang/rust/blob/master/src/ci/azure-pipelines/auto.yml#L32 For the windows/macos builders, it's similar, but you have to copy in the header from the auto.yml file as well (i.e., https://github.com/rust-lang/rust/blob/master/src/ci/azure-pipelines/auto.yml#L74-L81). We can't handle more than 6 concurrent builders being run on try, but you can drop the 2 builders we normally run if you want to test 6 at a time. Most of the time that's enough since you can isolate a problem through a normal r+ run and then debug it in try on that specific platform. |
|
As described above, the chosen approach doesn't work with some linkers, so we will have to do things differently. The performance improvements will still be achievable, but they will be done in follow-up PRs. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 22, 2020
…ichton Add `-Cbitcode-in-rlib`. This is a cut-down version of rust-lang#70458 that gets the compile-time wins. r? @alexcrichton
|
I'm attempting to resurrect this at #71528 |
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
Apr 29, 2020
This commit is an attempted resurrection of rust-lang#70458 where LLVM bitcode emitted by rustc into rlibs is stored into object file sections rather than in a separate file. The main rationale for doing this is that when rustc emits bitcode it will no longer use a custom compression scheme which makes it both easier to interoperate with existing tools and also cuts down on compile time since this compression isn't happening. The blocker for this in rust-lang#70458 turned out to be that native linkers didn't handle the new sections well, causing the sections to either trigger bugs in the linker or actually end up in the final linked artifact. This commit attempts to address these issues by ensuring that native linkers ignore the new sections by inserting custom flags with module-level inline assembly. Note that this does not currently change the API of the compiler at all. The pre-existing `-C bitcode-in-rlib` flag is co-opted to indicate whether the bitcode should be present in the object file or not. Finally, note that an important consequence of this commit, which is also one of its primary purposes, is to enable rustc's `-Clto` bitcode loading to load rlibs produced with `-Clinker-plugin-lto`. The goal here is that when you're building with LTO Cargo will tell rustc to skip codegen of all intermediate crates and only generate LLVM IR. Today rustc will generate both object code and LLVM IR, but the object code is later simply thrown away, wastefully.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 29, 2020
…ercote Store LLVM bitcode in object files, not compressed This commit is an attempted resurrection of rust-lang#70458 where LLVM bitcode emitted by rustc into rlibs is stored into object file sections rather than in a separate file. The main rationale for doing this is that when rustc emits bitcode it will no longer use a custom compression scheme which makes it both easier to interoperate with existing tools and also cuts down on compile time since this compression isn't happening. The blocker for this in rust-lang#70458 turned out to be that native linkers didn't handle the new sections well, causing the sections to either trigger bugs in the linker or actually end up in the final linked artifact. This commit attempts to address these issues by ensuring that native linkers ignore the new sections by inserting custom flags with module-level inline assembly. Note that this does not currently change the API of the compiler at all. The pre-existing `-C bitcode-in-rlib` flag is co-opted to indicate whether the bitcode should be present in the object file or not. Finally, note that an important consequence of this commit, which is also one of its primary purposes, is to enable rustc's `-Clto` bitcode loading to load rlibs produced with `-Clinker-plugin-lto`. The goal here is that when you're building with LTO Cargo will tell rustc to skip codegen of all intermediate crates and only generate LLVM IR. Today rustc will generate both object code and LLVM IR, but the object code is later simply thrown away, wastefully.
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
May 1, 2020
This commit finishes work first pioneered in rust-lang#70458 and started in rust-lang#71528. The `-C bitcode-in-rlib` option, which has not yet reached stable, is renamed to `-C embed-bitcode` since that more accurately reflects what it does now anyway. Various tests and such are updated along the way as well. This'll also need to be backported to the beta channel to ensure we don't accidentally stabilize `-Cbitcode-in-rlib` as well.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 2, 2020
…thercote Rename `bitcode-in-rlib` option to `embed-bitcode` This commit finishes work first pioneered in rust-lang#70458 and started in rust-lang#71528. The `-C bitcode-in-rlib` option, which has not yet reached stable, is renamed to `-C embed-bitcode` since that more accurately reflects what it does now anyway. Various tests and such are updated along the way as well. This'll also need to be backported to the beta channel to ensure we don't accidentally stabilize `-Cbitcode-in-rlib` as well.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 4, 2020
…nethercote Don't copy bytecode files into the incr. comp. cache. It's no longer necessary now that bitcode is embedded into object files. This change meant that `WorkProductFileKind::Bytecode` is no longer necessary, which means that type is no longer necessary, which allowed several places in the code to become simpler. This commit was written by @nnethercote in rust-lang#70458 but that didn't land. In the meantime though we managed to land it in rust-lang#71528 and that doesn't seem to be causing too many fires, so I'm re-sending this patch!
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.
From
.bc.zfile in the rlib to a section in the.ofile in the rlib.This is the main part of #66961. (The other part is changing Cargo to use the new
-Cembed-bitcode=noflag when appropriate.)r? @alexcrichton