DTLTO Cache optimization - rebased#203971
Conversation
llvmorg-github-actions
Bot
added
lld
lld:ELF
lld:COFF
platform:windows
LTO
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-elf Author: Katya Romanova (romanova-ekaterina) ChangesI created this new PR for DTLTO cache optimization and I will abandoning/closing old PR for that (#193281). After a major rewrite of DTLTO functionality, it was easier to start from scratch than to merge. Here is what had changed: With this optimization, after the native object file is saved on the disk, it's being renamed into a cache file. Full diff: https://github.com/llvm/llvm-project/pull/203971.diff 7 Files Affected:
diff --git a/lld/COFF/LTO.cpp b/lld/COFF/LTO.cpp
index 0329f6c2e9cea..4bfd62e7f7e2f 100644
--- a/lld/COFF/LTO.cpp
+++ b/lld/COFF/LTO.cpp
@@ -206,7 +206,8 @@ std::vector<InputFile *> BitcodeCompiler::compile() {
FileCache cache;
if (!ctx.config.ltoCache.empty())
cache = check(localCache("ThinLTO", "Thin", ctx.config.ltoCache,
- createAddBufferFn(files, file_names)));
+ createAddBufferFn(files, file_names),
+ !ctx.config.dtltoDistributor.empty()));
checkError(ltoObj->run(
[&](size_t task, const Twine &moduleName) {
diff --git a/lld/ELF/LTO.cpp b/lld/ELF/LTO.cpp
index e40575bffec62..4ec301890c7c8 100644
--- a/lld/ELF/LTO.cpp
+++ b/lld/ELF/LTO.cpp
@@ -336,7 +336,8 @@ SmallVector<std::unique_ptr<InputFile>, 0> BitcodeCompiler::compile() {
FileCache cache;
if (!ctx.arg.thinLTOCacheDir.empty())
cache = check(localCache("ThinLTO", "Thin", ctx.arg.thinLTOCacheDir,
- createAddBufferFn(files, filenames)));
+ createAddBufferFn(files, filenames),
+ !ctx.arg.dtltoDistributor.empty()));
if (!ctx.bitcodeFiles.empty())
checkError(ctx.e, ltoObj->run(
diff --git a/llvm/include/llvm/Support/Caching.h b/llvm/include/llvm/Support/Caching.h
index cebf071b0188f..4f3852bd310f7 100644
--- a/llvm/include/llvm/Support/Caching.h
+++ b/llvm/include/llvm/Support/Caching.h
@@ -20,6 +20,11 @@
#include "llvm/Support/MemoryBuffer.h"
namespace llvm {
+/// This type defines the callback to add a pre-existing file (e.g. in a cache).
+///
+/// Buffer callbacks must be thread safe.
+using AddBufferFn = std::function<void(unsigned Task, const Twine &ModuleName,
+ std::unique_ptr<MemoryBuffer> MB)>;
/// This class wraps an output stream for a file. Most clients should just be
/// able to return an instance of this base class from the stream callback, but
@@ -50,6 +55,7 @@ class CachedFileStream {
if (!Committed)
report_fatal_error("CachedFileStream was not committed.\n");
}
+ virtual AddBufferFn GetAddBuffer() { return AddBufferFn(); }
};
/// This type defines the callback to add a file that is generated on the fly.
@@ -101,12 +107,6 @@ struct FileCache {
std::string CacheDirectoryPath;
};
-/// This type defines the callback to add a pre-existing file (e.g. in a cache).
-///
-/// Buffer callbacks must be thread safe.
-using AddBufferFn = std::function<void(unsigned Task, const Twine &ModuleName,
- std::unique_ptr<MemoryBuffer> MB)>;
-
/// Create a local file system cache which uses the given cache name, temporary
/// file prefix, cache directory and file callback. This function does not
/// immediately create the cache directory if it does not yet exist; this is
@@ -117,7 +117,8 @@ LLVM_ABI Expected<FileCache> localCache(
const Twine &CacheNameRef, const Twine &TempFilePrefixRef,
const Twine &CacheDirectoryPathRef,
AddBufferFn AddBuffer = [](size_t Task, const Twine &ModuleName,
- std::unique_ptr<MemoryBuffer> MB) {});
+ std::unique_ptr<MemoryBuffer> MB) {},
+ bool CacheFileRename = false);
} // namespace llvm
#endif
diff --git a/llvm/lib/DTLTO/DTLTO.cpp b/llvm/lib/DTLTO/DTLTO.cpp
index d1917eb8d954a..3ff28b42bc7b0 100644
--- a/llvm/lib/DTLTO/DTLTO.cpp
+++ b/llvm/lib/DTLTO/DTLTO.cpp
@@ -284,9 +284,18 @@ Error lto::DTLTO::addObjectFilesToLink() {
createStringError(inconvertibleErrorCode(),
"Cannot get a cache file stream: %s",
Job.NativeObjectPath.data()));
- // Store a file buffer into the cache stream.
+
auto &CacheStream = *(CachedFileStreamOrErr->get());
- *(CacheStream.OS) << ObjFileMbRef.getBuffer();
+
+ // Store a file path into the cache stream. This file later will be
+ // renamed into cache file.
+ *(CacheStream.OS) << Job.NativeObjectPath;
+ if (Error Err = CacheStream.commit())
+ return Err;
+
+ AddBufferFn AddBuffer = CacheStream.GetAddBuffer();
+ AddBuffer(Job.Task, Job.ModuleID, std::move(ObjFileMbOrErr.get()));
+
if (Error Err = CacheStream.commit())
return Err;
} else {
diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp
index 40a5c44771b65..a3eb6ecc0077c 100644
--- a/llvm/lib/Support/Caching.cpp
+++ b/llvm/lib/Support/Caching.cpp
@@ -21,6 +21,7 @@
#if !defined(_MSC_VER) && !defined(__MINGW32__)
#include <unistd.h>
#else
+#include "llvm/Support/Windows/WindowsSupport.h"
#include <io.h>
#endif
@@ -29,7 +30,7 @@ using namespace llvm;
Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
const Twine &TempFilePrefixRef,
const Twine &CacheDirectoryPathRef,
- AddBufferFn AddBuffer) {
+ AddBufferFn AddBuffer, bool CacheRename) {
// Create local copies which are safely captured-by-copy in lambdas
SmallString<64> CacheName, TempFilePrefix, CacheDirectoryPath;
@@ -142,6 +143,37 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
AddBuffer(Task, ModuleName, std::move(*MBOrErr));
return Error::success();
}
+ virtual AddBufferFn GetAddBuffer() override { return AddBuffer; }
+ };
+
+ // This class is responsible for renaming/moving existing file into a
+ // cache directory. The path for an input file is passed through a string
+ // stream.
+ struct MoveFileToCache : CachedFileStream {
+ AddBufferFn AddBuffer;
+ size_t Task;
+ SmallString<0> FilePath;
+ MoveFileToCache(AddBufferFn AddBuffer, std::string EntryPath, size_t Task)
+ : CachedFileStream(std::make_unique<raw_svector_ostream>(FilePath),
+ std::move(EntryPath)),
+ AddBuffer(std::move(AddBuffer)), Task(Task) {}
+ ~MoveFileToCache() {
+ // Rename/move native object file into cache directory, if they are
+ // located the same device/logical drive, otherwise we use a copy.
+ std::error_code EC = sys::fs::rename(FilePath, ObjectPathName);
+#ifdef _WIN32
+ if (EC ==
+ std::error_code(ERROR_NOT_SAME_DEVICE, std::system_category()))
+#else
+ if (EC == std::make_error_code(std::errc::cross_device_link))
+#endif
+ EC = sys::fs::copy_file(FilePath, ObjectPathName);
+ if (EC)
+ report_fatal_error(Twine("Failed to rename or copy file ") +
+ FilePath + " to " + ObjectPathName + ": " +
+ EC.message() + "\n");
+ }
+ virtual AddBufferFn GetAddBuffer() override { return AddBuffer; }
};
return [=](size_t Task, const Twine &ModuleName)
@@ -153,6 +185,12 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
return createStringError(EC, Twine("can't create cache directory ") +
CacheDirectoryPath + ": " +
EC.message());
+ // MoveFileToChache class will rename/move the file into the cache on
+ // destruction.
+ if (CacheRename) {
+ return std::make_unique<MoveFileToCache>(
+ AddBuffer, std::string(EntryPath.str()), Task);
+ }
// Write to a temporary to avoid race condition
SmallString<64> TempFilenameModel;
diff --git a/llvm/test/ThinLTO/X86/dtlto/dtlto-cache-rename-optimization.ll b/llvm/test/ThinLTO/X86/dtlto/dtlto-cache-rename-optimization.ll
new file mode 100644
index 0000000000000..932770a5a0909
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/dtlto/dtlto-cache-rename-optimization.ll
@@ -0,0 +1,73 @@
+; DTLTO with llvm-lto2 and -cache-dir, using --save-temps
+; to leave distributor artifacts saved on the disk (except native object files).
+; native object files shouldn't be saved, because they are renamed into
+; cache entries.
+; The test also checks that the cache is populated
+RUN: rm -rf %t && split-file %s %t && cd %t
+
+; Generate bitcode files with summary.
+RUN: opt -thinlto-bc t1.ll -o t1.bc
+RUN: opt -thinlto-bc t2.ll -o t2.bc
+
+; Generate fake object files for mock.py to return.
+RUN: touch t1.o t2.o
+
+; Create an empty subdirectory to avoid having to account for the input files.
+RUN: mkdir %t/out && cd %t/out
+
+DEFINE: %{command} = llvm-lto2 run ../t1.bc ../t2.bc -o t.o -cache-dir cache-dir \
+DEFINE: --save-temps \
+DEFINE: -dtlto-distributor=%python \
+DEFINE: -dtlto-distributor-arg=%llvm_src_root/utils/dtlto/mock.py,../t1.o,../t2.o \
+DEFINE: -r=../t1.bc,t1,px \
+DEFINE: -r=../t2.bc,t2,px
+
+RUN: %{command}
+
+; 12 save-temps outputs plus the cache directory.
+RUN: ls | count 11
+RUN: ls | FileCheck %s --check-prefixes=THINLTO,SAVETEMPS,NOT-NATIVE
+RUN: ls cache-dir/* | count 2
+RUN: ls cache-dir/llvmcache-* | count 2
+
+; llvm-lto2 ThinLTO output files.
+THINLTO-DAG: {{^}}t.o.1{{$}}
+THINLTO-DAG: {{^}}t.o.2{{$}}
+
+; Common -save-temps files from llvm-lto2.
+SAVETEMPS-DAG: {{^}}t.o.resolution.txt{{$}}
+SAVETEMPS-DAG: {{^}}t.o.index.bc{{$}}
+SAVETEMPS-DAG: {{^}}t.o.index.dot{{$}}
+
+; -save-temps incremental files.
+SAVETEMPS-DAG: {{^}}t.o.0.0.preopt.bc{{$}}
+SAVETEMPS-DAG: {{^}}t.o.0.2.internalize.bc{{$}}
+
+; A jobs description JSON.
+SAVETEMPS-DAG: {{^}}t.[[#]].dist-file.json{{$}}
+
+; Summary shards emitted for DTLTO.
+SAVETEMPS-DAG: {{^}}t1.1.[[#]].native.o.thinlto.bc{{$}}
+SAVETEMPS-DAG: {{^}}t2.2.[[#]].native.o.thinlto.bc{{$}}
+
+; DTLTO native output files (the results of the external backend compilations)
+; should not be saved, because these files are renamed into cache entries
+NOT-NATIVE-NOT: native.o{{$}}
+
+;--- t1.ll
+
+target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @t1() {
+ ret void
+}
+
+;--- t2.ll
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @t2() {
+ ret void
+}
\ No newline at end of file
diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp
index 4bcf04df89ac9..848ba0a5c3289 100644
--- a/llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ b/llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -537,7 +537,8 @@ static int run(int argc, char **argv) {
FileCache Cache;
if (!CacheDir.empty())
- Cache = check(localCache("ThinLTO", "Thin", CacheDir, AddBuffer),
+ Cache = check(localCache("ThinLTO", "Thin", CacheDir, AddBuffer,
+ !DTLTODistributor.empty()),
"failed to create cache");
check(Lto->run(AddStream, Cache), "LTO::run failed");
|
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
I see there are a few DTLTO cache test failures - can you take a look and let me know when this is ready for review? |
|
@teresajohnson: All tests had passed. |
| @@ -117,7 +117,8 @@ LLVM_ABI Expected<FileCache> localCache( | |||
| const Twine &CacheNameRef, const Twine &TempFilePrefixRef, | |||
| const Twine &CacheDirectoryPathRef, | |||
| AddBufferFn AddBuffer = [](size_t Task, const Twine &ModuleName, | |||
| std::unique_ptr<MemoryBuffer> MB) {}); | |||
| std::unique_ptr<MemoryBuffer> MB) {}, | |||
| bool CacheFileRename = false); | |||
There was a problem hiding this comment.
Document usage in the comment
|
|
||
| // Store a file path into the cache stream. This file later will be | ||
| // renamed into cache file. | ||
| *(CacheStream.OS) << Job.NativeObjectPath; |
There was a problem hiding this comment.
The handling here and below with the direct AddBuffer callback is non-intuitive, and feels like it is contorting the existing interface in unnecessary ways.
Rather than adding the accessor to get AddBuffer, and using the OS to write the path, I think it would be cleaner and clearer to just make a dedicated method in your new MoveFileToCache class that takes the NativeObjectPath and ObjFileMbOrErr (and anything else that can't be saved in the new class via the constructor), and do the rename and call to AddBuffer there. You probably don't need to wait until commit to do the rename? Then you could simplify it (or just remove the override and fall back to the base class commit).
There was a problem hiding this comment.
Thanks @teresajohnson. For this optimisation, I think it would be useful to add functionality to the local file cache (llvm/lib/Support/Caching.cpp) to consume an existing on-disk file directly into the cache, instead of always reopening it and streaming its contents through a cache writer. This would involve modifying the Support library, but I think that is appropriate here because the functionality is generally useful, even if DTLTO would be the only user initially.
There was a problem hiding this comment.
I agree that the approach that Ben proposed is simpler, the only problem is that it won't fit the existing pattern/workflow of cache usage for other scenarios; because of that I prefer to implement what Teresa proposed. However, I don't have a strong opinion about it. I will be happy to implement what Ben suggested if Teresa (or other developers) think it's better.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
lgtm but 2 suggestions below
| } | ||
|
|
||
| virtual Error commit(std::unique_ptr<MemoryBuffer> MemBuf) { | ||
| return Error::success(); |
There was a problem hiding this comment.
This should probably have the same error if Committed is already set? You could just invoke CachedFileStream::commit() to avoid code duplication.
There was a problem hiding this comment.
ping on this suggestion.
|
|
||
| virtual Error commit() override { | ||
| virtual Error commit(std::unique_ptr<MemoryBuffer> MemBuf) override { | ||
| if (Committed) |
There was a problem hiding this comment.
Like CacheStream::commit() you could do the checking and setting of Committed via a call to CachedFileStream::commit().
|
A brand-new test has been added 4 days ago with the commit fe5f49f, and it failed on my PR but only on Linux and AArch64 (but strangely enough not on Windows). I will set up Linux environment today and have a look why this test is failing. I will also make changes that Teresa suggested. Thank you for reviewing! |
8d3732b to
470c1f7
Compare
|
Teresa, with the latest commit, I have only updated the failing Linux test that has been added shortly after I posted a PR. I will be away on vacation in the remote wilderness with zero internet access, but I will address your comment as soon as I return. |
470c1f7 to
bb07d97
Compare
|
@teresajohnson, is it OK to commit? I have already received a LGTM from you with a request to correct one minor thing (which I did), but after rebasing, I had to correct some of the newly written tests as well, since there were some failures. I would feel safer, if the latest commits will get reviewed too. Thank you! |
| if (!Committed) | ||
| report_fatal_error("CachedFileStream was not committed.\n"); | ||
| } | ||
| virtual AddBufferFn GetAddBuffer() { return AddBufferFn(); } |
There was a problem hiding this comment.
I think this function and its override and AddBufferFn are no longer used (leftover from the prior approach?) and can be removed.
| } | ||
|
|
||
| virtual Error commit(std::unique_ptr<MemoryBuffer> MemBuf) { | ||
| return Error::success(); |
There was a problem hiding this comment.
ping on this suggestion.
|
@romanova-ekaterina and @teresajohnson I think this is a candidate for cherry-picking to the release branch. |
…fail after my commit
f2e7438 to
c362bdb
Compare
3 participants
I created this new PR for DTLTO cache optimization and I will abandoning/closing old PR for that (#193281). After a major rewrite of DTLTO functionality that happened recently (#192629), it was easier to start from scratch than to merge.
Here is what had changed:
Currently, DTLTO cache works this way: when native object file is generated and sent back by the remote compilation, it's being saved on the disk, then the content of this file is read and written into a buffer and subsequently it's read from the buffer and written into the cache file. This is obviously non-efficient.
With this optimization, after the native object file is saved on the disk, it's being renamed into a cache file.