[PATCH] D42267: [ThinLTO] Allow 0 to be a valid value for pruning interval for C LTO API.

[Katya Romanova via Phabricator via llvm-commits]

kromanova added inline comments.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:161
+    ThinLTOCachePruningInterval("thinlto-cache-pruning-interval", 
+    cl::init(1200), cl::desc("Set ThinLTO cache pruning interval."));
 
----------------
bd1976llvm wrote:
> Under what circumstances does this change have an effect? 
> 
> I remember that the same default is set in the CachePruningPolicy constructor.
Before my change in llvm-lto.cpp we effectively overwrote the default value of PruningInterval, initializing it with 0.

```
ThinLTOProcessing(const TargetOptions &Options) {
    ...
    ThinGenerator.setCacheDir(ThinLTOCacheDir);
    ThinGenerator.setCachePruningInterval(ThinLTOCachePruningInterval);  //set PruningInterval to 0
    ThinGenerator.setFreestanding(EnableFreestanding);
    ...
}
```  
All other knobs (e.g. Expiration or MaxSizePercentageOfAvailableSpace) determining the pruning policy are not being set in llvm-lto.cpp, 
so they keep the default values assigned by CachePruningPolicy constructor.
With my change, we keep the default value (1200) for PruningInterval.

Why this is needed? I guess for consistency. When I wrote my tests, I compared the default pruning policy behavior with the behavior when PruningInterval == 0. 
I was puzzled to notice that the behavior was the same. Only then I noticed that we are intentionally setting the value 0 to PruningInterval in llvm-lto.cpp.
I decided to fix it. However, if this behavior is undesirable, I don't mind to change my test to explicitly pass option for setting PruningInterval value for both of the tests, rather than using a default value.



Repository:
  rL LLVM

https://reviews.llvm.org/D42267