Page MenuHomePhabricator

[lld-macho] Add LTO cache support
ClosedPublic

Authored by lgrey on Jul 13 2021, 11:21 AM.

Details

Reviewers
int3
gkm
thakis
Group Reviewers
Restricted Project
Commits
rGc931ff72bde4: [lld-macho] Add LTO cache support
Summary

This adds support for the lld-only --thinlto-cache-policy option, as well as implementations for ld64's -cache_path_lto, -prune_interval_lto, -prune_after_lto, and -max_relative_cache_size_lto.

Because llvm::CachePruningPolicy has reasonable default values, it's infeasible to give --thinlto-cache-policy precedence over the individual ld64 cache pruning options without replicating the parsing code. Currently, we ignore the individual options when the composite option is provided, but we can also give the individual options precedence if that seems better.

Test is adapted from lld/test/ELF/lto/cache.ll

Diff Detail

Event Timeline

lgrey created this revision.Jul 13 2021, 11:21 AM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
lgrey requested review of this revision.Jul 13 2021, 11:21 AM
lgrey added inline comments.
lld/MachO/Driver.cpp
1211

This is really awkward and repetitive but I couldn't think of a way to abstract this without making it *more* complex 🤷

Nice :)

lld/MachO/Driver.cpp
1211

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.

lld/MachO/LTO.cpp
129

Why is this here? The other ports don't have it.

154

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.)

lld/MachO/Options.td
71

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=" ...)

lld/test/MachO/Inputs/lto-cache.ll
10 ↗(On Diff #358356)

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)

lld/test/MachO/lto-cache.ll
6

Mention in CL desc that the test is based on lld/test/ELF/lto/cache.ll

13

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.

53

Can you add some coverage for the ld64-style options too?

int3 added inline comments.Jul 14 2021, 11:53 AM
lld/MachO/Driver.cpp
1217

no need for llvm::

lld/MachO/LTO.h
12

if we only need MemoryBuffer in this header, can we just include MemoryBuffer.h?

lld/test/MachO/lto-cache.ll
10–11

nit: we usually double-comment the "real" comments in lit tests, to distinguish them from RUN/CHECK lines

lgrey updated this revision to Diff 358778.Jul 14 2021, 4:28 PM
lgrey marked 7 inline comments as done.
lgrey edited the summary of this revision. (Show Details)
lgrey added inline comments.
lld/MachO/Driver.cpp
1211

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.

1217

Using the method thakis@ suggested instead.

lld/MachO/LTO.cpp
129

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?)

154

Based on the review when it was added, I would *guess* not. I'll experiment with some dSYMs.

lgrey marked an inline comment as done.Jul 14 2021, 4:29 PM
thakis accepted this revision.Jul 14 2021, 6:00 PM

LG with minor comments addressed :)

lld/MachO/Driver.cpp
225

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.

lld/test/MachO/lto-cache.ll
53

nit: -max_relative_cache_size_lto is still untested

This revision is now accepted and ready to land.Jul 14 2021, 6:00 PM
thakis added inline comments.Jul 15 2021, 4:00 AM
lld/test/MachO/lto-cache.ll
28

Looks like date -v fails on Linux and Windows, see presubmit bot above.

lgrey updated this revision to Diff 359013.Jul 15 2021, 9:23 AM

thakis@ comments

lgrey marked 3 inline comments as done.Jul 15 2021, 9:25 AM
lgrey added inline comments.
lld/test/MachO/lto-cache.ll
28

Removed since the functionality this was testing changed

This revision was landed with ongoing or failed builds.Jul 15 2021, 9:59 AM
Closed by commit rGc931ff72bde4: [lld-macho] Add LTO cache support (authored by lgrey, committed by thakis). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2021, 9:59 AM