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

Don't force rustc to do codegen for LTO builds #8192

Merged
merged 1 commit into from May 4, 2020

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented May 1, 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.

@rust-highfive
Copy link

rust-highfive commented May 1, 2020

r? @Eh2406

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

@rust-highfive rust-highfive added the S-waiting-on-review label May 1, 2020
let supports_bitcode_in_rlib = match kind {
CompileKind::Host => Some(rustc.cached_output(&bitcode_in_rlib_test).is_ok()),
let mut embed_bitcode_test = process.clone();
embed_bitcode_test.arg("-Cbitcode-in-rlib");
Copy link
Member Author

@alexcrichton alexcrichton May 1, 2020

Choose a reason for hiding this comment

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

Note that the naming here is intentionally a bit odd. This is being renamed in rust-lang/rust#71716 (and will get backported to beta). The naming here is intended to represent what should be the final state of things while also working with today's nightly.

@Eh2406
Copy link
Contributor

Eh2406 commented May 1, 2020

I don't know this well enough to know it works from a review, but it looks good to me. So r+ it when you and CI are happy.

@alexcrichton
Copy link
Member Author

alexcrichton commented May 1, 2020

r? @ehuss

Copy link
Contributor

@ehuss ehuss left a comment

I think it might be good to add public functions to all the deps in the complicated test, and call them from main.rs, to ensure the linker is able to actually pull the symbols in and use them.

There is one edge case that seems to break. Setting lto = "off" (or n or no) causes Cargo to behave as-if LTO is enabled, and linking fails because there is no object code (the linker crashes for me on macos). Perhaps that case should be handled? I'm uncertain about the exact use cases, but I know it is sometimes used to disable thin-local LTO.

src/cargo/core/compiler/context/mod.rs Outdated Show resolved Hide resolved
tests/testsuite/lto.rs Show resolved Hide resolved
@alexcrichton
Copy link
Member Author

alexcrichton commented May 1, 2020

There is one edge case that seems to break

Wow I had no idea that worked or was possible!

In any case, should be updated now. Thanks for taking a look!

Copy link
Contributor

@ehuss ehuss left a comment

I just want to double check. Because the computed Lto value is not in the fingerprint, artifacts can end up with different results based on the order of commands. Is that OK?

For example, cargo build --release --lib will build the lib with -Cbitcode-in-rlib=no, and then cargo build --release --bin test will link with that lib missing the bitcode. That seems ok, unless you are doing cross-lang LTO, right?

I dunno, I'm just feeling like it might be safer to include the Lto value in the fingerprint, just so that if the order of commands changes things it will rebuild. Or maybe the Lto computation could be more conservative and assume if lto is set in the profile that it should use Lto::EmbedBitcode instead of Lto::None if require_bitcode is false.

Or, maybe if lto is set in the profile, maybe it should assume require_bitcode is true for all the (non-host) roots? That way, I think the order of commands would never change things.

assert!(!require_bitcode); // can't depend on binaries/staticlib/etc
match unit.profile.lto {
profiles::Lto::Named(s) => match s.as_str() {
"n" | "no" | "off" => (Lto::None, false),
Copy link
Contributor

@ehuss ehuss May 1, 2020

Choose a reason for hiding this comment

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

I think this changes the semantics so that lto="off" is changed to be thin-local LTO (by not passing the lto flag). I suspect to properly handle it will require another enum variant.

Copy link
Member Author

@alexcrichton alexcrichton May 4, 2020

Choose a reason for hiding this comment

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

I continue to learn more about what this flag does every day.

Copy link
Member Author

@alexcrichton alexcrichton May 4, 2020

Choose a reason for hiding this comment

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

Hm so I'll switch this to keep passing -Clto=off, but tbh that probably isn't what you want configuring lto="off". You'd probably also want to do that for dependencies but we don't even do that today

@alexcrichton
Copy link
Member Author

alexcrichton commented May 4, 2020

Hm no that is not ok. That's quite subtle and pretty unfortunate that what I thought was a natural way to implement this doesn't actually work out. We'll definitely need to include it in the fingerprint.

FWIW the main purpose of this PR is to not do bitcode embedding (save on compile time), so I think we don't want to overapproximate what needs to be done (like we do today).

@alexcrichton
Copy link
Member Author

alexcrichton commented May 4, 2020

Ok I've updated with a test to ensure fingerprints are handled correctly and passing through -Clto=off as well.

@bors
Copy link
Contributor

bors commented May 4, 2020

The latest upstream changes (presumably #8204) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -40,8 +40,8 @@ pub struct TargetInfo {
pub rustflags: Vec<String>,
/// Extra flags to pass to `rustdoc`, see `env_args`.
pub rustdocflags: Vec<String>,
/// Remove this when it hits stable (1.44)
pub supports_bitcode_in_rlib: Option<bool>,
/// Remove this when it hits stable (1.45)
Copy link
Contributor

@ehuss ehuss May 4, 2020

Choose a reason for hiding this comment

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

If rust-lang/rust#71716 is backported to beta, shouldn't this still be 1.44?

Copy link
Member Author

@alexcrichton alexcrichton May 4, 2020

Choose a reason for hiding this comment

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

It's unfortunately a bit confusing, but this flag is also being used as a proxy for "rustc can read its own -Clinker-plugin-lto object files", which is functionality on nightly that is not being backported. Only the option rename is being backported to beta.

Copy link
Member Author

@alexcrichton alexcrichton May 4, 2020

Choose a reason for hiding this comment

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

Er actually turns out that the backport isn't necessary becuase -Cbitcode-in-rlib isn't even on beta.

@ehuss
Copy link
Contributor

ehuss commented May 4, 2020

r=me with conflicts resolved

I'm not 100% confident this won't end up causing unwanted rebuilds. But the only scenario I can think of is build --lib then build --bin, and that seems like it would be very rare.

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.
@alexcrichton
Copy link
Member Author

alexcrichton commented May 4, 2020

@bors: r=ehuss

@bors
Copy link
Contributor

bors commented May 4, 2020

📌 Commit e221925 has been approved by ehuss

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels May 4, 2020
@bors
Copy link
Contributor

bors commented May 4, 2020

Testing commit e221925 with merge 91dc1e1...

@bors
Copy link
Contributor

bors commented May 4, 2020

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 91dc1e1 to master...

@bors bors merged commit 91dc1e1 into rust-lang:master May 4, 2020
12 checks passed
bors added a commit to rust-lang-ci/rust that referenced this issue May 6, 2020
Update cargo

6 commits in 258c89644c4587273a3ed3ee9522d2640facba43..d7966eb004571b5464efece65867b66aa01b5d7b
2020-04-30 21:48:21 +0000 to 2020-05-05 02:52:21 +0000
- features: allow activated_features_unverified to communicate not-present (rust-lang/cargo#8194)
- Don't force rustc to do codegen for LTO builds (rust-lang/cargo#8192)
- Hint git-fetch-with-cli on git errors (rust-lang/cargo#8166)
- ¬∃x. ¬y =&gt; ∀x. y (rust-lang/cargo#8205)
- clippy fixes (rust-lang/cargo#8189)
- Rename bitcode-in-rlib flag to embed-bitcode (rust-lang/cargo#8204)
bors added a commit to rust-lang-ci/rust that referenced this issue May 7, 2020
Update cargo

7 commits in 258c89644c4587273a3ed3ee9522d2640facba43..f534844c25cacc5e004404cea835ac85e35ca3fd
2020-04-30 21:48:21 +0000 to 2020-05-06 14:39:10 +0000
- Avoid testing git-specific error messages (rust-lang/cargo#8212)
- features: allow activated_features_unverified to communicate not-present (rust-lang/cargo#8194)
- Don't force rustc to do codegen for LTO builds (rust-lang/cargo#8192)
- Hint git-fetch-with-cli on git errors (rust-lang/cargo#8166)
- ¬∃x. ¬y =&gt; ∀x. y (rust-lang/cargo#8205)
- clippy fixes (rust-lang/cargo#8189)
- Rename bitcode-in-rlib flag to embed-bitcode (rust-lang/cargo#8204)
bors added a commit to rust-lang-ci/rust that referenced this issue May 7, 2020
Update cargo

7 commits in 258c89644c4587273a3ed3ee9522d2640facba43..f534844c25cacc5e004404cea835ac85e35ca3fd
2020-04-30 21:48:21 +0000 to 2020-05-06 14:39:10 +0000
- Avoid testing git-specific error messages (rust-lang/cargo#8212)
- features: allow activated_features_unverified to communicate not-present (rust-lang/cargo#8194)
- Don't force rustc to do codegen for LTO builds (rust-lang/cargo#8192)
- Hint git-fetch-with-cli on git errors (rust-lang/cargo#8166)
- ¬∃x. ¬y =&gt; ∀x. y (rust-lang/cargo#8205)
- clippy fixes (rust-lang/cargo#8189)
- Rename bitcode-in-rlib flag to embed-bitcode (rust-lang/cargo#8204)
@ehuss ehuss mentioned this pull request Jun 10, 2020
bors added a commit that referenced this issue Jun 11, 2020
Some LTO fixes.

This reworks the LTO computation a little to address a few issues:

- `cargo build` in a project with both a lib and bin would not engage the optimization introduced in #8192 where the lib *should* be compiled with `-C linker-plugin-lto` (bitcode only). This happened because the old code was starting root units as `Lto::None`. The solution here is to conditionally choose the starting Lto for roots.
- A project with a dylib dependency would fail to build. It was building the dylib with `-C linker-plugin-lto` which is not valid.
- A project with a bin/lib would build the lib differently based on whether or not it was selected. This changes it so that the lib is built the same. See `lto::between_builds`, where the second build the lib is now fresh.
- Tests/benchmarks of a `lib` target will now support LTO.
- Treats example libs a little more consistently as regular libs.

I scattered some comments throughout, hopefully it's not too difficult to follow.

Closes #8337
@alexcrichton alexcrichton deleted the lto-optimizations branch Jul 29, 2020
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants