[PATCH] D135590: [ThinLTO][ELF] Add a ThinLTO warning if cache_size_bytes or cache_size_files is too small for the current link job.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks! Couple suggestions below.



================
Comment at: lld/ELF/LTO.cpp:378
+  if (!config->thinLTOCacheDir.empty())
+    checkCacheCapacity(files, config->thinLTOCachePolicy);
+
----------------
Can this handling be incorporated into pruneCache which is called a bit further below? The advantage of that is that it gets invoked by all cache users, which is more than just lld/ELF (i.e. the other lld formats, the old LTO API, and looks like lldb uses this as well). You could maybe just emit the warning the first time a file is pruned due to either of the size limits.


================
Comment at: lld/test/ELF/lto/cache-warnings.ll:26
+; Case 2: If the total size of the cache files created by the current link job is less than the maximum size for the cache directory in bytes, there is no warning.
+; RUN: ld.lld -v --thinlto-cache-dir=%t.cache --thinlto-cache-policy=prune_interval=0s:cache_size_bytes=1355 %t2.o %t.o -o %t3 2>&1 | FileCheck %s --implicit-check-not=warning
+; Case 3: If the total size of the cache files created by the current link job exceeds the maximum size for the cache directory in bytes, a warning is given.
----------------
I'm concerned that this test will end up being brittle and start failing as the compiler evolves to change the IR and make the bitcode larger or smaller. I think you can use command substitution via $() in the RUN line to perhaps compute the size of the files and +/- 5 from the total.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135590/new/

https://reviews.llvm.org/D135590