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

[Ying Yi via Phabricator via llvm-commits]

MaggieYi added a comment.

@MaskRay, please let me know if there are any further changes required. Thanks.



================
Comment at: lld/test/COFF/lto-cache-warnings.ll:1
+; REQUIRES: x86, shell
+
----------------
MaskRay wrote:
> 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:)
As mentioned in the Teresa's comment, `cache_size_bytes=$(($(cat %t.size.txt)+5))` requires shell. I have checked the test on a Windows machine and it failed in the RUN command which contains `cache_size_bytes=$(($(cat %t.size.txt)+5))`.


================
Comment at: lld/test/COFF/lto-cache-warnings.ll:6
+
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+
----------------
MaskRay wrote:
> 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\");`
Thanks, fixed! For Macho test, I used `;RUN: cd %t`, since the .o files exist in the %t folder as well.


================
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
+
----------------
MaskRay wrote:
> Use single quotes to avoid some escapes.
Fixed, thanks.


================
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
----------------
MaskRay wrote:
> `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).
Thanks, I have removed the timestamp file name.


================
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.
----------------
MaskRay wrote:
> 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.
Thanks for pointing this out, I have modified the sentence.


================
Comment at: llvm/lib/Support/CachePruning.cpp:269
+  // files.size() here.
+  size_t ActualNums = Files.size();
+  if (Policy.MaxSizeFiles && (ActualNums > Policy.MaxSizeFiles))
----------------
MaskRay wrote:
> `const size_t`
> 
> Drop a pair of parentheses below: `if (Policy.MaxSizeFiles && ActualNums > Policy.MaxSizeFiles)`
Fixed, thanks.


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

https://reviews.llvm.org/D135590