Conversation
rustbot
added
A-LLVM
|
r? @cuviper rustbot has assigned @cuviper. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
||
| ModulePassManager MPM; | ||
| bool NeedThinLTOBufferPasses = EmitThinLTO; | ||
| bool NeedThinLTOBufferPasses = true; |
There was a problem hiding this comment.
This used to be true when fat LTO is not used, so I ensured all fat LTO phases do NeedThinLTOBufferPasses = false below.
| MPM.addPass(BitcodeWriterPass(ThinLTODataOS)); | ||
| } | ||
| MPM.addPass(ThinLTOBitcodeWriterPass( | ||
| ThinLTODataOS, EmitThinLTOSummary ? &ThinLinkDataOS : nullptr)); |
There was a problem hiding this comment.
PreLinkNoLTO is never used when fat LTO is enabled.
| if (EmitThinLTO) { | ||
| // thin lto summaries prevent fat lto, so do not emit them if fat | ||
| // lto is requested. See PR #136840 for background information. | ||
| if (OptStage != LLVMRustOptStage::PreLinkFatLTO) { |
There was a problem hiding this comment.
This doesn't need to check LLVMRustOptStage::FatLTO as ThinLTOBufferRef is NULL in that case.
There was a problem hiding this comment.
Then, how is this block "For ... LTO" at all?
(per the existing comment, assuming unqualified means full)
There was a problem hiding this comment.
LTO runs this function twice. Once on each individual module (PreLinkFatLTO or PreLinkThinLTO). In this case ThinBufferRef is a valid pointer to write the bitcode to after optimizing. And a second time after doing module linking (FatLTO) or cross-module function importing (ThinLTO). In this case ThinBufferRef is NULL as we will emit object file(s) rather thaj bitcode.
There was a problem hiding this comment.
I see, then perhaps the "For" comment can mention that this is only expected in the prelink phase.
bjorn3
force-pushed
the
remove_z_emit_thin_lto
branch
from
df6f555 to
3a1c7d4
Compare
This comment has been minimized.
This comment has been minimized.
As far as I can tell it was introduced to allow fat LTO with -Clinker-plugin-lto. Later a change was made to automatically disable ThinLTO summary generation when -Clinker-plugin-lto -Clto=fat is used, so we can safely remove it.
bjorn3
force-pushed
the
remove_z_emit_thin_lto
branch
from
3a1c7d4 to
e93ee27
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
3 participants
As far as I can tell it was introduced in #98162 to allow fat LTO with
-Clinker-plugin-lto. In #136840 a change was made to automatically disable ThinLTO summary generation when-Clinker-plugin-lto -Clto=fatis used, so we can safely remove it.Fixes #152490