[PATCH] D138451: [lld/mac] Add support for distributed thinlto

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks!



================
Comment at: lld/MachO/LTO.cpp:250
+  auto outputFilePath = [objPathIsDir](int i) {
+    SmallString<261> filePath("/tmp/lto.tmp");
+    if (!config->ltoObjPath.empty()) {
----------------
MaskRay wrote:
> Why does this have `/tmp/`?
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)


================
Comment at: lld/test/MachO/thinlto-index-only.ll:2
+; REQUIRES: x86
+; RUN: rm -rf %t; split-file %s %t
+
----------------
MaskRay wrote:
> if you append `cd %t`, the `%t/` uses below can be removed.
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.


================
Comment at: lld/test/MachO/thinlto-index-only.ll:10
+; RUN: ls %t/2.o.thinlto.bc
+; RUN: not test -e %t/%t/3
+; RUN: %lld -dylib %t/1.o %t/2.o -o %t/3
----------------
MaskRay wrote:
> `not test -e %t/%t/3` this test is unneeded?
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.


================
Comment at: lld/test/MachO/thinlto-no-index.ll:1
+; REQUIRES: x86
+; RUN: rm -rf %t; split-file %s %t
----------------
MaskRay wrote:
> This test can be merged into thinlto-index-only.ll.
> 
> I'll remove test/ELF/lto/thinlto-no-index.ll
(That happened in b5ab42af84de – merged into this one too)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138451/new/

https://reviews.llvm.org/D138451