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

[Katya Romanova via Phabricator via llvm-commits]

kromanova added inline comments.


================
Comment at: include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:190
+  /// over the amount of available space on the disk will be reduced to the
+  /// amount of available space. A value of 0 will be ignored.
+  void setCacheMaxSizeBytes(unsigned MaxSizeBytes) {
----------------
tejohnson wrote:
> 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)?
In D42267  a value 0 for PruningInterval actually forces the scan to occur, so its meaning is somewhat different.





================
Comment at: include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:190
+  /// over the amount of available space on the disk will be reduced to the
+  /// amount of available space. A value of 0 will be ignored.
+  void setCacheMaxSizeBytes(unsigned MaxSizeBytes) {
----------------
kromanova wrote:
> tejohnson wrote:
> > 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)?
> In D42267  a value 0 for PruningInterval actually forces the scan to occur, so its meaning is somewhat different.
> 
> 
> 
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.




================
Comment at: tools/llvm-lto/llvm-lto.cpp:166
+static cl::opt<int>
+    ThinLTOCacheMaxSizeFiles("thinlto-cache-max-size-files", cl::init(100000), cl::desc("Set ThinLTO cache pruning directory maximum number of files."));
+
----------------
tejohnson wrote:
> tejohnson wrote:
> > line too long
> default here is off by a factor of 10 from the default in CachePruning.h.
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);
```




================
Comment at: tools/llvm-lto/llvm-lto.cpp:166
+static cl::opt<int>
+    ThinLTOCacheMaxSizeFiles("thinlto-cache-max-size-files", cl::init(100000), cl::desc("Set ThinLTO cache pruning directory maximum number of files."));
+
----------------
kromanova wrote:
> tejohnson wrote:
> > tejohnson wrote:
> > > line too long
> > default here is off by a factor of 10 from the default in CachePruning.h.
> 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);
> ```
> 
> 
Ah, good catch! Thanks. Can't count my zeros.


https://reviews.llvm.org/D42446