Skip to content

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

Merged
merged 1 commit into from
Jul 31, 2025

Conversation

Flakebi
Copy link
Contributor

@Flakebi Flakebi commented Feb 10, 2025

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

try-job: x86_64-gnu-debug
try-job: aarch64-gnu-debug

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@Flakebi Flakebi mentioned this pull request Feb 10, 2025
21 tasks
@workingjubilee
Copy link
Member

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

@jieyouxu
Copy link
Member

Unfortunately I have no clue either, so

r? compiler

@rustbot rustbot assigned fee1-dead and unassigned jieyouxu Feb 11, 2025
@Flakebi
Copy link
Contributor Author

Flakebi commented Feb 11, 2025

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);
}
Copy link
Member

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.

Copy link
Contributor Author

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

  1. lib uses lto=thin and main uses lto=thin
  2. lib uses lto=thin and main uses lto=fat
  3. lib uses lto=fat and main uses lto=thin
  4. 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.)

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned SparrowLii and unassigned fee1-dead Feb 16, 2025
Copy link
Member

@SparrowLii SparrowLii left a 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.
Copy link
Member

@SparrowLii SparrowLii Feb 17, 2025

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

Copy link
Contributor Author

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 :))

Copy link
Member

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.

@rustbot rustbot assigned Nadrieril and BoxyUwU and unassigned SparrowLii Feb 17, 2025
@Kobzol
Copy link
Member

Kobzol commented Feb 17, 2025

@dianqk Does this interact with your recent patch?

@dianqk
Copy link
Member

dianqk commented Feb 17, 2025

@dianqk Does this interact with your recent patch?

IMO, they aren't directly related.

@Nadrieril
Copy link
Member

r? codegen

@rustbot rustbot assigned saethlin and unassigned Nadrieril and BoxyUwU Feb 17, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2025
@ZuseZ4
Copy link
Member

ZuseZ4 commented Jul 30, 2025

@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

@Flakebi
Copy link
Contributor Author

Flakebi commented Jul 30, 2025

Aah yes, thanks, I meant a try run.
The two configurations for the cross-lang-lto-clang test are most interesting, that should be x86_64-gnu-debug and aarch64-gnu-debug, I added them to the PR description

@ZuseZ4
Copy link
Member

ZuseZ4 commented Jul 30, 2025

@bors try

rust-bors bot added a commit that referenced this pull request Jul 30, 2025
Fix linker-plugin-lto only doing thin lto

try-job: x86_64-gnu-debug
try-job: aarch64-gnu-debug
@rust-bors
Copy link

rust-bors bot commented Jul 30, 2025

⌛ Trying commit 6a1ed2f with merge cc3bd03

To cancel the try build, run the command @bors try cancel.

@rust-bors
Copy link

rust-bors bot commented Jul 30, 2025

☀️ Try build successful (CI)
Build commit: cc3bd03 (cc3bd03ea8b217fbd191dee23e1cf35df02e6025, parent: e5e79f8bd428d0b8d26e8240d718b134ef297459)

@dianqk
Copy link
Member

dianqk commented Jul 31, 2025

@ZuseZ4 FYI, you can use @bors try jobs=*-gnu-debug now. The new command is from #t-compiler > Testing try builds with new bors @ 💬.

@rust-bors
Copy link

rust-bors bot commented Jul 31, 2025

Unknown argument "now.". Run @bors2 help to see available commands.

@dianqk
Copy link
Member

dianqk commented Jul 31, 2025

@bors try jobs=*-gnu-debug

@rust-bors
Copy link

rust-bors bot commented Jul 31, 2025

⌛ Trying commit 6a1ed2f with merge d17dd8b

To cancel the try build, run the command @bors try cancel.

rust-bors bot added a commit that referenced this pull request Jul 31, 2025
Fix linker-plugin-lto only doing thin lto

try-job: *-gnu-debug
@dianqk
Copy link
Member

dianqk commented Jul 31, 2025

@bors try cancel

@rust-bors
Copy link

rust-bors bot commented Jul 31, 2025

Try build cancelled. Cancelled workflows:

Copy link
Member

@dianqk dianqk left a 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
@Flakebi Flakebi force-pushed the linker-plugin-lto-fat branch from 6a1ed2f to 7a127fb Compare July 31, 2025 08:39
@rustbot
Copy link
Collaborator

rustbot commented Jul 31, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@dianqk
Copy link
Member

dianqk commented Jul 31, 2025

@bors r+ thanks!

@bors
Copy link
Collaborator

bors commented Jul 31, 2025

📌 Commit 7a127fb has been approved by dianqk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2025
bors added a commit that referenced this pull request Jul 31, 2025
Rollup of 4 pull requests

Successful merges:

 - #136840 (Fix linker-plugin-lto only doing thin lto)
 - #144053 (Remove install Rust script from CI)
 - #144297 (Make `libtest::ERROR_EXIT_CODE` const public to not redefine it in rustdoc)
 - #144721 (`std_detect`: Linux 6.16 support for RISC-V)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8c09c1 into rust-lang:master Jul 31, 2025
10 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 31, 2025
rust-timer added a commit that referenced this pull request Jul 31, 2025
Rollup merge of #136840 - Flakebi:linker-plugin-lto-fat, r=dianqk

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

try-job: x86_64-gnu-debug
try-job: aarch64-gnu-debug
@bors
Copy link
Collaborator

bors commented Jul 31, 2025

⌛ Testing commit 7a127fb with merge 3fb1b53...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.