Fix linker-plugin-lto only doing thin lto #136840

Flakebi · 2025-02-10T23:45:01Z

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.

Set the ThinLTO=0 module flag to signal lld to do fat lto.

The analogous 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

r? @workingjubilee, as you’ve been reviewing most other amdgpu patches, not sure if there should be other reviewers for lto.

rustbot · 2025-02-10T23:45:04Z

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 · 2025-02-10T23:45:10Z

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 · 2025-02-10T23:45:12Z

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

workingjubilee · 2025-02-11T00:09:30Z

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 · 2025-02-11T00:35:31Z

Unfortunately I have no clue either, so

r? compiler

Flakebi · 2025-02-11T12:59:05Z

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 · 2025-02-11T17:39:59Z

compiler/rustc_codegen_llvm/src/context.rs

+    // 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);
+    }


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.

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

compiler/rustc_codegen_ssa/src/back/link.rs

fee1-dead · 2025-02-16T10:27:53Z

r? compiler

SparrowLii

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

SparrowLii · 2025-02-17T01:21:21Z

compiler/rustc_codegen_llvm/src/context.rs

@@ -290,6 +290,11 @@ pub(crate) unsafe fn create_module<'ll>(
        );
    }

+    // Disable ThinLTO if fat lto is requested. Otherwise lld defaults to thin lto.


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

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

Kobzol · 2025-02-17T09:33:30Z

@DianQK Does this interact with your recent patch?

DianQK · 2025-02-17T10:06:31Z

@DianQK Does this interact with your recent patch?

IMO, they aren't directly related.

compiler/rustc_codegen_ssa/src/back/linker.rs

Nadrieril · 2025-02-17T21:59:33Z

r? codegen

Flakebi · 2025-02-18T13:19:18Z

I think it would be much more acceptable if this PR could limit the change to amdhsa conditions.

AFAIU, fat LTO + linker-plugin-lto is currently broken for all targets, as lld just does thin lto, even if the user requested fat lto. Setting the ThinLTO=0 metadata fixes that and gets lld to do actual fat lto.

It’s just that x86 and other targets run fine with only thin lto. GPU targets (e.g. amdgpu and nvptx) currently require lto to function correctly. This may change when hardware vendors focus more on linking, but I think the performance dip makes it rather unattractive today. So, if fat LTO is not working correctly, it becomes apparent quickly on GPU targets, not so on x86.

I can move the amdhsa part to a separate PR if that’s preferred. The reason I included it here, is that I only managed to write a test for amdgpu so far and I wanted to include a test.

rustbot · 2025-02-19T23:57:55Z

The run-make-support library was changed

cc @jieyouxu

Flakebi · 2025-02-19T23:58:24Z

I found a way to reproduce the same on x86 by creating an object file that does not contain function summaries. I added a test and removed all the amdhsa-specific code from this PR. Diff to last push

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. Set the `ThinLTO=0` module flag to signal lld to do fat lto. The analogous 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 · 2025-02-20T09:28:26Z

Try to fix the test by forcing rust-lld (diff)

rustbot assigned jieyouxu Feb 10, 2025

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

Flakebi mentioned this pull request Feb 10, 2025

Tracking Issue for amdgcn target #135024

Open

16 tasks

rustbot assigned fee1-dead and unassigned jieyouxu Feb 11, 2025

bjorn3 reviewed Feb 11, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot assigned SparrowLii and unassigned fee1-dead Feb 16, 2025

SparrowLii reviewed Feb 17, 2025

View reviewed changes

rustbot assigned Nadrieril and BoxyUwU and unassigned SparrowLii Feb 17, 2025

DianQK reviewed Feb 17, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/back/linker.rs Outdated Show resolved Hide resolved

rustbot assigned saethlin and unassigned Nadrieril and BoxyUwU Feb 17, 2025

Flakebi force-pushed the linker-plugin-lto-fat branch from 0d63f96 to 2d33f69 Compare February 19, 2025 23:57

This comment has been minimized.

Sign in to view

Flakebi force-pushed the linker-plugin-lto-fat branch from 2d33f69 to d421833 Compare February 20, 2025 09:27

Flakebi force-pushed the linker-plugin-lto-fat branch from d421833 to b692fa0 Compare February 20, 2025 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix linker-plugin-lto only doing thin lto #136840

Fix linker-plugin-lto only doing thin lto #136840

Flakebi commented Feb 10, 2025 •

edited

Loading

rustbot commented Feb 10, 2025

rustbot commented Feb 10, 2025

rustbot commented Feb 10, 2025

workingjubilee commented Feb 11, 2025

jieyouxu commented Feb 11, 2025

Flakebi commented Feb 11, 2025

bjorn3 Feb 11, 2025

Flakebi Feb 12, 2025

This comment has been minimized.

fee1-dead commented Feb 16, 2025

SparrowLii left a comment

SparrowLii Feb 17, 2025 •

edited

Loading

Flakebi Feb 20, 2025

Kobzol commented Feb 17, 2025

DianQK commented Feb 17, 2025

Nadrieril commented Feb 17, 2025

Flakebi commented Feb 18, 2025

rustbot commented Feb 19, 2025

Flakebi commented Feb 19, 2025

This comment has been minimized.

Flakebi commented Feb 20, 2025

Fix linker-plugin-lto only doing thin lto #136840

Are you sure you want to change the base?

Fix linker-plugin-lto only doing thin lto #136840

Conversation

Flakebi commented Feb 10, 2025 • edited Loading

rustbot commented Feb 10, 2025

rustbot commented Feb 10, 2025

rustbot commented Feb 10, 2025

workingjubilee commented Feb 11, 2025

jieyouxu commented Feb 11, 2025

Flakebi commented Feb 11, 2025

bjorn3 Feb 11, 2025

Choose a reason for hiding this comment

Flakebi Feb 12, 2025

Choose a reason for hiding this comment

This comment has been minimized.

fee1-dead commented Feb 16, 2025

SparrowLii left a comment

Choose a reason for hiding this comment

SparrowLii Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Flakebi Feb 20, 2025

Choose a reason for hiding this comment

Kobzol commented Feb 17, 2025

DianQK commented Feb 17, 2025

Nadrieril commented Feb 17, 2025

Flakebi commented Feb 18, 2025

rustbot commented Feb 19, 2025

Flakebi commented Feb 19, 2025

This comment has been minimized.

Flakebi commented Feb 20, 2025

Flakebi commented Feb 10, 2025 •

edited

Loading

SparrowLii Feb 17, 2025 •

edited

Loading