Fix linker-plugin-lto only doing thin lto #136840
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could not assign reviewer from: |
rustbot
added
A-run-make
|
This PR modifies cc @jieyouxu |
21 tasks
|
I have barely any idea about LTO besides "it happens and it involves dlopening a compiler and shoving its serialized data back in it" tbh soo |
|
Unfortunately I have no clue either, so r? compiler |
|
For reference, the code that switches to thin lto when the flag is not set is here: https://github.com/llvm/llvm-project/blob/e258bca9505f35e0a22cb213a305eea9b76d11ea/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4441-L4446 // By default we compile with ThinLTO if the module has a summary, but the
// client can request full LTO with a module flag.
bool IsThinLTO = true;
if (auto *MD =
mdconst::extract_or_null<ConstantInt>(M.getModuleFlag("ThinLTO")))
IsThinLTO = MD->getZExtValue();The code in clang that sets the flag, which is replicated here for Rust is here: https://github.com/llvm/llvm-project/blob/560149b5e3c891c64899e9912e29467a69dc3a4c/clang/lib/CodeGen/BackendUtil.cpp#L1150 if (!TheModule->getModuleFlag("ThinLTO") && !CodeGenOpts.UnifiedLTO)
TheModule->addModuleFlag(llvm::Module::Error, "ThinLTO", uint32_t(0)); |
bjorn3
reviewed
Feb 11, 2025
| // Disable ThinLTO if fat lto is requested. Otherwise lld defaults to thin lto. | ||
| if sess.lto() == config::Lto::Fat { | ||
| llvm::add_module_flag_u32(llmod, llvm::ModuleFlagMergeBehavior::Override, "ThinLTO", 0); | ||
| } |
There was a problem hiding this comment.
What if a dependency is built with lto=true (aka lto=fat), but then the user wants to use thinLTO? I'm pretty sure the standard library is built with lto=true for example, but that shouldn't prevent thinLTO from ever working.
There was a problem hiding this comment.
Good question, it seems to change somewhat, but still work in general. I added a test for this.
What changes: Without this change, the test passes when
lib is compiled with O0 and main with O3 and
- lib uses lto=thin and main uses lto=thin
- lib uses lto=thin and main uses lto=fat
- lib uses lto=fat and main uses lto=thin
- lib uses lto=fat and main uses lto=fat
With this change, all of these keep passing except for case 3 (lib uses lto=fat and main uses lto=thin).
When lib is compiled with O1, O2 or O3, case 3 passes as well.
I assume this is the important case, as the standard library is compiled with optimizations.
(And lto with O0 is kinda questionable, except maybe for nvptx and amdgpu, but they require lto=fat anyway.)
bjorn3
reviewed
Feb 11, 2025
This comment has been minimized.
This comment has been minimized.
|
r? compiler |
SparrowLii
reviewed
Feb 17, 2025
| @@ -290,6 +290,11 @@ pub(crate) unsafe fn create_module<'ll>( | |||
| ); | |||
| } | |||
|
|
|||
| // Disable ThinLTO if fat lto is requested. Otherwise lld defaults to thin lto. | |||
There was a problem hiding this comment.
That sounds counterintuitive. Can you explain the relationship between the user's lto option and llvm's lto in the comments?
And I think it needs a individual test to ensure that the previous lto=fat option is not affected
There was a problem hiding this comment.
I changed the comment, is it clearer now?
(I want to affect the current lto=fat option, as it currently does thin lto, which I think is not intended and a bug :))
There was a problem hiding this comment.
I can confirm that the current behavior is surprising, since I ran into this issue with autodiff and it took multiple months to find out that despite selecting lto=fat there is still a thin-lto module in the compilation pipeline.
dianqk
reviewed
Feb 17, 2025
|
r? codegen |
bors
added
S-waiting-on-review
Flakebi
force-pushed
the
linker-plugin-lto-fat
branch
from
7cf1a30 to
16274ea
Compare
|
Fix the test for #84395 by removing the |
dianqk
reviewed
Mar 27, 2025
There was a problem hiding this comment.
Fix the test for #84395 by removing the
-Zemit-thin-lto=noflag and instead setting-Clto=fat(diff to previous version). As mentioned in that issue, clang also requires setting-fltowhen using-lto-embed-bitcode=optimizedand otherwise runs into the same issue, so I guess that’s another improvement.
Hmm, I think -Zemit-thin-lto=no and -Clto=fat have different meanings, one is not submitting the Thin LTO buffer, while the other is performing Rust's Fat LTO, but -Clto=fat should imply -Zemit-thin-lto=no.
Btw, -Zemit-thin-lto=no should also fix your issue?
When rust provides LLVM bitcode files to lld and the bitcode contains function summaries as used for thin lto, lld defaults to using thin lto. This prevents some optimizations that are only applied for fat lto. We solve this by not creating function summaries when fat lto is enabled. The bitcode for the module is just directly written out. An alternative solution would be to set the `ThinLTO=0` module flag to signal lld to do fat lto. The code in clang that sets this flag is here: https://github.com/llvm/llvm-project/blob/560149b5e3c891c64899e9912e29467a69dc3a4c/clang/lib/CodeGen/BackendUtil.cpp#L1150 The code in LLVM that queries the flag and defaults to thin lto if not set is here: https://github.com/llvm/llvm-project/blob/e258bca9505f35e0a22cb213a305eea9b76d11ea/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4441-L4446
Flakebi
force-pushed
the
linker-plugin-lto-fat
branch
from
16274ea to
660d1e2
Compare
|
Keep check for
Should they be different? As far as I understand #84395,
It does, but I think that’s a workaround that should not be needed :) |
This will be useful when we don't want to perform Rust's own LTO. |
|
@bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 28, 2025
Fix linker-plugin-lto only doing thin lto When rust provides LLVM bitcode files to lld and the bitcode contains function summaries as used for thin lto, lld defaults to using thin lto. This prevents some optimizations that are only applied for fat lto. We solve this by not creating function summaries when fat lto is enabled. The bitcode for the module is just directly written out. An alternative solution would be to set the `ThinLTO=0` module flag to signal lld to do fat lto. The code in clang that sets this flag is here: https://github.com/llvm/llvm-project/blob/560149b5e3c891c64899e9912e29467a69dc3a4c/clang/lib/CodeGen/BackendUtil.cpp#L1150 The code in LLVM that queries the flag and defaults to thin lto if not set is here: https://github.com/llvm/llvm-project/blob/e258bca9505f35e0a22cb213a305eea9b76d11ea/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4441-L4446
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
VlaDexa
added a commit
to VlaDexa/rust
that referenced
this pull request
May 2, 2025
…ianqk Fix linker-plugin-lto only doing thin lto When rust provides LLVM bitcode files to lld and the bitcode contains function summaries as used for thin lto, lld defaults to using thin lto. This prevents some optimizations that are only applied for fat lto. We solve this by not creating function summaries when fat lto is enabled. The bitcode for the module is just directly written out. An alternative solution would be to set the `ThinLTO=0` module flag to signal lld to do fat lto. The code in clang that sets this flag is here: https://github.com/llvm/llvm-project/blob/560149b5e3c891c64899e9912e29467a69dc3a4c/clang/lib/CodeGen/BackendUtil.cpp#L1150 The code in LLVM that queries the flag and defaults to thin lto if not set is here: https://github.com/llvm/llvm-project/blob/e258bca9505f35e0a22cb213a305eea9b76d11ea/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4441-L4446
|
Since it failed CI, let's remove it from the merge queue. @bors r- |
Weird, that should've been kicked out automatically because full CI failed. |
|
I think bors is a bit behind. There were 3 merged PRs still in the queue. I ran a synchronization and then these two poped up. So anyway. :') |
|
☔ The latest upstream changes (presumably #143731) made this pull request unmergeable. Please resolve the merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When rust provides LLVM bitcode files to lld and the bitcode contains
function summaries as used for thin lto, lld defaults to using thin lto.
This prevents some optimizations that are only applied for fat lto.
We solve this by not creating function summaries when fat lto is
enabled. The bitcode for the module is just directly written out.
An alternative solution would be to set the
ThinLTO=0module flag tosignal lld to do fat lto.
The code in clang that sets this flag is here:
https://github.com/llvm/llvm-project/blob/560149b5e3c891c64899e9912e29467a69dc3a4c/clang/lib/CodeGen/BackendUtil.cpp#L1150
The code in LLVM that queries the flag and defaults to thin lto if not
set is here:
https://github.com/llvm/llvm-project/blob/e258bca9505f35e0a22cb213a305eea9b76d11ea/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4441-L4446