This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Add very basic support for LTO
ClosedPublic

Authored by int3 on Nov 2 2020, 7:00 PM.

Details

Reviewers
compnerd
smeenai
MaskRay
Group Reviewers
Restricted Project
Commits
rG21f831134c90: [lld-macho] Add very basic support for LTO
Summary

Just enough to consume some bitcode files and link them. There's more
to be done around the symbol resolution API and the LTO config, but I don't yet
understand what all the various LTO settings do...

Diff Detail

Unit TestsFailed

Event Timeline

int3 created this revision.Nov 2 2020, 7:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 7:00 PM
int3 requested review of this revision.Nov 2 2020, 7:00 PM

Out of curiosity: are going to add distributed thinlto?
e.g. https://reviews.llvm.org/D46034

compnerd added inline comments.
lld/MachO/Driver.cpp
325

Why not write this as:

if (Optional<DylibFile *> dylib = makeDylibFromTAPI(mbref))
  newFile = *dylib;
334

LLVM style generally does not compare against nullptr or NULL.

lld/MachO/LTO.cpp
40

Could we not just do a .reserve on the std::vector on L37 and then do an emplace_back on the vector and use a range based for loop, and avoid the unnecessary construction and replacement of the resolutions?

int3 marked 3 inline comments as done.Nov 5 2020, 5:08 PM

@tschuett we'll want to implement that eventually, but it's not a priority. Thanks for the pointer to that diff!

int3 updated this revision to Diff 303302.Nov 5 2020, 5:08 PM

address comments

No worries and thank you!

compnerd accepted this revision.Nov 6 2020, 9:16 AM
This revision is now accepted and ready to land.Nov 6 2020, 9:16 AM
MaskRay added a subscriber: MaskRay.Nov 6 2020, 4:19 PM
MaskRay added inline comments.
lld/MachO/LTO.cpp
74

This needs a test file with more than one input file.

int3 added inline comments.Nov 9 2020, 9:55 AM
lld/MachO/LTO.cpp
74

I actually did try doing that (with 2 files), but it appears that the ltoObj compiled both of them in the same thread. Any idea how to get it to deterministically use more than 1?

MachO/CMakeLists.txt needs 3 dependencies LTO/MC/Passes otherwise it will fail to build in -DBUILD_SHARED_LIBS=on mode due to -Wl,-z,defs

MaskRay added inline comments.Nov 9 2020, 10:38 AM
lld/MachO/LTO.cpp
74

Ah, partitions is a ThinLTO concept. You'll need opt -module-summary a.ll -o a.o instead of llvm-as

smeenai accepted this revision.Nov 9 2020, 6:53 PM
smeenai added a subscriber: smeenai.

LGTM, very nice :)

int3 updated this revision to Diff 304039.Nov 9 2020, 9:25 PM
int3 marked 2 inline comments as done.

address comments

lld/MachO/LTO.cpp
74

Thanks for the pointer! I've added the test, let me know if it looks right

int3 updated this revision to Diff 304041.Nov 9 2020, 9:36 PM

pack into one test file

int3 updated this revision to Diff 304042.Nov 9 2020, 9:38 PM

fix

lld/test/MachO/lto-save-temps.ll
10 ↗(On Diff #304042)

it would be nice to check that the other temp files *don't* exist, but I'm not sure how to do that in a cross-platform way in llvm-lit

MaskRay accepted this revision.Nov 9 2020, 9:43 PM
Harbormaster completed remote builds in B78221: Diff 304041.
int3 updated this revision to Diff 304294.Nov 10 2020, 12:18 PM

fix use-after-free

This revision was landed with ongoing or failed builds.Nov 10 2020, 12:20 PM
This revision was automatically updated to reflect the committed changes.