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

LTO behavior depends on root targets #8337

Closed
ehuss opened this issue Jun 5, 2020 · 7 comments · Fixed by #8349
Closed

LTO behavior depends on root targets #8337

ehuss opened this issue Jun 5, 2020 · 7 comments · Fixed by #8349
Labels
A-lto Area: link-time optimization

Comments

@ehuss
Copy link
Contributor

ehuss commented Jun 5, 2020

Comparing these two commands building Cargo itself with lto='thin':

  1. cargo build --release
  2. cargo build --release --bin cargo

The first command does not engage the LTO codegen optimization added in #8192. On my system, the total build time goes from 187s to 150s.

It is a bit surprising, since the two commands generally produce the same output, so I would not expect them to use different settings.

Would it make sense to be a little smarter about how the root units are chosen? I think the crux of the issue is this line where it starts the lib at Lto::None in the first case. In the second case, it is computed as a dependency with Lto::OnlyBitcode.

@ehuss ehuss added the A-lto Area: link-time optimization label Jun 5, 2020
@alexcrichton
Copy link
Member

I agree that we should probably fix this since it's otherwise a bit surprising that your "main" library is so different from other libraries. I also agree that it's due to that root issue you pointed out.

I think it may be a possible solution to specify all roots as OnlyBitcode? That way we'd guarantee that root libraries are at least compiled.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 8, 2020

Hm. Starting with OnlyBitcode doesn't work in many situations (tests, dylib only, etc.). I'll try to whittle it down, but I'm thinking of moving the if block into a separate function and calling that to prime the initial value. It's quite tricky.

Do you have a preference on how check is handled? AFAIK, it ignores any lto-related flags, so I'm not sure it matters too much.

@ehuss
Copy link
Contributor Author

ehuss commented Jun 8, 2020

Double hmm... Building a project with a dylib dependency with LTO is currently broken. It builds the dylib with -C linker-plugin-lto, but that chokes during linking. Should that be EmbedBitcode or None? I'm not sure it matters too much, but I'm guessing EmbedBitcode?

Also, would you mind too much if we rename the Lto variants, I'm constantly having to look up what they mean. Maybe something like:

Lto::Run              // -C lto
Lto::OnlyBitcode      // -C linker-plugin-lto
Lto::ObjectAndBitcode // no flags
Lto::OnlyObject       // -C embed-bitcode=no

@ehuss
Copy link
Contributor Author

ehuss commented Jun 10, 2020

Just an update, I've been fiddling with this for a while, and I think I have something that should work better. I'll try to work on some more tests tomorrow and see if I can post a PR soon.

@alexcrichton
Copy link
Member

Hm for dylib dependencies I think it needs to be None. They don't participate in LTO at all, but IIRC you can't Rust-LTO dependency graphs with dylibs in them, the compiler should reject that?

@ehuss ehuss mentioned this issue Jun 10, 2020
@bors bors closed this as completed in ee417cb Jun 11, 2020
@xobs
Copy link

xobs commented Aug 30, 2020

I'm still experiencing this on 1.46. I had a repo that reproduced the issue on 1.45, and I was just about to file the issue when 1.46 was released. #8349 fixed my test case, however I have since noticed that it does not fix my actual project:

[5:57:06 PM] D:/Code/Xous/core> cargo build --release -p shell
   Compiling cfg-if v0.1.10
   Compiling getrandom v0.1.14
   Compiling log v0.4.11
   Compiling rand_core v0.5.1
   Compiling rand_chacha v0.2.2
   Compiling rand_pcg v0.2.1
   Compiling rand v0.7.3
   Compiling xous-macros v0.1.0 (D:\Code\Xous\core\macros)
   Compiling xous v0.1.0 (D:\Code\Xous\core\xous-rs)
   Compiling graphics-server v0.1.0 (D:\Code\Xous\core\examples\graphics-server)
   Compiling shell v0.1.0 (D:\Code\Xous\core\examples\shell)
    Finished release [optimized + debuginfo] target(s) in 25.41s
[5:57:35 PM] D:/Code/Xous/core> cargo build --release -p log-server
   Compiling cfg-if v0.1.10
   Compiling log v0.4.11
   Compiling xous v0.1.0 (D:\Code\Xous\core\xous-rs)
   Compiling log-server v0.1.0 (D:\Code\Xous\core\examples\log-server)
    Finished release [optimized + debuginfo] target(s) in 15.39s
[5:57:52 PM] D:/Code/Xous/core> cargo build --release -p shell
   Compiling cfg-if v0.1.10
   Compiling getrandom v0.1.14
   Compiling log v0.4.11
   Compiling rand_core v0.5.1
   Compiling rand_chacha v0.2.2
   Compiling rand_pcg v0.2.1
   Compiling rand v0.7.3
   Compiling xous-macros v0.1.0 (D:\Code\Xous\core\macros)
   Compiling xous v0.1.0 (D:\Code\Xous\core\xous-rs)
   Compiling graphics-server v0.1.0 (D:\Code\Xous\core\examples\graphics-server)
   Compiling shell v0.1.0 (D:\Code\Xous\core\examples\shell)
    Finished release [optimized + debuginfo] target(s) in 27.15s
[5:58:21 PM] D:/Code/Xous/core>

If I disable lto="thin" in my Cargo.toml it works correctly:

[5:58:21 PM] D:/Code/Xous/core> cargo build --release -p log-server
   Compiling log-server v0.1.0 (D:\Code\Xous\core\examples\log-server)
    Finished release [optimized + debuginfo] target(s) in 4.14s
[5:58:45 PM] D:/Code/Xous/core> cargo build --release -p shell
   Compiling shell v0.1.0 (D:\Code\Xous\core\examples\shell)
    Finished release [optimized + debuginfo] target(s) in 0.77s
[5:58:47 PM] D:/Code/Xous/core> cargo build --release -p log-server
    Finished release [optimized + debuginfo] target(s) in 0.19s
[5:58:49 PM] D:/Code/Xous/core> cargo build --release -p shell
    Finished release [optimized + debuginfo] target(s) in 0.27s
[5:58:56 PM] D:/Code/Xous/core>

The repository is available at https://github.com/betrusted-io/xous-core and the above commands can be used to demonstrate the issue.

One interesting thing to note is that both log-server and shell depend on xous, which has a feature that depends on log. If I remove this feature (by setting default = [] in xous-rs/Cargo.toml), then the rebuild works just fine with lto enabled.

@ehuss
Copy link
Contributor Author

ehuss commented Sep 1, 2020

@xobs, can you file a new issue? It doesn't seem directly related to this.

Also, briefly tried your repro steps, but it didn't cause any rebuilds.

You can run cargo with the CARGO_LOG=cargo::core::compiler::fingerprint=trace environment variable set to have it explain why it thinks it needs to rebuild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lto Area: link-time optimization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants