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

[Leonard Grey via Phabricator via llvm-commits]

lgrey added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1184
+  }
+  if (const Arg *arg = args.getLastArg(OPT_prune_interval_lto)) {
+    if (cachePolicyStringProvided) {
----------------
thakis wrote:
> 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.
Sorry, I wasn't clear. What I meant:
-`parseCachePruningPolicy` creates a new object, vs. updating an existing object in place
- Since a default-constructed `CachePruningPolicy` has defaults, you can't tell which fields `parseCachePruningPolicy` are default and which came from the string
- So that means, you either fill in a default constructed object and set values from the individual options and then stomp it with the new object from `parseCachePruningPolicy`, or you create an object with `parseCachePruningPolicy`, and fill in the individual options, except we have no way of knowing which fields were set by the policy string and which are default.

That said, a variant of this way works! But: it relies on an implementation detail in `parseCachePruningPolicy`: the fact that the string is parsed in order. That feels fragile to me, but happy to defer.


================
Comment at: lld/MachO/Driver.cpp:1190
+      int value = 0;
+      if (!llvm::to_integer(arg->getValue(), value))
+        error("-prune_interval_lto: invalid value: " + Twine(arg->getValue()));
----------------
int3 wrote:
> no need for llvm::
Using the method thakis@ suggested instead.


================
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");
----------------
thakis wrote:
> Why is this here? The other ports don't have it.
Reverted because of Chesterton's fence, but I don't really get it. We check for the first one, why not the rest? Overall, it's not immediately obvious to me that the interaction of save temps and caching works correctly as written in ELF/COFF (are we overwriting existing temps from cache hits with an empty buffer?)


================
Comment at: lld/MachO/LTO.cpp:155
+    if (file)
+      ret.push_back(make<ObjFile>(*file, 0, ""));
   return ret;
----------------
thakis wrote:
> 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.)
Based on the [review when it was added](https://reviews.llvm.org/D78221#1985775), I would *guess* not. I'll experiment with some dSYMs.


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

https://reviews.llvm.org/D105922