[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
llvm-commits at lists.llvm.org
Wed Oct 19 10:51:01 PDT 2022
MaggieYi added a comment.
Thanks Teresa and Fangrui for the comments. Sorry for the delay in updating the patch. I have been trying to find a way to get the warning invoked by all cache users.
As mentioned above, I cannot incorporate the handling into pruneCache.
I managed to get the total cache file sizes, which come out from the current link, by modifying the llvm/lib/LTO/LTO.cpp file.
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index dc28b681a151..a24f3149bead 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -74,6 +74,10 @@ cl::opt<bool> EnableLTOInternalization(
"enable-lto-internalization", cl::init(true), cl::Hidden,
cl::desc("Enable global value internalization in LTO"));
+/// The required cache size for the current link job. If thinLTOCacheDir is
+/// not specified, it is disabled by default.
+static uint64_t RequireCacheSize = 0;
+
// Computes a unique hash for the Module considering the current list of
// export/import and other global analysis results.
// The hash is produced in \p Key.
@@ -1292,16 +1296,32 @@ public:
return RunThinBackend(AddStream);
SmallString<40> Key;
+ uint64_t CachedFileSize = 0;
// The module may be cached, this helps handling it.
computeLTOCacheKey(Key, Conf, CombinedIndex, ModuleID, ImportList,
ExportList, ResolvedODR, DefinedGlobals, CfiFunctionDefs,
CfiFunctionDecls);
- Expected<AddStreamFn> CacheAddStreamOrErr = Cache(Task, Key);
+ Expected<AddStreamFn> CacheAddStreamOrErr = Cache(Task, Key, &CachedFileSize);
if (Error Err = CacheAddStreamOrErr.takeError())
return Err;
AddStreamFn &CacheAddStream = *CacheAddStreamOrErr;
- if (CacheAddStream)
- return RunThinBackend(CacheAddStream);
+ if (CacheAddStream) {
+ auto Result = RunThinBackend(CacheAddStream);
+ // Get the file size of new created cache file.
+ Expected<std::unique_ptr<CachedFileStream>> StreamOrErr =
+ CacheAddStream(Task);
+ if (Error Err = StreamOrErr.takeError())
+ report_fatal_error(std::move(Err));
+ std::unique_ptr<CachedFileStream> &Stream = *StreamOrErr;
+ auto FileName = Stream->ObjectPathName;
+ std::error_code EC = llvm::sys::fs::file_size(FileName, CachedFileSize);
+ if (EC)
+ report_fatal_error(Twine("Unable to obtain file size for a file ") +
+ FileName + ": " + EC.message() + "\n");
+ RequireCacheSize += CachedFileSize;
+ return Result;
+ }
+ RequireCacheSize += CachedFileSize;
return Error::success();
}
I also have to change FileCache by adding a new pointer type parameter CachedFileSize:
diff --git a/llvm/include/llvm/Support/Caching.h b/llvm/include/llvm/Support/Caching.h
index bef23ae757f2..017bfd232afb 100644
--- a/llvm/include/llvm/Support/Caching.h
+++ b/llvm/include/llvm/Support/Caching.h
@@ -52,8 +52,8 @@ using AddStreamFn =
///
/// if (AddStreamFn AddStream = Cache(Task, Key))
/// ProduceContent(AddStream);
-using FileCache =
- std::function<Expected<AddStreamFn>(unsigned Task, StringRef Key)>;
+using FileCache = std::function<Expected<AddStreamFn>(
+ unsigned Task, StringRef Key, uint64_t *CachedFileSize)>;
diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp
index d6902f660e39..6b9f7bd4f1d0 100644
--- a/llvm/lib/Support/Caching.cpp
+++ b/llvm/lib/Support/Caching.cpp
@@ -37,7 +37,8 @@ Expected<FileCache> llvm::localCache(Twine CacheNameRef,
TempFilePrefixRef.toVector(TempFilePrefix);
CacheDirectoryPathRef.toVector(CacheDirectoryPath);
- return [=](unsigned Task, StringRef Key) -> Expected<AddStreamFn> {
+ return [=](unsigned Task, StringRef Key,
+ uint64_t *CachedFileSize = nullptr) -> Expected<AddStreamFn> {
// This choice of file name allows the cache to be pruned (see pruneCache()
// in include/llvm/Support/CachePruning.h).
SmallString<64> EntryPath;
@@ -48,6 +49,12 @@ Expected<FileCache> llvm::localCache(Twine CacheNameRef,
Twine(EntryPath), sys::fs::OF_UpdateAtime, &ResultPath);
std::error_code EC;
if (FDOrErr) {
+ if (CachedFileSize) {
+ sys::fs::file_status Status;
+ EC = sys::fs::status(*FDOrErr, Status);
+ if (!EC)
+ *CachedFileSize = Status.getSize();
+ }
ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
MemoryBuffer::getOpenFile(*FDOrErr, EntryPath,
/*FileSize=*/-1,
If the developers use FileCache but don’t need to get the cached file sizes, they need to pass the nullptr, e.g.
Expected<AddStreamFn> CacheAddStreamOrErr = Cache(Task, UniqueKey, nullptr);
I am not sure whether this method is acceptable?
Could you please let me know your thoughts on this?
Thanks,
================
Comment at: lld/ELF/LTO.cpp:204
+ if (Policy.MaxSizeFiles && (files.size() > Policy.MaxSizeFiles))
+ WithColor::warning()
+ << "ThinLTO cache pruning happens since the total number of the files "
----------------
MaskRay wrote:
> `message(`
>
> Drop trailing `.`. Please follow the style of other messages and diagnostics.
>
> Be terse in some wording; suggest `the number of created files (number) exceeds the maximum ... (number)`
Thanks, fixed it by using 'warn()' function and modified the message information.
================
Comment at: lld/ELF/LTO.cpp:378
+ if (!config->thinLTOCacheDir.empty())
+ checkCacheCapacity(files, config->thinLTOCachePolicy);
+
----------------
tejohnson wrote:
> 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.
Thanks.
1. Just emit a warning the first time a file is pruned and add a test for it.
2. I am not able to put this handling into pruneCache since pruneCache performs pruning for all cache files in the given cache directory. In this patch, we add up the sizes of the cache files that come out of the current link job. The warning is only given if cache_size_bytes or cache_size_files is too small for the total sizes/number of cache files that come out of the current link job. In the pruneCache function, there is not any information about cache files which were used by the current link job. Therefore, I cannot use pruneCache.
================
Comment at: lld/test/ELF/lto/cache-warnings.ll:2
+; REQUIRES: x86
+; NetBSD: noatime mounts currently inhibit 'touch' from updating atime
+; UNSUPPORTED: system-netbsd
----------------
MaskRay wrote:
> Non-RUN non-CHECK comments use `;;` for new tests.
Thanks, fixed.
================
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.
----------------
tejohnson wrote:
> 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.
Thanks, the test is fixed following your suggestion.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135590/new/
https://reviews.llvm.org/D135590
More information about the llvm-commits
mailing list