[PATCH] D105922: [lld-macho] Add LTO cache support

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

LG with minor comments addressed :)



================
Comment at: lld/MachO/Driver.cpp:225
+  // Anything specified in `--thinlto-cache-policy` will override individual
+  // ld64-compat options in case of conflic.
+  if (const Arg *arg = args.getLastArg(OPT_thinlto_cache_policy)) {
----------------
I'd put this in the loop above: If you say `-prune_interval 10 --thinlto-cache-policy=20s -prune_interval 30` I think it's least surprising if you end up with a prune interval of 30s.

We usually follow a "last setting on commandline wins" policy: For example, `-Wno-return-type` disables the warning for missing return types, `-Wno-return-type -Wall` enables it because `-Wall` is later and overrides it, `-Wno-return-type -Wall -Wno-return-type` disables it. That's a somewhat academic discussion since in practice nobody will pass both kinds of flags here, but I think it's still better to process them in order.

Also, as-is, this ignores all but the last `--thin-lto-cache-policy=` flag, and processing them in the loop together with the others fixes that too.


================
Comment at: lld/test/MachO/lto-cache.ll:52
+; CHECK: llvmcache-newer
+; CHECK-NOT: llvmcache-old
+
----------------
thakis wrote:
> Can you add some coverage for the ld64-style options too?
nit: -max_relative_cache_size_lto is still untested


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

https://reviews.llvm.org/D105922