[ThinLTO][C-API] Correct api comments

[bd1976llvm]

Summary

Negative values never disabled the pruning - they simply set high values for the pruning interval.

Once https://reviews.llvm.org/D41231 lands the behaviour will be that negative values set the maximum pruning interval (which appears to have been the intention from the start).

I have adjusted the comments to reflect this, removed any inaccurate statements, and corrected any typos I spotted in the English.

[tejohnson]

include/llvm-c/lto.h

767 ↗

(On Diff #127091)

I prefer leaving a note here about the value -1. You can note that it sets the largest possible pruning interval, effectively disabling it.

[tejohnson]

include/llvm-c/lto.h

767 ↗

(On Diff #127091)

actually, a negative value, not just -1.

[mehdi_amini]

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

152 ↗

(On Diff #127091)

Why are you removing the indication about the default?

[bd1976llvm]

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

152 ↗

(On Diff #127091)

This was the original default but it was changed later and the comment was not updated. See: https://github.com/llvm-mirror/llvm/commit/f4c5ea4908a0d2f1e9adc793b74858dd3c69cc7a.

[bd1976llvm]

Updated patch to address review comments

[tejohnson]

LGTM

[kromanova]

include/llvm-c/lto.h

767 ↗

(On Diff #127177)

It will be nice if you add one more clause in this sentence, explaining that this effectively disables pruning.

786 ↗

(On Diff #127177)

It looks like we are setting the default values in CachePruning.h, so the comment "An unspecified default value" could be substituted with "Default value of 1200 seconds (20 minutes) is applied".

Obsolete comments are confusing. When I called this API in Sony's proprietary linker, I relied on this particular comment and end up doing initialization myself in my code. BTW, Ben, since I noticed it, we need to remove this initialization code I talked about from our (Sony's) proprietary linker.

[kromanova]

include/llvm-c/lto.h

786 ↗

(On Diff #127177)

I would prefer mentioning one more time that negative values effectively disable pruning. Hopefully, after your change the umbrella comment above mentions it, but it will be nice to repeat it the comment directly related to this function.

787 ↗

(On Diff #127177)

Also, a value of 0 is not ignored, but instead it forces the scan to occur. See comment in CachePruning.h

However, please do not make this change now, leave it "as is". I will send a patch a little later to accept 0 interval in thinlto_codegen_set_cache_pruning_function (as well as in 2 other APIs for setting relative size and expiration). I will do this change in the comment(s) at the same time as I do the functional change itself.

[kromanova]

include/llvm-c/lto.h

795 ↗

(On Diff #127177)

Please apply the same changes as you did for thinlto_codegen_set_cache_pruning_interval
to the rest of thinLTO APIs responsible for pruning policy/incremental builds.
Otherwise, the comments look inconsistent.

E.g. for thinlto_codegen_set_final_cache_size_relative_to_available_space,
the following changes has to be made:

(1) across build -> across builds
(2) An unspecified default value will be applied -> Default is 75% (see comment in CachePruning.h)

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

152 ↗

(On Diff #127177)

Please mention one more time here that negative values effectively disable pruning

[bd1976llvm]

include/llvm-c/lto.h

786 ↗

(On Diff #127177)

I can't change the unspecified default comment as this is part of the api i.e. these defaults might change in the future.

I will mention the "effectively disabling" in another review as this patch has already been accepted.

787 ↗

(On Diff #127177)

Great - those new cache policies will be useful to customers.

795 ↗

(On Diff #127177)

I can't change the unspecified default comment as this is part of the api i.e. these defaults might change in the future.

I will add the tidyups in another review as this patch has already been accepted.

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

152 ↗

(On Diff #127177)

I will mention the "effectively disabling" in another review as this patch has already been accepted.

[mehdi_amini]

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

152 ↗

(On Diff #127091)

Ah! Thanks :)

[bd1976llvm]

I have reworked this now, see: https://reviews.llvm.org/D41497.

llvm/trunk/include/llvm-c/lto.h

extern void thinlto_codegen_add_cross_referenced_symbol(thinlto_code_gen_t cg,
                                                         int length);
 
 /**
  * @} // endgoup LLVMCTLTO
  * @defgroup LLVMCTLTO_CACHING ThinLTO Cache Control
  * @ingroup LLVMCTLTO
  *
  * These entry points control the ThinLTO cache. The cache is intended to
- * support incremental build, and thus needs to be persistent accross build.
- * The client enabled the cache by supplying a path to an existing directory.
+ * support incremental builds, and thus needs to be persistent across builds.
+ * The client enables the cache by supplying a path to an existing directory.
  * The code generator will use this to store objects files that may be reused
  * during a subsequent build.
  * To avoid filling the disk space, a few knobs are provided:
- *  - The pruning interval limit the frequency at which the garbage collector
- *    will try to scan the cache directory to prune it from expired entries.
- *    Setting to -1 disable the pruning (default).
+ *  - The pruning interval limits the frequency at which the garbage collector
+ *    will try to scan the cache directory to prune expired entries.
+ *    Setting to a negative number applies the maximum interval.
  *  - The pruning expiration time indicates to the garbage collector how old an
  *    entry needs to be to be removed.
- *  - Finally, the garbage collector can be instructed to prune the cache till
+ *  - Finally, the garbage collector can be instructed to prune the cache until
  *    the occupied space goes below a threshold.
  * @{
  */
 
 /**
  * Sets the path to a directory to use as a cache storage for incremental build.
  * Setting this activates caching.
  *
  * \since LTO_API_VERSION=18
  */
 extern void thinlto_codegen_set_cache_dir(thinlto_code_gen_t cg,
                                           const char *cache_dir);
 
 /**
- * Sets the cache pruning interval (in seconds). A negative value disable the
- * pruning. An unspecified default value will be applied, and a value of 0 will
- * be ignored.
+ * Sets the cache pruning interval (in seconds). A negative value sets the
+ * maximum possible pruning interval. An unspecified default value will be
+ * applied, and a value of 0 will be ignored.
  *
  * \since LTO_API_VERSION=18
  */
 extern void thinlto_codegen_set_cache_pruning_interval(thinlto_code_gen_t cg,
                                                        int interval);
 
 /**
  * Sets the maximum cache size that can be persistent across build, in terms of

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

struct CachingOptions {
     std::string Path;                    // Path to the cache, empty to disable.
     CachePruningPolicy Policy;
   };
 
   /// Provide a path to a directory where to store the cached files for
   /// incremental build.
   void setCacheDir(std::string Path) { CacheOptions.Path = std::move(Path); }
 
-  /// Cache policy: interval (seconds) between two prune of the cache. Set to a
-  /// negative value (default) to disable pruning. A value of 0 will be ignored.
+  /// Cache policy: interval (seconds) between two prunes of the cache. A
+  /// negative value sets the maximum possible pruning interval. A value
+  /// of 0 will be ignored.
   void setCachePruningInterval(int Interval) {
     static_assert(std::is_same<decltype(CacheOptions.Policy.Interval),
                                std::chrono::seconds>::value,
                   "ensure same types to avoid risk of overflow");
     if (Interval)
       CacheOptions.Policy.Interval = Interval > 0 ? std::chrono::seconds(Interval)
                                                   : std::chrono::seconds::max();
   }