Use llvm::computeLTOCacheKey to determine post-ThinLTO CGU reuse #76859
Conversation
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.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit cfe07cd with merge f35705fb01020929514f970af7f0c5878c90cb37... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued f35705fb01020929514f970af7f0c5878c90cb37 with parent f3c923a, future comparison URL. |
|
Finished benchmarking try commit (f35705fb01020929514f970af7f0c5878c90cb37): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
r? @pnkfelix |
|
cc @rust-lang/wg-incr-comp -- could we find a reviewer for this PR? It's blocking re-enabling debug assertions in CI on some builders (#75199). |
|
@nikic Does this look reasonable to you? |
|
Looks very reasonable, and is hopefully the end of the road for this class of problems... @bors r=davidtwco,nikic |
|
📌 Commit cfe07cd has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
|
Presumably due to finer-grained tracking in some cases, this was a major improvement on some incremental tests. Of course, the correctness is much more important here :) |
…youxu Call module_name_to_str instead of just unwrapping This makes the ICE message in rust-lang#130678 more clear. It looks like not calling this function was just an oversight in rust-lang#76859, but clearly not a major one because it's taken us 4 years to notice.
Call module_name_to_str instead of just unwrapping This makes the ICE message in rust-lang#130678 more clear. It looks like not calling this function was just an oversight in rust-lang#76859, but clearly not a major one because it's taken us 4 years to notice. try-job: i686-msvc
Call module_name_to_str instead of just unwrapping This makes the ICE message in rust-lang#130678 more clear. It looks like not calling this function was just an oversight in rust-lang#76859, but clearly not a major one because it's taken us 4 years to notice. try-job: i686-msvc
Call module_name_to_str instead of just unwrapping This makes the ICE message in rust-lang/rust#130678 more clear. It looks like not calling this function was just an oversight in rust-lang/rust#76859, but clearly not a major one because it's taken us 4 years to notice. try-job: i686-msvc
Call module_name_to_str instead of just unwrapping This makes the ICE message in rust-lang/rust#130678 more clear. It looks like not calling this function was just an oversight in rust-lang/rust#76859, but clearly not a major one because it's taken us 4 years to notice. try-job: i686-msvc
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 #67020 and PR #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::computeLTOCacheKeyis used to compute a SHA-1 hashfrom 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 #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.