-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Fix linker-plugin-lto only doing thin lto #136840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Could not assign reviewer from: |
This PR modifies cc @jieyouxu |
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)); |
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
This comment has been minimized.
This comment has been minimized.
r? compiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know little about lto, either. I think it would be much more acceptable if this PR could limit the change to amdhsa
conditions.
r? compiler
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
r? codegen |
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. |
660d1e2
to
b5b0282
Compare
I finally found some time to look at this again. Debugging the aarch64 test failure from an x86 system was not the most straightforward thing 😅 I rebased to fix conflicts, new changes are in the second commit b5b0282. In CI, the cross-lang-lto-clang passed on x86_64 but not on aarch64. I don’t have a good way to debug this, but with some local changes (my clang to cross-compiling to aarch64 uses different target features, so nothing gets inlined by default; and I wasn’t able to link a rust std for aarch64), I think I found the culprit. |
This comment has been minimized.
This comment has been minimized.
PR CI passed (though it doesn’t run the interesting tests). If a person of power can start a bors check run, that would be great. @rustbot ready |
@Flakebi I haven't hearedof check runs, but I can start a try run for you if you want. But you would need to specify first for which target you want to run it, see here: https://rustc-dev-guide.rust-lang.org/tests/ci.html?highlight=bors%20try#try-builds |
Aah yes, thanks, I meant a try run. |
@bors try |
Fix linker-plugin-lto only doing thin lto try-job: x86_64-gnu-debug try-job: aarch64-gnu-debug
@ZuseZ4 FYI, you can use @bors try jobs=*-gnu-debug now. The new command is from #t-compiler > Testing try builds with new bors @ 💬. |
Unknown argument "now.". Run |
@bors try jobs=*-gnu-debug |
Fix linker-plugin-lto only doing thin lto try-job: *-gnu-debug
@bors try cancel |
Try build cancelled. Cancelled workflows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes look good to me. Please squash your commits.
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
6a1ed2f
to
7a127fb
Compare
Some changes occurred in compiler/rustc_codegen_ssa |
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 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
try-job: x86_64-gnu-debug
try-job: aarch64-gnu-debug