This is an archive of the discontinued LLVM Phabricator instance.

[lld/mac] Add support for distributed ThinLTO
ClosedPublic

Authored by thakis on Nov 21 2022, 11:20 AM.

Details

Reviewers
MaskRay
Group Reviewers
Restricted Project
Commits
rGaa0883b59ae1: [lld/mac] Add support for distributed ThinLTO
Summary

Adds support for the following flags:

  • --thinlto-index-only, --thinlto-index-only=
  • --thinlto-emit-imports-files
  • --thinlto-emit-index-files
  • --thinlto-object-suffix-replace=
  • --thinlto-prefix-replace=

See https://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
for some words on --thinlto-index-only.

I don't really need the other flags, but they were in the vicinity
and _someone_ might need them, so I figured I'd add them too.

-object_path_lto now sets c.AlwaysEmitRegularLTOObj as in the other ports,
which means it can now only point to a filename for non-thin LTO.
I think that was the intent of D129705 anyways, so update
test/MachO/lto-object-path.ll to use a non-thin bitcode file for that test.

Diff Detail

Event Timeline

thakis created this revision.Nov 21 2022, 11:20 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 21 2022, 11:20 AM
thakis requested review of this revision.Nov 21 2022, 11:20 AM

Distraction: I always thought a local distributed link process is less scary to ninja, because *most* linker processes are single-threaded.

thakis retitled this revision from [lld/mac] Add support distributed thinlto to [lld/mac] Add support for distributed thinlto.Nov 21 2022, 4:22 PM
thakis edited the summary of this revision. (Show Details)Nov 22 2022, 11:05 AM

@MaskRay, would you be comfortable reviewing this? Afaict you did quite a bit of work on the elf version. This should look fairly similar to that.

thakis added inline comments.Nov 23 2022, 5:58 AM
lld/MachO/LTO.cpp
193

This here got extracted to the outputFilePath lambda, since I need this path in the index-only codepath as well.

(If desired, I could pull the minor refactoring and reordering in this function into its own patch. I figured it's small enough that that's not needed.)

247

This is moving up above the pruneCache call because I need objPathIsDir in the index only path, and the index-only handling happens before cache pruning. Moving this up should be safe.

thakis updated this revision to Diff 477489.Nov 23 2022, 6:44 AM

add thinlto-obj-path.ll; forgot to git add lld/test/MachO/thinlto-obj-path.ll previously

MaskRay added a comment.EditedNov 23 2022, 2:53 PM

I'd like to. I am going to be out of town for one week starting from tomorrow (then another week starting from 12-06) so I am likely slow to getting to this...

No rush, this isn't urgent. Thanks!

Friendly ping, the week of 12/6 is over :)

(But there's still no rush, as long as it isn't forgotten.)

(Mostly out of town 2022-12-01 ~ 2022-12-05, and have a long pile of backlog now...)

Generally looks good. lld/MachO/CMakeLists.txt should add BitWriter to fix -DBUILD_SHARED_LIBS=on builds.

lld/MachO/LTO.cpp
250

Why does this have /tmp/?

lld/test/MachO/thinlto-index-only.ll
3

if you append cd %t, the %t/ uses below can be removed.

11

not test -e %t/%t/3 this test is unneeded?

26
lld/test/MachO/thinlto-no-index.ll
2

This test can be merged into thinlto-index-only.ll.

I'll remove test/ELF/lto/thinlto-no-index.ll

10
lld/test/MachO/thinlto-prefix-replace.ll
14
MaskRay added inline comments.Jan 3 2023, 3:49 PM
lld/test/MachO/thinlto-obj-path.ll
6 ↗(On Diff #477489)

FYI I just improved ELF/lto/thinlto-obj-path.ll and ELF/lto/obj-path.ll a bit.

thakis updated this revision to Diff 488301.Jan 11 2023, 11:18 AM
thakis marked 6 inline comments as done.

address comments

Thanks!

lld/MachO/LTO.cpp
250

This just moved up from below. But it's for when you don't pass -object_path_lto, then the .o files go into /tmp/lto.tmp. (See https://reviews.llvm.org/D138451#inline-1337643)

lld/test/MachO/thinlto-index-only.ll
3

I'm not a fan of that style: I often copy failing RUN lines from lit output into my terminal, and I don't want to have to cd there.

11

Sorry, this was meant to say not test -e %t/3. It tests that the lld invocation two lines above doesn't write its output in --thinlto-index-only mode.

lld/test/MachO/thinlto-no-index.ll
2

(That happened in b5ab42af84de – merged into this one too)

MaskRay accepted this revision.Jan 11 2023, 11:25 AM

LGTM. Perhaps another #lld-macho folk wants to take a look as well.

lld/MachO/Driver.cpp
1838
lld/MachO/InputFiles.cpp
2218

You can use structured binding declaration now that C++17 can be used.

lld/test/MachO/thinlto-index-only.ll
3

OK, it's fine. I use a custom script to parse RUN lines and track CWD, so cd %t isn't really a problem for me...
I find %t especially convenient when multiple output files are created. In rare occasions whether an output file exists prior to the test execution matters and ensuring everything is removed makes the test more robust.

This revision is now accepted and ready to land.Jan 11 2023, 11:25 AM

thinlto (subject)

ThinLTO

Adds support for the following flags:

Better to add double dashes to the following bullet points.

for some words on -thinlto-index-only.

--thinlto-index-only (the patch only allows double-dash forms which match lld/ELF).

thakis updated this revision to Diff 488334.Jan 11 2023, 12:23 PM
thakis marked 2 inline comments as done.
thakis edited the summary of this revision. (Show Details)

tweaks

thakis added a comment.EditedJan 11 2023, 12:25 PM

Thanks! I'll wait another day, but this has been sitting here for a while, so I think folks had plenty time to comment :)

thakis retitled this revision from [lld/mac] Add support for distributed thinlto to [lld/mac] Add support for distributed ThinLTO.Jan 11 2023, 7:28 PM
MaskRay accepted this revision.Jan 11 2023, 7:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 5:54 PM

FYI:

This introduced build failure (due to link time error : undefined reference to "writeIndexToFile"),
and was later fixed by https://github.com/llvm/llvm-project/commit/87bea593626a103ea4d63ab4a027ec956ccc2b56

haowei added a subscriber: haowei.Feb 24 2023, 4:14 PM

We are seeing a linker error on Mac builder today when we try to roll the clang toolchain for Flutter, error message (build task: https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20Unopt/25679/overview):

[3356/6995] LINK ./geometry_benchmarks
[3357/6995] SOLINK libtessellator.dylib libtessellator.dylib.TOC
FAILED: libtessellator.dylib libtessellator.dylib.TOC 
if [ ! -e ./libtessellator.dylib -o ! -e ./libtessellator.dylib.TOC ] || otool -l ./libtessellator.dylib | grep -q LC_REEXPORT_DYLIB ; then ../../buildtools/mac-x64/clang/bin/clang++ -shared -isysroot /Volumes/Work/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk -mmacosx-version-min=10.14.0  -Wl,-object_path_lto,./lto_libtessellator.o -arch x86_64 -nostdlib++ -Wl,-search_paths_first -L. -Wl,-rpath,@loader_path/. -Wl,-rpath,@loader_path/../../.. -o ./libtessellator.dylib -Wl,-filelist,./libtessellator.dylib.rsp   -framework Foundation && { otool -l ./libtessellator.dylib | grep LC_ID_DYLIB -A 5; nm -gP ./libtessellator.dylib | cut -f1-2 -d' ' | grep -v U$$; true; } > ./libtessellator.dylib.TOC; else ../../buildtools/mac-x64/clang/bin/clang++ -shared -isysroot /Volumes/Work/s/w/ir/cache/osx_sdk/XCode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk -mmacosx-version-min=10.14.0  -Wl,-object_path_lto,./lto_libtessellator.o -arch x86_64 -nostdlib++ -Wl,-search_paths_first -L. -Wl,-rpath,@loader_path/. -Wl,-rpath,@loader_path/../../.. -o ./libtessellator.dylib -Wl,-filelist,./libtessellator.dylib.rsp   -framework Foundation && { otool -l ./libtessellator.dylib | grep LC_ID_DYLIB -A 5; nm -gP ./libtessellator.dylib | cut -f1-2 -d' ' | grep -v U$$; true; } > ./libtessellator.dylib.tmp && if ! cmp -s ./libtessellator.dylib.tmp ./libtessellator.dylib.TOC; then mv ./libtessellator.dylib.tmp ./libtessellator.dylib.TOC ; fi; fi
ld64.lld: error: No available targets are compatible with triple ""
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

We did a manual bisecting clang prebuilts and we think this patch is causing the issue.

The bug can be reproduced on a Mac x64 with following steps:

  • Create a source file foo.c containing int foo() { return 123; }
  • Run clang++ -c -o foo.o foo.c
  • Run clang++ -shared -nostdlib -Wl,-object_path_lto,foo.o -o foo.dylib foo.o

With this patch, it will output: ld64.lld: error: No available targets are compatible with triple "". Without this patch, the clang++ will return 0.
Would you mind take a look please?

We are still trying to understand why this patch is causing this issue, maybe it uncovered a bug somewhere else.

We are seeing a linker error on Mac builder today when we try to roll the clang toolchain for Flutter, error message (build task: https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20Unopt/25679/overview):

I was away on leave for a bit. Are you still seeing this? It feels like it should be unrelated (?)

With this patch, it will output: ld64.lld: error: No available targets are compatible with triple "". Without this patch, the clang++ will return 0.
Would you mind take a look please?

We are still trying to understand why this patch is causing this issue, maybe it uncovered a bug somewhere else.

We're seeing the same with Firefox, and this comes from the AlwaysEmitRegularLTOObj change. The problem we have is that we use -object_path_lto to have a deterministic path that keeps the objects around for dsymutil (otherwise, being temporary files, they are removed, although I guess we could use -save-temps instead, but that would remove the determinism), and /some/ things we build end up not using LTO, so the LTO dir ends up empty for them, but with AlwaysEmitRegularLTOObj being true, the error is emitted. I guess there's a case to be made that maybe that error should not be emitted when the directory is empty because there was nothing to LTO.