Page MenuHomePhabricator

[ThinLTO][ELF] Add --thinlto-emit-index-files option
ClosedPublic

Authored by Northbadge on Jun 14 2022, 12:59 PM.

Details

Summary

Allows ThinLTO indices to be written to disk on-the-fly/as-part-of “normal” linker execution. Previously ThinLTO indices could be written via --thinlto-index-only but that would cause the linker to exit early. For MLGO specifically, this enables saving the ThinLTO index files without having to restart the linker to collect data only available at later stages (i.e. output of --save-temps) of the linker's execution.

Note, this option does not currently work with:
--thinlto-object-suffix-replace, as this is intended to be used to consume minimized IR bitcode files while --thinlto-emit-index-files is intended to be run together with InProcessThinLTO (which cannot parse minimized IR).
--thinlto-prefix-replace support is left unimplemented but can be implemented if needed

Diff Detail

Event Timeline

Northbadge created this revision.Jun 14 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 12:59 PM
Northbadge requested review of this revision.Jun 14 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 12:59 PM

Add missing braces

gentle reminder, thanks!

tejohnson added inline comments.Jun 21 2022, 12:14 PM
lld/test/ELF/lto/thinlto-emit-index.ll
40

I don't follow this comment, and we shouldn't be generating any imports files with this option I believe? But comment on that later on - looks like that would be trivial to support.

llvm/include/llvm/LTO/LTO.h
201

Document new parameters

llvm/lib/LTO/LTO.cpp
1231

It seems pretty straightforward to support import file emission along with the index files - maybe just go ahead and wire it up?

Allows ThinLTO indices to be written to disk on-the-fly/as-part-of “normal” linker execution. Previously thinLTO indices could be written via --thinlto-index-only but that would cause the linker to exit early.

Can you state the motivation for not exiting early? What files are needed for --thinlto-emit-index-files?

lld/test/ELF/lto/thinlto-emit-index.ll
4

lld/test/ELF tests, at least the new ones, use ;; for non-RUN-non-CHECK lines. This makes the comments stand out and helps possibly future FileCheck feature to catch unused/misused check prefixes.

6

Use rm -rf %t && mkdir %t && cd %t. This is more readable and less error-prone than many rm -f %t*.

Northbadge marked 5 inline comments as done.

Add support for --thinlto-emit-imports-files, and resolved inline comments. Thanks for the reviews!

Northbadge edited the summary of this revision. (Show Details)Jun 22 2022, 11:24 AM

Allows ThinLTO indices to be written to disk on-the-fly/as-part-of “normal” linker execution. Previously thinLTO indices could be written via --thinlto-index-only but that would cause the linker to exit early.

Can you state the motivation for not exiting early?

I've updated the summary, but essentially, for MLGO it lets us collect the ThinLTO index files and any IR or .o files we need in one execution of the linker, rather than 2.

I'm not too sure what you meant with

What files are needed for --thinlto-emit-index-files?

though. It should require all the files you'd normally pass in to lld to do a ThinLTO link, except this patch enables passing extra options to save the ThinLTO index files.

tejohnson accepted this revision.Jun 22 2022, 12:14 PM

lgtm with one comment fix suggestion. Wait to see if @MaskRay has any additional comments.

lld/test/ELF/lto/thinlto-emit-index.ll
38

Suggest: "Test that LLD also generates empty imports file with the --thinlto-emit-imports-files option." (I realize the original comment was cloned from the other test, but let's go ahead and improve it here.)

This revision is now accepted and ready to land.Jun 22 2022, 12:14 PM
MaskRay requested changes to this revision.Jun 22 2022, 4:20 PM
MaskRay added inline comments.
lld/test/ELF/lto/thinlto-emit-index.ll
8

Since you have used cd, you don't need %t in the output filenames (%t1.o). You may just use 1.o.

I just updated thinlto-index-only.ll

55

thinlto-prefix-replace has been tested. Don't repeat the test here.

This revision now requires changes to proceed.Jun 22 2022, 4:20 PM
Northbadge marked 2 inline comments as done.
Northbadge edited the summary of this revision. (Show Details)

Resolved most inline comments

lld/test/ELF/lto/thinlto-emit-index.ll
55

I'm testing to make sure that an error is thrown when both thinlto-emit-index-files and thinlto-prefix-replace is specified though

MaskRay accepted this revision.Jun 22 2022, 8:47 PM
MaskRay added inline comments.
lld/test/ELF/lto/thinlto-emit-index.ll
12

You may change all test -e to test -f or just ls.

This revision is now accepted and ready to land.Jun 22 2022, 8:47 PM
Northbadge marked 2 inline comments as done.

Switch from test to ls. Thanks!

This revision was landed with ongoing or failed builds.Jun 23 2022, 12:38 PM
This revision was automatically updated to reflect the committed changes.