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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Nice :)



================
Comment at: lld/MachO/Driver.cpp:1184
+  }
+  if (const Arg *arg = args.getLastArg(OPT_prune_interval_lto)) {
+    if (cachePolicyStringProvided) {
----------------
lgrey wrote:
> This is really awkward and repetitive but I couldn't think of a way to abstract this without making it *more* complex 🤷
As far as I can tell, parseCachePruningPolicy() applies options on top of the defaults in the policy, so using it doesn't conflict with using the reasonable defaults for things not specified. The way I'd try to structure this is:

```
  SmallString<128> ltoPolicy; 
  auto add = [ltoPolicy](Twine add) {
    if (ltoPolicy.empty())
      ltoPolicy.append(':');
    ltoPolicy.append(add);
  }
  for (const Arg *arg : args.filtered(OPT_thinlto_cache_policy, OPT_prune_interval_lto,
                                                         OPT_prune_after_lto, OPT_max_relative_cache_size_lto)) {
    switch (arg->getOption().getID()) {
    case OPT_thinlto_cache_policy: add(arg->getValue()); break;
    case OPT_prune_interval_lto:
      if (!strcmp("-1", arg->getValue())
        add("prune_interval=87600h"); // 10 years
      else
        add(Twine("prune_interval=") + arg->getValue() + "s");
      break;
    case OPT_prune_after_lto: add(Twine("prune_after=" + arg->getValue() + "s"); break;
    case OPT_max_relative_cache_size_lto: add(Twine("cache_size=" + arg->getValue() + "%"); break;
    }
  }
  config->thinLTOCachePolicy =
        CHECK(parseCachePruningPolicy(ltoPolicy),
              "invalid LTO cache policy");
```

I think that should work and it seems like a fairly natural way to handle this to me.

In any case, please pull the cache policy flag handling logic into a `handleLTOPolicyFlags()` helper function.


================
Comment at: lld/MachO/LTO.cpp:129
     for (unsigned i = 1; i != maxTasks; ++i)
-      saveBuffer(buf[i], config->outputFile + Twine(i) + ".lto.o");
+      if (!buf[i].empty())
+        saveBuffer(buf[i], config->outputFile + Twine(i) + ".lto.o");
----------------
Why is this here? The other ports don't have it.


================
Comment at: lld/MachO/LTO.cpp:155
+    if (file)
+      ret.push_back(make<ObjFile>(*file, 0, ""));
   return ret;
----------------
The COFF port says

```
    // Get the native object contents either from the cache or from memory.  Do
    // not use the cached MemoryBuffer directly, or the PDB will not be
    // deterministic.
```

Is this something we need to worry about? (Fine as-is for now in any case though.)


================
Comment at: lld/MachO/Options.td:71
     Group<grp_lld>;
+def thinlto_cache_policy: Separate<["--"], "thinlto-cache-policy">,
+    HelpText<"Pruning policy for the ThinLTO cache">,
----------------
lld-style options use `--` and style Joined if they take args, and then use a trailing `=` (…usually. `--lto-O` is an exception). This makes the lld extensions look different from the ld64 compat options.

(eg `Joined<["--"], "thinlto-cache-policy=" ...`)


================
Comment at: lld/test/MachO/Inputs/lto-cache.ll:10
+
+declare void @globalfunc(...)
----------------
You can use `split-file` to have several .ll files in a single .ll test file. See e.g. lld/test/MachO/lto-save-temps.ll for an example.

(split-file is new(ish), that's why most ELF tests don't use it yet)


================
Comment at: lld/test/MachO/lto-cache.ll:5
+
+; RUN: opt -module-hash -module-summary %s -o %t.o
+; RUN: opt -module-hash -module-summary %p/Inputs/lto-cache.ll -o %t2.o
----------------
Mention in CL desc that the test is based on lld/test/ELF/lto/cache.ll


================
Comment at: lld/test/MachO/lto-cache.ll:12
+; RUN: touch -t 197001011200 %t.cache/llvmcache-foo %t.cache/foo
+; RUN: %lld -cache_path_lto %t.cache --thinlto-cache-policy prune_after=1h:prune_interval=0s -o %t3 %t2.o %t.o
+
----------------
nit: You can put `\` at the end of a RUN line to continue it on the next line:

```
; RUN: foo bar baz \
; RUN:   quux
```

LLVM's style technically is 80 cols, but some people go a bit over in test files sometimes. But keeping it short enough so it doesn't wrap in phab is probably a good idea.


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


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

https://reviews.llvm.org/D105922