Ignore incremental when lto is set. #6643
Conversation
|
r? @dwijnand (rust_highfive has picked a reviewer for you, use r? to override) |
|
LGTM, but looks like the kind of thing we'd want Alex's eyes on. |
|
Thanks for this! Everything looks great here to me, but would it be possible to hold off on the final change for disabling incremental with LTO for a few days? I think this is something we'll actually want to fix in upstream rustc, especially before turning incremental release mode on by default. I think the fix shouldn't be too hard in theory too! I was also under the impression that this wasn't coming up too much in practice, but if it is coming up a lot then we should probably go ahead and land anyway |
|
Absolutely, there is no rush at all. I don't think it's coming up much at all. Can you say more what rustc would do? Would it also ignore incremental, or is it being changed so that incremental works with lto? |
|
Oh I'd ideally just like to get incremental working with LTO. The compiler could at least be incremental up to the point that it executes LTO, which is as good of an interpretation as we can make of incremental + LTO |
|
My intention is that rust-lang/rust#58378 makes the behavior change here obsolete as LTO + incremental should work, but this contains good refactorings regardless and would be good to land! |
Currently the compiler will produce an error if both incremental compilation and full fat LTO is requested. With recent changes and the advent of incremental ThinLTO, however, all the hard work is already done for us and it's actually not too bad to remove this error! This commit updates the codegen backend to allow incremental full fat LTO. The semantics are that the input modules to LTO are all produce incrementally, but the final LTO step is always done unconditionally regardless of whether the inputs changed or not. The only real incremental win we could have here is if zero of the input modules changed, but that's so rare it's unlikely to be worthwhile to implement such a code path. cc rust-lang#57968 cc rust-lang/cargo#6643
…haelwoerister rustc: Implement incremental "fat" LTO Currently the compiler will produce an error if both incremental compilation and full fat LTO is requested. With recent changes and the advent of incremental ThinLTO, however, all the hard work is already done for us and it's actually not too bad to remove this error! This commit updates the codegen backend to allow incremental full fat LTO. The semantics are that the input modules to LTO are all produce incrementally, but the final LTO step is always done unconditionally regardless of whether the inputs changed or not. The only real incremental win we could have here is if zero of the input modules changed, but that's so rare it's unlikely to be worthwhile to implement such a code path. cc rust-lang#57968 cc rust-lang/cargo#6643
|
☔ The latest upstream changes (presumably #6687) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Since rust-lang/rust#58378 ended up landing the functional change here I think is no longer necessary, but the refactorings still look good to me. @ehuss want to rebase and we can merge? |
|
Closing in favor of #6688. |
Incremental profile cleanup. Some minor code cleanup, and doc updates regarding incremental compilation. Move incremental logic into the profile computation, which makes it a little more consistent and helps simplify things a little (such as removing the fingerprint special-case). This introduces a small change in behavior with the `build.incremental` config variable. Previously `build.incremental = true` was completely ignored. Now, it is treated the same as `CARGO_INCREMENTAL=1` environment variable. Moves config profile validation to the workspace so that warnings do not appear multiple times (once for each manifest). Split from #6643.
This makes it so that incremental compilation is forcefully disabled if
ltois enabled, instead of failing to compile.This moves incremental logic into the profile computation, which makes it a little more consistent and helps simplify things a little (such as removing the fingerprint special-case).
This also introduces a small change in behavior with the
build.incrementalconfig variable. Previouslybuild.incremental = truewas completely ignored. Now, it is treated the same asCARGO_INCREMENTAL=1environment variable.Another small change is to move config profile validation to the workspace so that warnings do not appear multiple times (once for each manifest).
Fixes #4255