This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add a couple of more knobs to C API to control cache size.
ClosedPublic

Authored by kromanova on Jan 23 2018, 3:04 PM.

Details

Summary

Exposing a couple of C LTO API functions to control the size and amount of files in cache directory. These functions have been supported in C++ LTO API for a long time.

Diff Detail

Repository
rL LLVM

Event Timeline

kromanova created this revision.Jan 23 2018, 3:04 PM

Hello,
Will it possible to review this patch also? Thank you so much in advance.
Does someone knows who in Apple is responsible for ThinLTO? It might be helpful to add this person as a reviewer.
Katya.

tejohnson added inline comments.Jan 30 2018, 7:48 PM
include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
169

extraneous new line

189

In CachePruning.h, a value of 0 for this field disables size based pruning. Why do something different here (which seems to go in the opposite direction from D42267)?

tools/llvm-lto/llvm-lto.cpp
164

line too long

167

line too long

167

default here is off by a factor of 10 from the default in CachePruning.h.

kromanova updated this revision to Diff 132127.Jan 31 2018, 2:47 AM
kromanova marked 4 inline comments as done.

Addressed code review comments.

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
189

In D42267 a value 0 for PruningInterval actually forces the scan to occur, so its meaning is somewhat different.

189

When writing the patch, I was hesitating for a whether to ignore value 0 here (for a time being) or use it for initializing CacheOptions.Policy.* (as a part of this patch).

I decided to ignore a value 0 for now and that's why :

the value 0 for two previously existing pruning control functions is currently ignored (while I believe it should be changed to have the same effect as in C++ AP!)

  • setCacheEntryExpiration
  • setMaxCacheSizeRelativeToAvailableSpace

I added these two knobs in this patch:

  • setCacheMaxSizeBytes
  • setCacheMaxSizeFiles

and decided to keep for now consistent with setCacheEntryExpiration and setMaxCacheSizeRelativeToAvailableSpace, since all of these 4 functions are related.

I could have changed all of these functions and accept value 0 as a part of this patch, but a lot of tests needs to be written, so I thought it's better to do it as a separate commit.

tools/llvm-lto/llvm-lto.cpp
167

Done.

However, I noticed that most of the functions from line 68 6o 215 in llvm-lto.cpp are formatted very inconsistently. I'd like to make the formatting uniformed. It could be done as a part of this or (preferably) different commit. If you think this "tidying up" needs to be done, which function should I use as a good example?
I personally prefer the formatting similar to the one used below, but I'm open to any other. as long as it's consistent. Or I could just run this part of the file through clang-format. Let me know.

static cl::list<std::string> ExportedSymbols(
    "exported-symbol",
    cl::desc("List of symbols to export from the resulting object file"),
    cl::ZeroOrMore);
167

Ah, good catch! Thanks. Can't count my zeros.

kromanova planned changes to this revision.Jan 31 2018, 2:57 AM
kromanova added inline comments.
include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
189

Also, before making a subsequent commit to accept (instead of ignoring) 0 values for setCache* functions, I'd like to get ld64 developers option that they are OK with this. Who is responsible for ld64 or LTO project in Apple?

Please let me know if more changes needed to this patch.

Based on the discussion in this code review, I'm planning to make two simple follow-up patches:
(1) formatting in llvm-lto.cpp (let me know an example of preferred formatting that I could use as a template or I could use plain clang-format).
(2) accepting 0 values for setCacheEntryExpiration, setMaxCacheSizeRelativeToAvailableSpace, setCacheMaxSizeBytes, setCacheMaxSizeFiles.

Adding Apple's developers to the list of subscribers, since this patch is related to C LTO API .

Teresa said that Peter Collingbourne might be the code owner for C LTO API, so I'm including him as a reviewer for this code review as well. This review was sitting for a while. Peter, could you please have a look and approve if everything looks OK to you.

pcc added a comment.Feb 12 2018, 4:09 PM

I have no strong feelings about what happens in the legacy API, so probably someone at Apple should take a look first.

Peter suggested that someone from Apple should have a look. I added Steven Wu as a reviewer.
Steven, could you (or someone else responsible for ld64 or ThinLTO) review and approve?

The API change is fine for ld64. But you need to do the following when you update LTO C API:

  • You need to declare the interface in include/llvm-c/lto.h
  • You need to bump LTO_API_VERSION in that header and document the new API is available since the new LTO_API_VERSION.
  • You need to update tools/lto/lto.exports. This is the export list used by ld64 and only the interface listed in that file will be exported on darwin platform.
kromanova updated this revision to Diff 136219.Feb 27 2018, 7:38 PM

Updated this patch to address Steven Wu's comments:

  • Declared new interfaces in include/llvm-c/lto.h;
  • Bumped LTO_API_VERSION;
  • Documented the new APIs available since the new LTO_API_VERSION;
  • Updated tools/lto/lto.exports (hopefully the order of the functions is not important, because I re-group them slightly).
steven_wu accepted this revision.Feb 28 2018, 4:20 PM

LGTM. And yes, order doesn't matter.

This revision is now accepted and ready to land.Feb 28 2018, 4:20 PM
This revision was automatically updated to reflect the committed changes.