[ThinLTO]Allow setting of maximum cache size with 64-bit number, and provide C-interface function for large values

[jhenderson]

Summary

The maximum cache size in terms of bytes is a 64-bit number. However, the methods to set it only take unsigned, which means that the maximum cache size cannot be specified above 4GB. That's quite small compared to the output of some projects, so it makes sense to provide the ability to set larger values in that field.

We also need a C-interface function that provides a greater range than the existing thinlto_codegen_set_cache_size_bytes, which also only takes an unsigned, so this change also adds thinlto_codegen_set_cache_size_megabytes.

[tejohnson]

include/llvm-c/lto.h

828 ↗

(On Diff #165245)

Shouldn't this be changed to uint64_t? I don't think that would break any existing users since it is wider.

[jhenderson]

include/llvm-c/lto.h

828 ↗

(On Diff #165245)

That's what I originally thought (and I would prefer to do it if possible) but I think there are two issues, which may both be irrelevant:

1. Is uint64_t definitely available in all versions of C that this header is intended to be compatible with? I don't know what the compatibility guarantees are.
2. In the past, my team have occasionally updated the LTO library (as a DLL) without updating the header in our linker's source tree. If somebody were to do this in the future, (from one with the old, smaller size, to one with the newer size), I think this would result in unexpected behaviour (because the caller thinks it's calling an unsigned version, but the function called actually takes a uint64_t). In our case, that won't happen, since I'll be updating our local header once we integrate these changes in our downstream port, but I wasn't sure if this is something that might affect other users. I couldn't find any documentation explaining the compatibility guarantees of the C interface.

[tejohnson]

include/llvm-c/lto.h

828 ↗

(On Diff #165245)

Those are good questions and I don't know the answers as I don't have a lot of familiarity with the C interface. Hopefully someone else knows the compatibility guarantees there.

[mehdi_amini]

include/llvm-c/lto.h

828 ↗

(On Diff #165245)

IIRC: it is expected to preserve ABI, or when impossible, bump the version number (the linker is expected then to check it at runtime and adjust its behavior)

[steven_wu]

LGTM

include/llvm-c/lto.h

828 ↗

(On Diff #165245)

I agree it needs to be ABI compatible and I can see changing the width of that value can cause problems, though I don't think change the cache size is going to break the build.

I would prefer the current approach, which is to add a new API. unsigned megabyte is not quite as large is uint64_t but I guess it is big enough.

[jhenderson]

include/llvm-c/lto.h

828 ↗

(On Diff #165245)

Thanks for the comments. I'm going to go ahead and land this as is.

I would prefer the current approach, which is to add a new API. unsigned megabyte is not quite as large is uint64_t but I guess it is big enough.

One alternative would be a function thinlto_codegen_set_cache_size_bytes_ull or similar, but I think this should be big enough for now.

[jhenderson]

tools/llvm-lto/llvm-lto.cpp

161 ↗

(On Diff #165245)

https://bugs.llvm.org/show_bug.cgi?id=19665 has meant this doesn't work on my Linux machine, so I'm changing it to unsigned long long.

[jhenderson]

I've had to back out rL342233, because the LLD cache test started failing intermittently, and I don't know why (it works for me locally). Would somebody mind trying to apply this patch and see if they get a failure?

[jhenderson]

In D52023#1235082, @jhenderson wrote:

I've had to back out rL342233, because the LLD cache test started failing intermittently, and I don't know why (it works for me locally). Would somebody mind trying to apply this patch and see if they get a failure?

Looks like the test failure was just a flaky test, so I've relanded this as rL342366.

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

 
 /**
  * @defgroup LLVMCLTO LTO
  * @ingroup LLVMC
  *
  * @{
  */
 
-#define LTO_API_VERSION 22
+#define LTO_API_VERSION 23
 
 /**
  * \since prior to LTO_API_VERSION=3
  */
 typedef enum {
     LTO_SYMBOL_ALIGNMENT_MASK              = 0x0000001F, /* log2 of alignment */
     LTO_SYMBOL_PERMISSIONS_MASK            = 0x000000E0,
     LTO_SYMBOL_PERMISSIONS_CODE            = 0x000000A0,

  * will be ignored.
  *
  * \since LTO_API_VERSION=22
  */
 extern void thinlto_codegen_set_cache_size_bytes(thinlto_code_gen_t cg,
                                                  unsigned max_size_bytes);
 
 /**
+ * Same as thinlto_codegen_set_cache_size_bytes, except the maximum size is in
+ * megabytes (2^20 bytes).
+ *
+ * \since LTO_API_VERSION=23
+ */
+extern void
+thinlto_codegen_set_cache_size_megabytes(thinlto_code_gen_t cg,
+                                         unsigned max_size_megabytes);
+
+/**
  * Sets the maximum number of files in the cache directory. An unspecified
  * default value will be applied. A value of 0 will be ignored.
  *
  * \since LTO_API_VERSION=22
  */
 extern void thinlto_codegen_set_cache_size_files(thinlto_code_gen_t cg,
                                                  unsigned max_size_files);
 

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

public:
   void setMaxCacheSizeRelativeToAvailableSpace(unsigned Percentage) {
     if (Percentage)
       CacheOptions.Policy.MaxSizePercentageOfAvailableSpace = Percentage;
   }
 
   /// Cache policy: the maximum size for the cache directory in bytes. A value
   /// 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) {
+  void setCacheMaxSizeBytes(uint64_t MaxSizeBytes) {
     if (MaxSizeBytes)
       CacheOptions.Policy.MaxSizeBytes = MaxSizeBytes;
   }
 
   /// Cache policy: the maximum number of files in the cache directory. A value
   /// of 0 will be ignored.
   void setCacheMaxSizeFiles(unsigned MaxSizeFiles) {
     if (MaxSizeFiles)

llvm/trunk/test/ThinLTO/X86/cache.ll

 ; RUN: %python -c "with open(r'%t.cache/llvmcache-foo-76', 'w') as file: file.truncate(76)"
 ; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-max-size-bytes 78847
 ; RUN: ls %t.cache/llvmcache-foo-8
 ; RUN: ls %t.cache/llvmcache-foo-76
 ; RUN: not ls %t.cache/llvmcache-foo-16
 ; RUN: not ls %t.cache/llvmcache-foo-100k
 ; RUN: not ls %t.cache/llvmcache-foo-77k
 
+; Verify that specifying a max size > 4GB for the cache directory does not
+; prematurely prune, due to an integer overflow.
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: %python -c "with open(r'%t.cache/llvmcache-foo-10', 'w') as file: file.truncate(10)"
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-max-size-bytes 4294967297
+; RUN: ls %t.cache/llvmcache-foo-10
+
+; Verify that negative numbers aren't accepted for the
+; --thinlto-cache-max-size-bytes switch
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: not llvm-lto %t.bc --thinlto-cache-max-size-bytes -1 2>&1 | FileCheck %s
+; CHECK: -thinlto-cache-max-size-bytes option: '-1' value invalid
+
 ; Verify that specifying max number of files in the cache directory prunes
 ; it to this amount, removing the oldest files first.
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; Create cache files with different sizes.
 ; Only 75B and 76B files should stay after pruning.
 ; RUN: %python -c "print(' ' * 1023)" > %t.cache/llvmcache-foo-1023
 ; RUN: touch -t 198002011200 %t.cache/llvmcache-foo-1023
 ; RUN: %python -c "print(' ' * 15)" > %t.cache/llvmcache-foo-15

llvm/trunk/tools/llvm-lto/llvm-lto.cpp

 
 static cl::opt<std::string>
     ThinLTOCacheDir("thinlto-cache-dir", cl::desc("Enable ThinLTO caching."));
 
 static cl::opt<int>
     ThinLTOCachePruningInterval("thinlto-cache-pruning-interval",
     cl::init(1200), cl::desc("Set ThinLTO cache pruning interval."));
 
-static cl::opt<int>
+static cl::opt<unsigned long long>
     ThinLTOCacheMaxSizeBytes("thinlto-cache-max-size-bytes",
     cl::desc("Set ThinLTO cache pruning directory maximum size in bytes."));
 
 static cl::opt<int>
     ThinLTOCacheMaxSizeFiles("thinlto-cache-max-size-files", cl::init(1000000),
     cl::desc("Set ThinLTO cache pruning directory maximum number of files."));
 
 static cl::opt<std::string> ThinLTOSaveTempsPrefix(

llvm/trunk/tools/lto/lto.cpp

void thinlto_codegen_set_final_cache_size_relative_to_available_space(
   return unwrap(cg)->setMaxCacheSizeRelativeToAvailableSpace(Percentage);
 }
 
 void thinlto_codegen_set_cache_size_bytes(
     thinlto_code_gen_t cg, unsigned MaxSizeBytes) {
   return unwrap(cg)->setCacheMaxSizeBytes(MaxSizeBytes);
 }
 
+void thinlto_codegen_set_cache_size_megabytes(
+    thinlto_code_gen_t cg, unsigned MaxSizeMegabytes) {
+  uint64_t MaxSizeBytes = MaxSizeMegabytes;
+  MaxSizeBytes *= 1024 * 1024;
+  return unwrap(cg)->setCacheMaxSizeBytes(MaxSizeBytes);
+}
+
 void thinlto_codegen_set_cache_size_files(
     thinlto_code_gen_t cg, unsigned MaxSizeFiles) {
   return unwrap(cg)->setCacheMaxSizeFiles(MaxSizeFiles);
 }
 
 void thinlto_codegen_set_savetemps_dir(thinlto_code_gen_t cg,
                                        const char *save_temps_dir) {
   return unwrap(cg)->setSaveTempsDir(save_temps_dir);

llvm/trunk/tools/lto/lto.exports

 thinlto_module_get_num_objects
 thinlto_module_get_object
 thinlto_codegen_set_pic_model
 thinlto_codegen_set_cache_dir
 thinlto_codegen_set_cache_pruning_interval
 thinlto_codegen_set_cache_entry_expiration
 thinlto_codegen_set_final_cache_size_relative_to_available_space
 thinlto_codegen_set_cache_size_bytes
+thinlto_codegen_set_cache_size_megabytes
 thinlto_codegen_set_cache_size_files
 thinlto_codegen_set_savetemps_dir
 thinlto_codegen_set_cpu
 thinlto_debug_options
 lto_module_is_thinlto
 thinlto_codegen_add_must_preserve_symbol
 thinlto_codegen_add_cross_referenced_symbol
 thinlto_codegen_set_codegen_only
 thinlto_codegen_disable_codegen
 thinlto_module_get_num_object_files
 thinlto_module_get_object_file
 thinlto_set_generated_objects_dir