This is an archive of the discontinued LLVM Phabricator instance.

Reland "[lld] Preliminary fat-lto-object support"
ClosedPublic

Authored by paulkirth on Mar 23 2023, 5:58 PM.

Details

Summary

This patch adds support to lld for --fat-lto-objects. We add a new
--fat-lto-objects option to LLD, and slightly change how it chooses input
files in the driver when the option is set.

Fat LTO objects contain both LTO compatible IR, as well as generated object
code. This allows users to defer the choice of whether to use LTO or not to
link-time. This is a feature available in GCC for some time, and makes the
existing -ffat-lto-objects option functional in the same way as GCC's.

If the --fat-lto-objects option is passed to LLD and the input files are fat
object files, then the linker will chose the LTO compatible bitcode sections
embedded within the fat object and link them together using LTO. Otherwise,
standard object file linking is done using the assembly section in the object
files.

The previous version of this patch had a missing REQUIRES: x86 line in
fatlto.invalid.s. Additionally, it was reported that this patch caused
a test failure in export-dynamic-symbols.s, however,
29112a994694baee070a2021e00f772f1913d214 disabled the
export-dynamic-symbols.s test on Windows due to a quotation difference
between platforms, unrelated to this patch.

Original RFC: https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977

Diff Detail

Event Timeline

paulkirth created this revision.Mar 23 2023, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 5:58 PM
paulkirth requested review of this revision.Mar 23 2023, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 5:58 PM
paulkirth updated this revision to Diff 507940.Mar 23 2023, 7:08 PM

remove unused yaml input files for testing

paulkirth updated this revision to Diff 507945.Mar 23 2023, 7:15 PM

Fix typo in test code.

paulkirth updated this revision to Diff 508775.Mar 27 2023, 1:16 PM
paulkirth retitled this revision from [lld] Preliminary fat-lot-object support to [lld] Preliminary fat-lto-object support.

Fix typo in title

Add release note

aeubanks added inline comments.
lld/docs/ReleaseNotes.rst
29 ↗(On Diff #517707)

typo

paulkirth updated this revision to Diff 517731.Apr 27 2023, 3:24 PM
paulkirth marked an inline comment as done.

Fix typo

MaskRay requested changes to this revision.Jun 17 2023, 5:24 PM

Thanks. This looks mostly good but archives need to be implemented.

If the -fat-lto-objects flag is passed to LLD and the input files are fat

If the --fat-lto-objects option is ...

lld/ELF/Options.td
618

BB

Don't allow single-dash long options for new options.

defm fat_lto_objects

lld/docs/ReleaseNotes.rst
34 ↗(On Diff #532273)

Double backsticks. Prefer option over flag especially for linkers.

lld/test/ELF/fatlto/Inputs/a-LTO.ll
2

Extra files can be avoided with split-file

lld/test/ELF/fatlto/fatlto.test
5

Add period for complete sentences throughout.

Delete ;; Clean up and initialize test dir which is not too useful.

6

; RUN: rm -rf %t && mkdir %t && cd %t

Do this in one line and you can avoid %t/ below...

MaskRay added inline comments.Jun 17 2023, 5:24 PM
lld/ELF/Driver.cpp
313

Add a test that the .llvm.lto has invalid LLVM IR.

You can use .section .llvm.lto,"e" in assembly to construct such a test case.

2745

This code can be avoided if .llvm.lto has the SHF_EXCLUDE flag.

lld/test/ELF/fatlto/Inputs/a-LTO.ll
6

Add a _start function to prevent a linker warning. Maybe just change a to _start

lld/test/ELF/fatlto/fatlto.test
25

--fat-lto-objects

30

We should test --start-lib (inLib) and fat lto objects in an archive.

54

This obj2yaml comparison is not common. Just use cmp foo-fatNoLTO foo-noLTO

This revision now requires changes to proceed.Jun 17 2023, 5:24 PM
paulkirth updated this revision to Diff 535529.Jun 28 2023, 2:17 PM
paulkirth marked 2 inline comments as done.

Fix some comments

  • update option to use --
  • remove check against .llvm.lto string
  • update some test checks w/ new flag & mark .llvm.lto section exclude w/ objcopy

There are still several comments to address regarding tests.

I wonder whether --fat-lto-objects should be the default. Since clang -ffat-lto-objects is an opt-in feature, we don't need to worry about default --fat-lto-objects breaking existing users.
AFAIK LLVMgold defaults to --fat-lto-objects (currently there is no -?-plugin-opt= option to toggle this feature, but it'd be nice to have one.

MaskRay added inline comments.Jun 28 2023, 2:22 PM
lld/test/ELF/fatlto/Inputs/main-LTO.ll
31

This can be omitted. If not, having the 15.0.0 identifier for a 17.0.0 or 18.0.0 feature lures readers to think that this may be available for 15.0.0

MaskRay added inline comments.Jun 28 2023, 2:35 PM
lld/test/ELF/fatlto/Inputs/main-LTO.ll
31

I think we can use very simple IR to improve signal-to-noise ratio. ls -lS should list some very simple IR, e.g. inline-asm.ll

paulkirth marked 7 inline comments as done.

Rebase and address comments.

  • Add archive support
  • Update tests to use split-file
  • Add test for invalid IR
  • Use normal comparison

This still needs to get a test for inLib and for archives.

Also, in Driver.cpp, the FatLTO support seems a bit repetitive, so I plan to
simplify w/ a helper function, but wanted to be sure I've handled the new cases
for archives correctly first before refactoring.

paulkirth updated this revision to Diff 539799.Jul 12 2023, 4:59 PM
paulkirth edited the summary of this revision. (Show Details)

Introduce helper function to reduce repeated code and add tests for archive and --start-lib

paulkirth marked 2 inline comments as done.Jul 12 2023, 5:02 PM
paulkirth added inline comments.
lld/test/ELF/fatlto/fatlto.test
6

I opted to leave %t/, but have refactored to combine the rm -rf and split-file

man lld/docs/ld.lld.1. Can you also add the option to the manpage?

lld/test/ELF/fatlto/fatlto.test
7

Ditto everywhere

10

--add-section=.llvm.lto=%t/a-fatLTO.bc --set-section-flags=.llvm.lto=exclude

12

This llvm-readobj -S %t/a-fatLTO.o check is unneeded. The operation is tested in test/tools/llvm-objcopy/ELF.

20

This llvm-readobj -S %t/main-fatLTO.o check is unneeded.

28
paulkirth marked 6 inline comments as done.

Rebase and address comments

Add --fat-lto-objects to man page documentation

Revise manapge text for clarity

paulkirth added inline comments.Jul 17 2023, 10:56 AM
lld/docs/ld.lld.1
624–627 ↗(On Diff #541140)

I think this reads pretty naturally, but I'll defer to @MaskRay on the final text.

MaskRay added inline comments.Jul 17 2023, 12:02 PM
lld/ELF/Options.td
619

relocatable object file

lld/docs/ReleaseNotes.rst
34 ↗(On Diff #532273)

`--fat-lto-objects` option is added to support LLVM FatLTO.

lld/docs/ld.lld.1
624–627 ↗(On Diff #541140)

Perhaps

.It Fl -fat-lto-objects
Use the .llvm.lto section, which contains LLVM bitcode, in fat LTO object files to perform LTO.
.It Fl -no-fat-lto-objects
Ignore the .llvm.lto section in relocatable object files (default).

You can use man docs/ld.lld.1 to check the content. Fl appends a dash itself...

Address comments

  • update man page text
  • use relocatable object file more consistently
  • slightly revise text in release notes.
paulkirth marked 3 inline comments as done.Jul 17 2023, 12:25 PM
MaskRay added inline comments.Jul 17 2023, 12:51 PM
lld/ELF/Options.td
619

This can reuse the manpage description.

For example, the ".text" part in "Use the .text section in the relocatable object file (default)" is not accurate.

lld/test/ELF/fatlto/fatlto.invalid.s
4 ↗(On Diff #541186)

Add error: is present in the diagnostic.

10 ↗(On Diff #541186)

delete trailing blank line

lld/test/ELF/fatlto/fatlto.test
107

We should not test module summary index. They are unstable and may change, and cause hassle for future LTO changes. This part should be generated by opt --module-summary.

paulkirth updated this revision to Diff 541217.Jul 17 2023, 1:35 PM
paulkirth marked 4 inline comments as done.

Fix some test issues.

  • remove summaries from IR, and generate them w/ opt
  • improve error checking in fatlto.invalid.s
  • reuse wording from man page in help text
MaskRay accepted this revision.Jul 17 2023, 1:38 PM
This revision is now accepted and ready to land.Jul 17 2023, 1:38 PM

@MaskRay the CI failures here and in D152973 seem unrelated to the FatLTO patches (e.g. MLIR + windows).

Are we good w/ landing the linker support for LLD + gold now, or would you prefer that I rebase these and get the premerge checks passing first?

@MaskRay the CI failures here and in D152973 seem unrelated to the FatLTO patches (e.g. MLIR + windows).

Are we good w/ landing the linker support for LLD + gold now, or would you prefer that I rebase these and get the premerge checks passing first?

You may land this patch after fixing a "Support" nit for the release note. I have applied this patch and check-lld-elf passes.

You can also land the gold patch. Pre-commit build bots don't seem to check binutils. You'll need a local checkout of binutils-gdb, e.g. -DLLVM_BINUTILS_INCDIR=$HOME/Dev/binutils-gdb/include.

lld/docs/ReleaseNotes.rst
34 ↗(On Diff #532273)

Support should not be capitalized.

This revision was landed with ongoing or failed builds.Jul 19 2023, 4:07 PM
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
lld/test/ELF/fatlto/fatlto.test
55

Should this test be suppressed if x86 is uncofigred?

paulkirth added inline comments.Jul 19 2023, 6:01 PM
lld/test/ELF/fatlto/fatlto.test
55

Doesn’t this test have a requires x86 line?

chapuni added inline comments.Jul 19 2023, 6:08 PM
lld/test/ELF/fatlto/fatlto.invalid.s
1 ↗(On Diff #542232)

This should be suppressed, too.

lld/test/ELF/fatlto/fatlto.test
55

Sorry, this has required x86. fatlto.invalid.s doesn't.

paulkirth added inline comments.Jul 19 2023, 6:10 PM
lld/test/ELF/fatlto/fatlto.test
55

Oh shoot. Sorry about that. I’m mid commute atm. If you could revert or forward fix I’d appreciate it. Otherwise I can get to this in about 40 minutes.

Hey, an LLD test is failing on our buildbot after this was committed. Could you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/24150

dyung added a subscriber: dyung.Jul 19 2023, 7:13 PM
paulkirth reopened this revision.Jul 19 2023, 8:39 PM

@ormris, sorry for the delay on responding, I saw the forward fix go out and didn't check for a couple hours. I've reverted this for now until I can properly investigate, since I still see that bot having an issue after the forward fix.

This revision is now accepted and ready to land.Jul 19 2023, 8:39 PM
paulkirth updated this revision to Diff 543681.Jul 24 2023, 1:30 PM

Rebase, add REQUIRES line in test, and update summary

Looks like the windows failure was unrelated to the previous patch, and
that test is now unsupported on windows.

@MaskRay, I see that 29112a994694baee070a2021e00f772f1913d214 seems to have disabled the export-dynamic-symbols.s test on Windows, which was what was failing previously in CI. Would you be OK w/ a reland now that the REQUIRES line is in both tests, and the windows failure was unrelated?

paulkirth updated this revision to Diff 543682.Jul 24 2023, 1:32 PM
paulkirth retitled this revision from [lld] Preliminary fat-lto-object support to Reland "[lld] Preliminary fat-lto-object support".
paulkirth edited the summary of this revision. (Show Details)

Actually update summary on phabricator.

paulkirth updated this revision to Diff 543683.Jul 24 2023, 1:33 PM
paulkirth edited the summary of this revision. (Show Details)

Fix typo in summary.

paulkirth marked 2 inline comments as done.Jul 24 2023, 1:34 PM

@MaskRay are we OK w/ landing this now? We've reached consensus on the gold patch, so I think these should go in relatively close to one another.

@MaskRay are we OK w/ landing this now? We've reached consensus on the gold patch, so I think these should go in relatively close to one another.

LGTM. Please double check this works. We don't want more reverts.

This revision was automatically updated to reflect the committed changes.