Do not reuse post LTO products when exports change #71131
Conversation
…file in incremental compilation. This is symmetric to PR rust-lang#67020, which handled the case where the LLVM module's *imports* changed. This commit builds upon the infrastructure added there; the export map is just the inverse of the import map, so we can build the export map at the same time that we load the serialized import map. Fix rust-lang#69798
Namely, a regression test for issue rust-lang#69798 (export added), and the inverse of that test (export removd).
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors rollup=never This may affect performance, especially of incremental compilations. |
pnkfelix
added
the
beta-nominated
|
nominating for beta backport; this bug is old, but it was exposed by a recent change (PR #67332), which has not made it to stable yet. |
|
r? @alexcrichton or @michaelwoerister (if you are still around) |
|
I think both @michaelwoerister and @alexcrichton have scaled back their involvement with the project. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 12207f6 with merge c8085c9fa29609558b4745fc37c7a14c335e0ec3... |
|
The symmetry here makes sense, r=me. |
|
☀️ Try build successful - checks-azure |
|
Queued c8085c9fa29609558b4745fc37c7a14c335e0ec3 with parent edc0258, future comparison URL. |
Renamed the struct to make it a little clearer that it doesn't just hold one imports map. (I couldn't bring myself to write it as `ThinLTOImportsExports` though, mainly since the exports map is literally derived from the imports map data.) Added some doc to the struct too. Revised comments to add link to the newer issue that discusses why the exports are relevant. Renamed a few of the methods so that the two character difference is more apparent (because 1. the method name is shorter and, perhaps more importantly, the changed characters now lie at the beginning of the method name.)
Wow, these results seem so bad that I'm having a hard time believing them to be true...
|
spastorino
added
the
T-compiler
|
Is that because |
|
Post rust-lang/rustc-perf#645 retry: |
|
Awaiting bors try build completion |
|
I'm going to land this as is for now, and look into the performance regression as a follow-up work item. |
|
@bors r=nagisa rollup=never |
|
📌 Commit d05ae3a has been approved by |
bors
added
S-waiting-on-bors
pnkfelix
added
the
beta-accepted
|
@bors p=2 (this gets slightly elevated priority because we want to backport it in time for next week's release, and I really dislike backporting things that have not landed in nightly. It remains rollup=never due to potential for performance impact.) |
|
⌛ Testing commit d05ae3a with merge 73ee43c429079a6fb8f4c00eb04c5934bd1c8abb... |
|
@bors retry (yielding this for a rollup, will run this pr after that, when i'm asleep so that there is no need to monitor the queue :P ) |
|
@rust-timer build 4909ccc0a8d5affc76bd6e900e76f9a46d0e3e9e |
|
Queued 4909ccc0a8d5affc76bd6e900e76f9a46d0e3e9e with parent 4e4d49d, future comparison URL. |
|
Finished benchmarking try commit 4909ccc0a8d5affc76bd6e900e76f9a46d0e3e9e, comparison URL. |
|
☀️ Test successful - checks-azure |
Mark-Simulacrum
removed
the
beta-nominated
…ulacrum [beta] backports This includes: * Do not reuse post LTO products when exports change rust-lang#71131 * macro_rules: `NtLifetime` cannot start with an identifier rust-lang#70768 * Update RELEASES.md for 1.43.0 rust-lang#70354 r? @ghost
During incremental ThinLTO compilation, we attempt to re-use the optimized (post-ThinLTO) bitcode file for a module if it is 'safe' to do so. Up until now, 'safe' has meant that the set of modules that our current modules imports from/exports to is unchanged from the previous compilation session. See PR rust-lang#67020 and PR rust-lang#71131 for more details. However, this turns out be insufficient to guarantee that it's safe to reuse the post-LTO module (i.e. that optimizing the pre-LTO module would produce the same result). When LLVM optimizes a module during ThinLTO, it may look at other information from the 'module index', such as whether a (non-imported!) global variable is used. If this information changes between compilation runs, we may end up re-using an optimized module that (for example) had dead-code elimination run on a function that is now used by another module. Fortunately, LLVM implements its own ThinLTO module cache, which is used when ThinLTO is performed by a linker plugin (e.g. when clang is used to compile a C proect). Using this cache directly would require extensive refactoring of our code - but fortunately for us, LLVM provides a function that does exactly what we need. The function `llvm::computeLTOCacheKey` is used to compute a SHA-1 hash from all data that might influence the result of ThinLTO on a module. In addition to the module imports/exports that we manually track, it also hashes information about global variables (e.g. their liveness) which might be used during optimization. By using this function, we shouldn't have to worry about new LLVM passes breaking our module re-use behavior. In LLVM, the output of this function forms part of the filename used to store the post-ThinLTO module. To keep our current filename structure intact, this PR just writes out the mapping 'CGU name -> Hash' to a file. To determine if a post-LTO module should be reused, we compare hashes from the previous session. This should unblock PR rust-lang#75199 - by sheer chance, it seems to have hit this issue due to the particular CGU partitioning and optimization decisions that end up getting made.
…twco,nikic Use llvm::computeLTOCacheKey to determine post-ThinLTO CGU reuse During incremental ThinLTO compilation, we attempt to re-use the optimized (post-ThinLTO) bitcode file for a module if it is 'safe' to do so. Up until now, 'safe' has meant that the set of modules that our current modules imports from/exports to is unchanged from the previous compilation session. See PR rust-lang#67020 and PR rust-lang#71131 for more details. However, this turns out be insufficient to guarantee that it's safe to reuse the post-LTO module (i.e. that optimizing the pre-LTO module would produce the same result). When LLVM optimizes a module during ThinLTO, it may look at other information from the 'module index', such as whether a (non-imported!) global variable is used. If this information changes between compilation runs, we may end up re-using an optimized module that (for example) had dead-code elimination run on a function that is now used by another module. Fortunately, LLVM implements its own ThinLTO module cache, which is used when ThinLTO is performed by a linker plugin (e.g. when clang is used to compile a C proect). Using this cache directly would require extensive refactoring of our code - but fortunately for us, LLVM provides a function that does exactly what we need. The function `llvm::computeLTOCacheKey` is used to compute a SHA-1 hash from all data that might influence the result of ThinLTO on a module. In addition to the module imports/exports that we manually track, it also hashes information about global variables (e.g. their liveness) which might be used during optimization. By using this function, we shouldn't have to worry about new LLVM passes breaking our module re-use behavior. In LLVM, the output of this function forms part of the filename used to store the post-ThinLTO module. To keep our current filename structure intact, this PR just writes out the mapping 'CGU name -> Hash' to a file. To determine if a post-LTO module should be reused, we compare hashes from the previous session. This should unblock PR rust-lang#75199 - by sheer chance, it seems to have hit this issue due to the particular CGU partitioning and optimization decisions that end up getting made.
Do not reuse post lto products when exports change
Generalizes code from PR #67020, which handled case when imports change.
Fix #69798