This is an archive of the discontinued LLVM Phabricator instance.

Reland "[gold] Add preliminary FatLTO support to the Gold plugin""
ClosedPublic

Authored by paulkirth on Jun 14 2023, 3:27 PM.

Details

Summary

This changes the definition if isSectionBitcode to only be valid for the
.llvm.lto section, since this API is only called from LTO, and the
.llvmbc section was not intended to be used for LTO. This allows the
gold plugin to keep its existing behavior without introducing any
significant changes.

Diff Detail

Event Timeline

paulkirth created this revision.Jun 14 2023, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2023, 3:27 PM
paulkirth requested review of this revision.Jun 14 2023, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2023, 3:27 PM
tejohnson added inline comments.Jun 15 2023, 6:57 AM
llvm/test/LTO/X86/Inputs/bcsection.s
1 ↗(On Diff #531548)

Are we now missing a test for .llvmbc? Also, shouldn't .llvmbc and .llvm.lto be handled differently? I.e. the former ignored and the latter accepted and LTO linked (for which it should have a test similar to the lld one). Or is there meant to be a follow on patch to fully enable FatLTO handling in gold?

paulkirth added inline comments.Jun 15 2023, 10:21 AM
llvm/lib/Object/ObjectFile.cpp
82

BTW I feel like we should document this somewhere. I guess "Release Notes"? but maybe also some place else. Are there any suggestions to where that should live?

llvm/test/LTO/X86/Inputs/bcsection.s
1 ↗(On Diff #531548)

ah, yeah, I guess we would also want a negative test for .llvmbc. I'll add that. I was thinking that since the test was trying to LTO compile w/ .llvmbc section that just updating this to .llvm.lto would be sufficient.

Or is there meant to be a follow on patch to fully enable FatLTO handling in gold?

In my testing with this patch gold handles things as I think would be expected. i.e. if given 2 fat-lto-objects files and passed -plugin LLVMGold.so, it will use the bitcode section in .llvm.lto to generate code. When linking from clang and passing -fno-lto the -plugin LLVM.Gold.so option isn't passed to the gold linker, and thus it uses the .text section.

I can try to whip up a similar test to the LLD one here and test that behavior more thoroughly.

Off the top of your head, are there other test cases I should consider? I'd like to be thorough, but most of the tests I've thought of are some kind of layering violation, and should be (I believe) covered by existing tests in other layers.

Rebase and add some tests for .llvmbc.

I still need to add a test similar to the LLD one

Add test case similar to the LLD one.

I still need to create some tests for the .llvmbc section.

How does gold recognize .llvm.lto section for LTO?

llvm/test/LTO/X86/Inputs/llvm.lto.section.s
2

This is a small extra file. You can use split-file to avoid the extra file.

.section .llvm.lto,"e"

llvm/test/tools/gold/X86/fatlto/fatlto.test
5

As in the lld patch, ; RUN: rm -rf %t && mkdir %t && cd %t

12

readonly,exclude

56

As in the lld patch, just use cmp to compare object files.

I see. In claim_file_hook, Expected<std::unique_ptr<InputFile>> ObjOrErr = InputFile::create(BufferRef); transitively calls findBitcodeInObject, which finds .llvm.lto.

We probably should support -?-plugin-opt=(no-)?fat-lto-objects to parallel the lld option.

Use similar tests to LLD

paulkirth marked an inline comment as done.

Fix some test issues.

  • Remove REQUIRES
  • Remove --fat-lto-objects from command lines
  • Fix ordering of objects in the archive case
paulkirth marked an inline comment as done.Jul 13 2023, 12:23 PM

We probably should support -?-plugin-opt=(no-)?fat-lto-objects to parallel the lld option.

So, I'm not sure on that. On one hand, I think it makes sense from a consistency standpoint. On the other hand, the llvm gold plugin is already added when the user wants LTO, and will give select the bitcode sections for them. We added the --(no-)fat-lto-objects flag in LLD in part because there isn't an equivalent LTO on/off switch. Using the plugin at all seems to serve the same purpose to me.

Also, I'm not really sure how/why that would be desirable. Is the idea so you can use regular (Thin)LTO objects + FatLTO objects, but only choose the .text of the fat object? In my experiments GCC would link the fat objects w/ LTO unless you passed -fno-lto, but maybe that isn't an important consideration.

Supporting those meaningfully may also be a bit challenging, since it appears to me that we have less ability to change the behavior here than we do in LLD. But maybe I'm overthinking that part, and just plumbing a flag through to InputFile::create(and whatever else) is enough?

llvm/test/LTO/X86/Inputs/llvm.lto.section.s
2

I think it makes sense to keep this as is, since it is more or less the same as the existing bcsection.s.

If you feel strongly, then I can add a follow up patch to do so, and handle the related input files in a similar way.

We probably should support -?-plugin-opt=(no-)?fat-lto-objects to parallel the lld option.

So, I'm not sure on that. On one hand, I think it makes sense from a consistency standpoint. On the other hand, the llvm gold plugin is already added when the user wants LTO, and will give select the bitcode sections for them. We added the --(no-)fat-lto-objects flag in LLD in part because there isn't an equivalent LTO on/off switch. Using the plugin at all seems to serve the same purpose to me.

Also, I'm not really sure how/why that would be desirable. Is the idea so you can use regular (Thin)LTO objects + FatLTO objects, but only choose the .text of the fat object? In my experiments GCC would link the fat objects w/ LTO unless you passed -fno-lto, but maybe that isn't an important consideration.

Supporting those meaningfully may also be a bit challenging, since it appears to me that we have less ability to change the behavior here than we do in LLD. But maybe I'm overthinking that part, and just plumbing a flag through to InputFile::create(and whatever else) is enough?

OK, if supporting avoiding LTO for a relocatable object file with .llvm.lto is too difficult, this change is fine.

paulkirth updated this revision to Diff 541220.Jul 17 2023, 1:40 PM

Update tests

  • check that error: is in the error message
  • avoid keeping the module summary in IR and generate it w/ opt --module-summary
MaskRay added inline comments.Jul 17 2023, 2:45 PM
llvm/test/tools/gold/X86/fatlto/fatlto.invalid.s
3

This should be gold?

11

delete blank line

paulkirth marked 2 inline comments as done.

Fix test so that it actually uses gold instead of lld.

MaskRay accepted this revision.Jul 18 2023, 11:28 AM
This revision is now accepted and ready to land.Jul 18 2023, 11:28 AM
This revision was landed with ongoing or failed builds.Jul 19 2023, 4:08 PM
This revision was automatically updated to reflect the committed changes.

The tools/gold/X86/fatlto/fatlto.test fails for us on everything but x86_64:

******************** TEST 'LLVM :: tools/gold/X86/fatlto/fatlto.test' FAILED ********************
Script:
--
: 'RUN: at line 3';   rm -rf /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp && split-file /builddir/build/BUILD/llvm-17.0.0.src/test/tools/gold/X86/fatlto/fatlto.test /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp
: 'RUN: at line 6';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-LTO.ll --filetype=obj -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o
: 'RUN: at line 7';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/opt --module-summary /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-LTO.ll -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.bc
: 'RUN: at line 8';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-objcopy --add-section=.llvm.lto=/builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.bc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o
: 'RUN: at line 9';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-objcopy --set-section-flags=.llvm.lto=readonly,exclude /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o
: 'RUN: at line 10';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-readobj -S /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o | /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/FileCheck --check-prefix=CHECK-A /builddir/build/BUILD/llvm-17.0.0.src/test/tools/gold/X86/fatlto/fatlto.test
: 'RUN: at line 14';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.ll --filetype=obj -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o
: 'RUN: at line 15';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/opt --module-summary /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.ll -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.bc
: 'RUN: at line 16';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-objcopy --add-section=.llvm.lto=/builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.bc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o
: 'RUN: at line 17';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-objcopy --set-section-flags=.llvm.lto=readonly,exclude /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o
: 'RUN: at line 18';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-readobj -S /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o | /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/FileCheck --check-prefix=CHECK-MAIN /builddir/build/BUILD/llvm-17.0.0.src/test/tools/gold/X86/fatlto/fatlto.test
: 'RUN: at line 23';   /usr/bin/ld.gold -plugin /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/lib64/LLVMgold.so -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o
: 'RUN: at line 24';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-readobj -S /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO | /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/FileCheck --check-prefix=CHECK-LTO-TARGET /builddir/build/BUILD/llvm-17.0.0.src/test/tools/gold/X86/fatlto/fatlto.test
: 'RUN: at line 27';   /usr/bin/ld.gold -plugin /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/lib64/LLVMgold.so -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO.start_lib --start-lib /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o --end-lib
: 'RUN: at line 28';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-readobj -S /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO.start_lib | /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/FileCheck --check-prefix=CHECK-LTO-TARGET /builddir/build/BUILD/llvm-17.0.0.src/test/tools/gold/X86/fatlto/fatlto.test
: 'RUN: at line 34';   /usr/bin/ld.gold -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatNoLTO /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-fatLTO.o
: 'RUN: at line 35';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-readobj -S /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatNoLTO | /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/FileCheck --check-prefix=CHECK-NON-LTO-TARGET /builddir/build/BUILD/llvm-17.0.0.src/test/tools/gold/X86/fatlto/fatlto.test
: 'RUN: at line 42';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/opt --module-summary /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-LTO.ll -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-LTO.bc
: 'RUN: at line 43';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/opt --module-summary /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.ll -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.bc
: 'RUN: at line 44';   /usr/bin/ld.gold -plugin /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/lib64/LLVMgold.so -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-LTO /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-LTO.bc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.bc
: 'RUN: at line 45';   cmp /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-LTO
: 'RUN: at line 50';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-LTO.ll --filetype=obj -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a.o
: 'RUN: at line 51';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.ll --filetype=obj -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main.o
: 'RUN: at line 53';   /usr/bin/ld.gold -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-noLTO /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a.o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main.o
: 'RUN: at line 54';   cmp /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatNoLTO /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-noLTO
: 'RUN: at line 57';   /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/bin/llvm-ar rcs /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a.a /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a-fatLTO.o
: 'RUN: at line 58';   /usr/bin/ld.gold -plugin /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/lib64/LLVMgold.so -o /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO.archive /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/main-LTO.bc /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/a.a
: 'RUN: at line 59';   cmp /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-fatLTO.archive /builddir/build/BUILD/llvm-17.0.0.src/redhat-linux-build/test/tools/gold/X86/fatlto/Output/fatlto.test.tmp/foo-LTO
--
Exit Code: 1

Command Output (stderr):
--
/usr/bin/ld.gold: error: /tmp/lit-tmp-_vrw63qp/lto-llvm-fd907e.o: incompatible target

The documentation in https://llvm.org/docs/BitCodeFormat.html#native-object-file-wrapper-format says:

Bitcode files for LLVM IR may also be wrapped in a native object file (i.e. ELF, COFF, Mach-O). The bitcode must be stored in a section of the object file named __LLVM,__bitcode for MachO and .llvmbc for the other object formats. This wrapper format is useful for accommodating LTO in compilation pipelines where intermediate objects must be native object files which contain metadata in other sections.

Not all tools support this format. For example, lld and the gold plugin will ignore these sections when linking object files.

We noticed this as this change has broken our downstream LTO bitcode wrapper as it was using .llvmbc as a section name. I'm sure we can adapt, but would be good to clarify this section of the reference manual.

@tbaeder, could you provide a bit more information about the configuration you are using? The test is in an x86 directory, which should mean it only runs on x86 enabled targets, right? If this is blocking you, I'm OK w/ a revert, but do want to understand what the issue is.

@peter.smith thank you for pointing that out. I will be sure to update the documentation.

nikic added a comment.Jul 20 2023, 1:27 PM

@tbaeder, could you provide a bit more information about the configuration you are using? The test is in an x86 directory, which should mean it only runs on x86 enabled targets, right? If this is blocking you, I'm OK w/ a revert, but do want to understand what the issue is.

The test requires not only the x86 target in LLVM, but also that ld.gold supports it, which will generally only be the case on x86 hosts. So I think the test needs something like REQUIRES: x86_64-linux (not sure if that's the right spelling...).

@peter.smith thank you for pointing that out. I will be sure to update the documentation.

FYI this change also broke Rust LTO for the same reason (.llvmbc used for its own fat LTO equivalent). I expect that can be fixed by switching it to use .llvm.lto, but not sure if that will cause any other issues.

@tbaeder, could you provide a bit more information about the configuration you are using? The test is in an x86 directory, which should mean it only runs on x86 enabled targets, right? If this is blocking you, I'm OK w/ a revert, but do want to understand what the issue is.

The test requires not only the x86 target in LLVM, but also that ld.gold supports it, which will generally only be the case on x86 hosts. So I think the test needs something like REQUIRES: x86_64-linux (not sure if that's the right spelling...).

I see. Thank you for pointing that out. I think I may just revert this for now while we hash out the potential issues downstream.

@peter.smith thank you for pointing that out. I will be sure to update the documentation.

FYI this change also broke Rust LTO for the same reason (.llvmbc used for its own fat LTO equivalent). I expect that can be fixed by switching it to use .llvm.lto, but not sure if that will cause any other issues.

Hmm, I wasn't aware that rustc used a .llvmbc section. Thanks for pointing this out .The initial discussion on discourse pointed out that .llvmbc was only added to support the apple use case (discussion on this point starts roughly here https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977/3), and non-lto usage should go away. We moved to a .llvm.lto section to make the distinction clear that this was LTO compatible bitcode and not just whatever the frontend emitted.

In your opinion, do we need a soft transition here? If so, any advice on what that should look like would be helpful.

paulkirth reopened this revision.Jul 20 2023, 1:56 PM
This revision is now accepted and ready to land.Jul 20 2023, 1:56 PM
paulkirth updated this revision to Diff 542686.Jul 20 2023, 2:47 PM
paulkirth retitled this revision from [gold] Add preliminary FatLTO support to the Gold plugin to Reland "[gold] Add preliminary FatLTO support to the Gold plugin"".
paulkirth edited the summary of this revision. (Show Details)

Add REQUIRES lines to gold tests and update summary

paulkirth updated this revision to Diff 543607.Jul 24 2023, 9:50 AM

Update docs on bitcode sections in object files to describe the new .llvm.lto section and how it is intended to differ from .llvmbc.

nikic added inline comments.Jul 24 2023, 1:59 PM
llvm/docs/BitCodeFormat.rst
488

Do we? This documentation seems to be the only reference to __lto I can find.

isSectionBitcode() for MachoOObjectFile only checks for __LLVM,__bitcode.

@peter.smith @nikic @MaskRay @tejohnson, given that Rust and others seem to use the .llvmbc, I see a few ways we can move forward here.

  1. have a hard transition (e..g., this patch), and make downstream consumers adapt now.
  2. have a soft transition, and mark .llvmbc as deprecated, then remove in the following release (so about 6 months from now).
  3. forgo trying to separate the old uses of .llvmbc from new FatLTO uses and just reuse the .llvmbc section name.
  4. ... something completely different?

I'm not really sure what the best way to go about this would be. I'd lean towards a soft transition (2), since that seems more forgiving and less hostile towards downstream consumers, but I think there are merits for any of the approaches. There are probably other options I haven't considered either, so thoughts and suggestions would be quite welcome.

paulkirth added inline comments.Jul 24 2023, 2:09 PM
llvm/docs/BitCodeFormat.rst
488

You are correct. I haven't implemented MachO support yet. I was over zealous trying to update this, and that is currently incorrect.

paulkirth added inline comments.Jul 24 2023, 2:18 PM
llvm/docs/BitCodeFormat.rst
488

As an FYI to reviewers, I will wait on rewording this again until we reach a consensus on how to proceed w/ the transition.

MaskRay added a subscriber: alexcrichton.EditedJul 24 2023, 2:20 PM

@peter.smith @nikic @MaskRay @tejohnson, given that Rust and others seem to use the .llvmbc, I see a few ways we can move forward here.

  1. have a hard transition (e..g., this patch), and make downstream consumers adapt now.
  2. have a soft transition, and mark .llvmbc as deprecated, then remove in the following release (so about 6 months from now).
  3. forgo trying to separate the old uses of .llvmbc from new FatLTO uses and just reuse the .llvmbc section name.
  4. ... something completely different?

I'm not really sure what the best way to go about this would be. I'd lean towards a soft transition (2), since that seems more forgiving and less hostile towards downstream consumers, but I think there are merits for any of the approaches. There are probably other options I haven't considered either, so thoughts and suggestions would be quite welcome.

If -ffat-lto-objects doesn't need to go to the LLVM 17.0.0 release, we can do a soft transition:

  • Keep the LLVM and Clang parts in the release/17.x branch
  • Revert the Clang Driver -ffat-lto-objects change in release/17.x so that Clang 17 still doesn't support -ffat-lto-objects.
  • Give some time for @peter.smith and https://github.com/rust-lang/rust/ to migrate to .llvm.lto . I am not familiar with rust-lang/rust, but it seems that @alexcrichton is involved in some changes. Note, giving some time for the downstream should be considered a courtesy, not a norm.

I am curious how downstream users perceive a breakage? Is any of IRObjectFile::findBitcodeInObject, IRObjectFile::findBitcodeInMemBuffe, lto_module_is_object_file_for_target used?

If a C API for finding .llvm.lto is needed, we can add one and even back port it into release/17.x.

nikic added a comment.EditedJul 24 2023, 2:35 PM

From a Rust perspective, I'm reasonably confident that we can deal with the patch as-is. We currently use findBitcodeInMemBuffer(), but we should easily be able to replace that with our own implementation. (I suspect we'll continue using the llvmbc section for the time being, specifically so only our own LTO implementation picks it up, which would match previous behavior where llvmbc was not used by linker plugin LTO or lld.)

The timing here is unfortunate, but I would prefer to land both the lld and gold changes before LLVM 17 branches, so we're not left with half-implemented fat object support.

If -ffat-lto-objects doesn't need to go to the LLVM 17.0.0 release, we can do a soft transition:

Given that Rust pins the LLVM revision to a stable release I think it may be good to get it into Clang 17. Doubly so, since it seems to me that they already have a version of FatLTO, and would benefit from using the FatLTO pipeline.

  • Keep the LLVM and Clang parts in the release/17.x branch
  • Revert the Clang Driver -ffat-lto-objects change in release/17.x so that Clang 17 still doesn't support -ffat-lto-objects.
  • Give some time for @peter.smith and https://github.com/rust-lang/rust/ to migrate to .llvm.lto . I am not familiar with rust-lang/rust, but it seems that @alexcrichton is involved in some changes. Note, giving some time for the downstream should be considered a courtesy, not a norm.

Thanks for the suggestion. If we're not trying to make the next release, this sounds reasonable to me, though I am curious to hear what others think, too.

If -ffat-lto-objects doesn't need to go to the LLVM 17.0.0 release, we can do a soft transition:

Given that Rust pins the LLVM revision to a stable release I think it may be good to get it into Clang 17. Doubly so, since it seems to me that they already have a version of FatLTO, and would benefit from using the FatLTO pipeline.

My suggestion is based on my (possibly wrong) understanding that rust-lang/rust only needs the LLVM side change, which is in release/17.x, but not Clang or lld changes :)
That said, if people feel that it's good to ensure clang and lld changes are in release/17.x, I'm totally fine as well.

(I'll go on a trip from Thursday onwards and won't be able to respond in time. Apologies if anyone needs me to do anything.)

nikic added a comment.Jul 24 2023, 3:20 PM

If -ffat-lto-objects doesn't need to go to the LLVM 17.0.0 release, we can do a soft transition:

Given that Rust pins the LLVM revision to a stable release I think it may be good to get it into Clang 17. Doubly so, since it seems to me that they already have a version of FatLTO, and would benefit from using the FatLTO pipeline.

I don't think we are going to use the FatLTO pipeline as currently implemented in Rust, due to its bad compile-time characteristics. We need a FatLTO implementation that does not require compiling the module twice.

If -ffat-lto-objects doesn't need to go to the LLVM 17.0.0 release, we can do a soft transition:

Given that Rust pins the LLVM revision to a stable release I think it may be good to get it into Clang 17. Doubly so, since it seems to me that they already have a version of FatLTO, and would benefit from using the FatLTO pipeline.

I don't think we are going to use the FatLTO pipeline as currently implemented in Rust, due to its bad compile-time characteristics. We need a FatLTO implementation that does not require compiling the module twice.

That's unfortunate, and is something I hope we can address moving forward. I vaguely recall your suggestion that after D148010 we could probably do better w.r.t. optimizing the pipeline, is that still correct? Also, out of curiosity, how does rustc avoid the edge cases brought up in the initial patches, like missing certain transforms for ThinLTO + SamplePGO?

nikic added a comment.Jul 25 2023, 1:50 AM

I don't think we are going to use the FatLTO pipeline as currently implemented in Rust, due to its bad compile-time characteristics. We need a FatLTO implementation that does not require compiling the module twice.

That's unfortunate, and is something I hope we can address moving forward. I vaguely recall your suggestion that after D148010 we could probably do better w.r.t. optimizing the pipeline, is that still correct?

It would at least make things a good bit simpler, by removing thin vs full LTO differences from the equation.

Also, out of curiosity, how does rustc avoid the edge cases brought up in the initial patches, like missing certain transforms for ThinLTO + SamplePGO?

I'm not sure Rust even supports SamplePGO. Maybe there is an unstable feature for it.

But the truth is that Rust doesn't have a dedicated fat LTO pipeline at all, it just runs the usual pipeline and then both embeds the final bitcode and emits object code. This is of course bad (for the same reasons the current full LTO pipeline is bad), and replacing it with a proper fat LTO pipeline would be great.

@peter.smith @nikic @MaskRay @tejohnson, given that Rust and others seem to use the .llvmbc, I see a few ways we can move forward here.

  1. have a hard transition (e..g., this patch), and make downstream consumers adapt now.
  2. have a soft transition, and mark .llvmbc as deprecated, then remove in the following release (so about 6 months from now).
  3. forgo trying to separate the old uses of .llvmbc from new FatLTO uses and just reuse the .llvmbc section name.
  4. ... something completely different?

I'm not really sure what the best way to go about this would be. I'd lean towards a soft transition (2), since that seems more forgiving and less hostile towards downstream consumers, but I think there are merits for any of the approaches. There are probably other options I haven't considered either, so thoughts and suggestions would be quite welcome.

I think we can adapt fairly quickly as it is an embedded toolchain and we don't support mixing LTO objects from previous versions, I think we'll be able to cope with an instant change. May be worth an RFC/FYI on Discourse as some downstream projects merge with upstream less frequently than we do.

I don't think we are going to use the FatLTO pipeline as currently implemented in Rust, due to its bad compile-time characteristics. We need a FatLTO implementation that does not require compiling the module twice.

That's unfortunate, and is something I hope we can address moving forward. I vaguely recall your suggestion that after D148010 we could probably do better w.r.t. optimizing the pipeline, is that still correct?

It would at least make things a good bit simpler, by removing thin vs full LTO differences from the equation.

Right, I had forgotten that patch is only about smoothing out the differences between the two pipelines. I suppose that if there are not major differences, then we could potentially use the UnifiedLTO pipeline and then (I think) we'd be able to just do the module simplification (+ anything needed for instrumentation) for generating the bitcode , and then run module optimization after to generate the object code. At least thats the rough idea in my head ATM. There are no doubt some unfortunate edge-cases, but I'm hoping we sort those out in the not too distant future.

Also, out of curiosity, how does rustc avoid the edge cases brought up in the initial patches, like missing certain transforms for ThinLTO + SamplePGO?

I'm not sure Rust even supports SamplePGO. Maybe there is an unstable feature for it.

Indeed, I see a -Z profile-sample--use=val flag in the help. Seems to have landed in https://github.com/rust-lang/rust/commit/a17193dbb931ea0c8b66d82f640385bce8b4929a, a couple years ago.

But the truth is that Rust doesn't have a dedicated fat LTO pipeline at all, it just runs the usual pipeline and then both embeds the final bitcode and emits object code. This is of course bad (for the same reasons the current full LTO pipeline is bad), and replacing it with a proper fat LTO pipeline would be great.

Agreed. I'd be very happy if this simplifies things for Rust. It's an important usecase for Fuchsia, so the sooner we can get support into rustc the better.

@peter.smith @nikic @MaskRay @tejohnson, given that Rust and others seem to use the .llvmbc, I see a few ways we can move forward here.

  1. have a hard transition (e..g., this patch), and make downstream consumers adapt now.
  2. have a soft transition, and mark .llvmbc as deprecated, then remove in the following release (so about 6 months from now).
  3. forgo trying to separate the old uses of .llvmbc from new FatLTO uses and just reuse the .llvmbc section name.
  4. ... something completely different?

I'm not really sure what the best way to go about this would be. I'd lean towards a soft transition (2), since that seems more forgiving and less hostile towards downstream consumers, but I think there are merits for any of the approaches. There are probably other options I haven't considered either, so thoughts and suggestions would be quite welcome.

I think we can adapt fairly quickly as it is an embedded toolchain and we don't support mixing LTO objects from previous versions, I think we'll be able to cope with an instant change. May be worth an RFC/FYI on Discourse as some downstream projects merge with upstream less frequently than we do.

OK, well, it seems like maybe we should just land these now, since it sounds like both downstream consumers will be able to adapt w/o much trouble. A post on Discourse is a good idea. I should be able to do that later today.

So, if there are no concerns, are we all OK w/ landing this again modulo documentation changes?

paulkirth updated this revision to Diff 544004.Jul 25 2023, 9:09 AM

Revise documentation of bitcode sections

I'm happy for this to be relanded modulo documentation updates.

paulkirth added inline comments.Jul 25 2023, 11:34 AM
llvm/docs/BitCodeFormat.rst
473–480

@peter.smith @nikic I've gone ahead and revised this, since it seems we're OK w/ relanding now. Are we happy w/ the new text?

paulkirth marked an inline comment as done.Aug 7 2023, 11:40 AM

ping. Are we OK w/ landing this now, or are there other changes we want to adopt?

Ping. @nikic @peter.smith do we still need changes to the verbiage in the documentation? I've also posted an FYI in discourse about 2 weeks ago. https://discourse.llvm.org/t/planned-changes-to-object-file-support-for-embeded-bitcode-sections-in-elf-files/72546

Are there any remaining concerns? or is this OK to reland?

nikic accepted this revision.Aug 17 2023, 12:48 AM

LGTM, sorry for the delay.

However, please file a revert for clang fat LTO support for the 17.x release branch, as this patch should not be backported, so we need to revert the other instead.

I've filed https://github.com/llvm/llvm-project/issues/64508 to address the backporting. I think that's following the correct guidelines, but I'm a bit unclear on the process between what is in our docs and what has been posted on discourse.

This revision was landed with ongoing or failed builds.Aug 18 2023, 3:57 PM
This revision was automatically updated to reflect the committed changes.