Skip to content
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

Store LLVM bitcode in object files, not compressed #71528

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

alexcrichton
Copy link
Member

This commit is an attempted resurrection of #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 #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.

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2020
@alexcrichton
Copy link
Member Author

As an extra note, I've tested this on Windows/mac/linux and confirmed that where bytecode sections previously appeared they're now all gone by default. Additionally I've tested that the builders which originally failed on #70458 pass with this implementation.

@petrochenkov
Copy link
Contributor

Nit: I'm not sure why the option name changed since it was approved in #70458.
Is there case for emitting bitcode into other library kinds in the future making the -in-rlib part obsolete?

@alexcrichton
Copy link
Member Author

I'm not really sure, most of this bytecode embedding stuff has been driven by demand and it's hard to predict future demands. It's also not on stable yet, so if we want to switch up the name that's also fine. Given the fall out in #70458 though I would personally prefer to leave refactorings/etc to a future PR to make sure this actually can land first (and doesn't need to be backed out)

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

#70458 had a commit "Don't copy bytecode files into the incr. comp. cache." I think that could be applied here?

Can the -Zembed-bitcode flag be removed now? Perhaps in a follow-up?

src/librustc_codegen_llvm/back/write.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member Author

Ah yeah I saw the extra patch, although I'm personally not very familiar with it. I was also hoping that given the hiccups we saw earlier we probably don't want to design too much around this until we're more sure that it works. WDYT about landing this first, making sure it doesn't wildly break everyone on nightly, and then we can land some cleanups which this unlocks?

@ecstatic-morse
Copy link
Contributor

r? @nnethercote

@@ -129,8 +129,8 @@ impl<'a> ArchiveBuilder<'a> for LlvmArchiveBuilder<'a> {
let obj_start = name.to_owned();

self.add_archive(rlib, move |fname: &str| {
// Ignore bytecode/metadata files, no matter the name.
if fname.ends_with(RLIB_BYTECODE_EXTENSION) || fname == METADATA_FILENAME {
// Ignore metadata files, no matter the name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Ignore metadata files, no matter the name.
// Ignore metadata files

@@ -1829,7 +1811,7 @@ fn add_upstream_rust_crates<'a, B: ArchiveBuilder<'a>>(

let mut any_objects = false;
for f in archive.src_files() {
if f.ends_with(RLIB_BYTECODE_EXTENSION) || f == METADATA_FILENAME {
if f == METADATA_FILENAME {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, one less reason to rewrite rlibs every time we link. I wonder if there is a way for the linker to skip the metadata file without having to remove it from the archive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not true, rlibs are already passed directly to the linker. Only in situations like LTO where we want to remove object code or we're doing something -force_load do we remove things from rlibs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed the return above.

@nnethercote
Copy link
Contributor

WDYT about landing this first, making sure it doesn't wildly break everyone on nightly, and then we can land some cleanups which this unlocks?

Sounds good. That way we can also see if the extra patch has any measurable perf impact. I will review in a bit.

src/librustc_codegen_llvm/back/lto.rs Outdated Show resolved Hide resolved
src/test/ui/lto-rustc-loads-linker-plugin.rs Outdated Show resolved Hide resolved
src/test/ui/lto-thin-rustc-loads-linker-plugin.rs Outdated Show resolved Hide resolved
src/librustc_codegen_ssa/back/write.rs Show resolved Hide resolved
@nnethercote
Copy link
Contributor

One more thing: src/doc/rustc/src/codegen-options/index.md mentions compressed bitcode for the bitcode-in-rlib option, please update that. (And a gold star if you could move that section up near the top so it's in alphabetical order; I missed doing that in #71374.)

@alexcrichton alexcrichton force-pushed the no-more-bitcode branch 2 times, most recently from da0af77 to 95fd968 Compare April 28, 2020 18:45
@alexcrichton
Copy link
Member Author

Ok updated!

@nnethercote
Copy link
Contributor

Follow-ups required:

  • Possibly rename -Cbitcode-in-rlib as -Cembed-bitcode (and removed -Zembed-bitcode). This will require backporting this to beta?
  • The "Don't copy bytecode files into the incr. comp. cache." commit from Move LLVM bitcode destination #70458.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 28, 2020

📌 Commit 95fd968d9c95e1e5d872f2f0d5378f38ff658f86 has been approved by nnethercote

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2020
@nnethercote
Copy link
Contributor

@bors p=1

Because follow-ups to beta will be necessary if this lands successfully.

@bors
Copy link
Contributor

bors commented Apr 29, 2020

⌛ Testing commit ef89cc8 with merge 1357af3...

@bors
Copy link
Contributor

bors commented Apr 30, 2020

☀️ Test successful - checks-azure
Approved by: nnethercote
Pushing 1357af3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 30, 2020
@bors bors merged commit 1357af3 into rust-lang:master Apr 30, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #71528!

Tested on commit 1357af3.
Direct link to PR: #71528

🎉 rustc-dev-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Apr 30, 2020
Tested on commit rust-lang/rust@1357af3.
Direct link to PR: <rust-lang/rust#71528>

🎉 rustc-dev-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m).
@alexcrichton alexcrichton deleted the no-more-bitcode branch April 30, 2020 14:28
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!
alexcrichton added a commit to alexcrichton/cargo that referenced this pull request May 4, 2020
This commit updates Cargo's implementation of LTO builds to do less work
and hopefully be faster when doing a cold build. Additionaly this should
save space on disk! The general idea is that the compiler does not need
object files if it's only going to perform LTO with some artifacts. In
this case all rustc needs to do is load bitcode from dependencies. This
means that if you're doing an LTO build generating object code for
intermediate dependencies is just wasted time!

Here Cargo is updated with more intrusive knowledge about LTO. Cargo
will now analyze the dependency graph to figure out which crates are
being compiled with LTO, and then it will figure out which dependencies
only need to have bitcode in them. Pure-bitcode artifacts are emitted
with the `-Clinker-plugin-lto` flag. Some artifacts are still used in
multiple scenarios (such as those shared between build scripts and final
artifacts), so those are not compiled with `-Clinker-plugin-lto` since
the linker is not guaranteed to know how to perform LTO.

This functionality was recently implemented in rust-lang/rust#71528
where rustc is now capable of reading bitcode from `-Clinker-plugin-lto`
rlibs. Previously rustc would only read its own format of bitcode, but
this has now been extended! This support is now on nightly, hence this
PR.
bors added a commit to rust-lang/cargo that referenced this pull request May 4, 2020
Don't force rustc to do codegen for LTO builds

This commit updates Cargo's implementation of LTO builds to do less work
and hopefully be faster when doing a cold build. Additionaly this should
save space on disk! The general idea is that the compiler does not need
object files if it's only going to perform LTO with some artifacts. In
this case all rustc needs to do is load bitcode from dependencies. This
means that if you're doing an LTO build generating object code for
intermediate dependencies is just wasted time!

Here Cargo is updated with more intrusive knowledge about LTO. Cargo
will now analyze the dependency graph to figure out which crates are
being compiled with LTO, and then it will figure out which dependencies
only need to have bitcode in them. Pure-bitcode artifacts are emitted
with the `-Clinker-plugin-lto` flag. Some artifacts are still used in
multiple scenarios (such as those shared between build scripts and final
artifacts), so those are not compiled with `-Clinker-plugin-lto` since
the linker is not guaranteed to know how to perform LTO.

This functionality was recently implemented in rust-lang/rust#71528
where rustc is now capable of reading bitcode from `-Clinker-plugin-lto`
rlibs. Previously rustc would only read its own format of bitcode, but
this has now been extended! This support is now on nightly, hence this
PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants