[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, I like the new version with the checking centralized in pruneCache. A few suggestions below.



================
Comment at: lld/test/ELF/lto/cache-warnings.ll:29
+; RUN: cat %t.size1.txt \
+; RUN: | %python -c "import subprocess, sys; size = int(sys.stdin.read()); cache_dir=r'%t.cache'; subprocess.run([r'ld.lld', '--verbose', '--thinlto-cache-dir={}'.format(cache_dir), '--thinlto-cache-policy=prune_interval=0s:cache_size_bytes={}'.format(size), r'%t2.o', r'%t.o', '-o', r'%t3'], stderr=subprocess.STDOUT)" | FileCheck %s --implicit-check-not=warning:
+
----------------
I'm not sure invoking lld via python subprocess is ideal for testing. I think this could be done much more simply via shell substitution via $() (you have to REQUIRE shell though). Even if you need python to compute the total file sizes initially you could then do something like

RUN: ld.lld .... --thinlto-cache-policy=prune_interval=0s:cache_size_bytes=$(($(cat size.txt) + 5)) ...

I know I have seen $(cat somefile.txt) used before, so I assume the evaluation described above should also work - can you give it a try to simplify a lot of this?


================
Comment at: llvm/include/llvm/Support/CachePruning.h:83
+bool pruneCache(StringRef Path, CachePruningPolicy Policy,
+                const std::vector<std::unique_ptr<MemoryBuffer>> &files = {});
 } // namespace llvm
----------------
If all clients are passing in files with your patch, probably just go ahead and remove the default parameter.


================
Comment at: llvm/lib/Support/CachePruning.cpp:269
+  if (Policy.MaxSizeFiles && (files.size() > Policy.MaxSizeFiles))
+    llvm::errs() << "warning: ThinLTO cache pruning happens since the number "
+                    "of created files exceeds the maximum number of files; "
----------------
Probably using the warning interface is better than errs(). You could probably pass in the ErrorHandler if needed.

Also, @MaskRay had an earlier suggestion about the format of the message that I think would be good to incorporate (including the number of files in the limit and the actual number of files that caused the limit to be exceeded): https://reviews.llvm.org/D135590#inline-1308533


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

https://reviews.llvm.org/D135590