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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision as: MaskRay.
MaskRay added inline comments.


================
Comment at: lld/test/COFF/lto-cache-warnings.ll:1
+; REQUIRES: x86, shell
+
----------------
Does any feature require `shell`? I feel that every RUN line is normal and can be interpreted by lit, so `shell` is likely unneeded. But you can check this by running the test on a Windows machine which I assume you have access to:)


================
Comment at: lld/test/COFF/lto-cache-warnings.ll:6
+
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+
----------------
Nit: `-r` is more common than `-R`.

Note, if you do `; RUN: rm -rf %t && mkdir %t && cd %t`, you can drop every occurrence `%t` below and just use an arbitrary word as the directory name, and you can avoid the slightly strange `%t.cache` reference in `%python -c "import os, sys; os.chdir(r\"%t.cache\");`


================
Comment at: lld/test/COFF/lto-cache-warnings.ll:23
+; RUN: lld-link /lldltocache:%t.cache /lldltocachepolicy:prune_interval=0s:cache_size_bytes=32k /out:%t3 /entry:main %t2.o %t.o 2>&1
+; RUN: %python -c "import os, sys; os.chdir(r\"%t.cache\"); print(sum(os.path.getsize(filename) for filename in os.listdir('.') if os.path.isfile(filename) and filename.startswith(\"llvmcache-\")))" > %t.size.txt 2>&1
+
----------------
Use single quotes to avoid some escapes.


================
Comment at: llvm/lib/Support/CachePruning.cpp:263
 
+  // files.size() is greater the number of inputs  by one. However, a timestamp
+  // file (named llvmcache.timestamp) is created and stored in the cache
----------------
`files.size() is greater than the number of inputs by one`


================
Comment at: llvm/lib/Support/CachePruning.cpp:264
+  // files.size() is greater the number of inputs  by one. However, a timestamp
+  // file (named llvmcache.timestamp) is created and stored in the cache
+  // directory if --thinlto-cache-policy option is used. llvmcache.timestamp is
----------------
`llvmcache.timestamp` was just mentioned above, so repeating it here seems verbose (and if we decide to rename, there is one comment needing to update).


================
Comment at: llvm/lib/Support/CachePruning.cpp:267
+  // used to prune the old cache file. The cache pruning will happen if (the
+  // number of inputs+1) is greater than Policy.MaxSizeFiles. Therefore, I used
+  // files.size() here.
----------------
Technically it may not be `+1`. It's `+ config->ltoPartitions`  (see `config->ltoPartitions = args::getInteger(args, OPT_lto_partitions, 1);` and `ParallelCodeGenParallelismLevel`). I suggest that you avoid being so specific in this sentence.


================
Comment at: llvm/lib/Support/CachePruning.cpp:269
+  // files.size() here.
+  size_t ActualNums = Files.size();
+  if (Policy.MaxSizeFiles && (ActualNums > Policy.MaxSizeFiles))
----------------
`const size_t`

Drop a pair of parentheses below: `if (Policy.MaxSizeFiles && ActualNums > Policy.MaxSizeFiles)`


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

https://reviews.llvm.org/D135590