Add the ability to plug a cache on the LTO API.
I tried to write such that a linker implementation can
control the cache backend. I tried multiple design before
settling on this one, even if I'm not totally happy about
it. Suggestions welcome if any.
Details
Diff Detail
Event Timeline
include/llvm/LTO/Caching.h | ||
---|---|---|
39 | Is it worth typedef'ing std::function<void(std::unique_ptr<MemoryBuffer>)> such that you don't repeat yourself here? |
Thanks for catching this :)
include/llvm/LTO/Caching.h | ||
---|---|---|
68 | It is for grouping (\defgroup), but here it is a bad copy/paste. I removed all this code. |
I had a bit of trouble following the flow, so I added in some suggestions for more comments and some assertion checking. Also, I think I found a couple of errors, see below. Once you have a fixed patch uploaded I will give it a try with gold-plugin.
include/llvm/LTO/Caching.h | ||
---|---|---|
32 | Is it really optional? It seems like we should always have a TempFilename if we tried to write to the cache via getStream(). Maybe: "Path to temporary file used to buffer output that will be written to a cache entry when this object is destroyed." | |
33 | Needs better description. E.g. "User-supplied callback to write the buffer loaded from cache into the native output object." | |
50 | "return true and set EntryPath on cache hit." | |
54 | Is it valid to create a CacheObjectOutput with an empty CacheDirectoryPath? Perhaps the constructor could assert that it is non-empty, and this could simply return true. | |
lib/LTO/Caching.cpp | ||
31 | Suggest comment like "Return if client did not attempt caching via tryLoadFromCache". | |
33 | I think TempFilename would only be empty here if tryLoadFromCache() returned true (so we didn't need to write a new cache entry). A comment would be helpful. E.g. something like "TempFilename will be non-empty if a new cache entry was created, so save it to EntryPath in the cache." | |
51 | It seems wrong that we don't have a return after this, otherwise in the case where a new cache entry is written AddBuffer() will be called twice. I guess it is simply inefficient but will work - you will load again from EntryPath that was just written, and rewrite to the output file. | |
54 | Add a comment above here that this is the handling when tryLoadFromCache returned true and we can simply load the existing entry from the cache. | |
63 | This would happen if the caching client didn't first call tryLoadFromCache before passing along the created CacheObjectOutput. Seems like it would be clearer to make this assert here, as that seems to be wrong usage of this class. | |
lib/LTO/LTO.cpp | ||
48 | add "and other global analysis results" after "export/import". | |
55 | Add "." at end. | |
88 | This is what the old version was doing, but here we don't have the list of preserved symbols here. Wouldn't this be the ExportedGUIDs set created in runThinLTO? That should presumably be passed down here too and used as in your existing cache key computation in ThinLTOCodeGenerator. | |
92 | This should be hashing GUIDs not linkage. | |
525 | As per offline discussion, move this block under isCachingEnabled() | |
540 | What if caching not enabled? In that case we shouldn't call addOutput above, so we won't have an Output. It should presumably just call addOutput() here then before returning. Add a comment that this returns the cached output if enabled. |
I added a high level description above class NativeObjectOutput and class CacheObjectOutput. Let me know if it helps.
include/llvm/LTO/Caching.h | ||
---|---|---|
50 | EntryPath is unconditionally set, I added this. | |
lib/LTO/Caching.cpp | ||
51 | Right, indeed I didn't intend to callback here, we should delete the temporary and reload the cache entry (mmap) and call the callback. | |
lib/LTO/LTO.cpp | ||
88 | Since internalization is done on the summary now, we don't really need to hash the "preserved symbols". We hash the linkage type from the index instead. I updated the comment to reflect the change in the code. | |
92 | Let me know if the above comment is enough. | |
540 | We need the output in the first place to be able to check if the caching is enabled or not. |
Do the tests pass for you? I got a seg fault in llvm-lto2 in new test case, but didn't get a chance to look into it.
lib/LTO/Caching.cpp | ||
---|---|---|
22 | FileSystem.h |
Oh wrong patch, I thought we were talking about the commons one, I'll retest this one later.
Looked at the llvm-lto2 core dump. The problem is that the Path variable declared in the AddOutput lambda is destroyed by the time it is used in the AddBuffer callback. Presumably it should be passed into the CacheObjectOutput constructor and a copy saved there.
The new comments help a lot. A few minor suggestions. Will add callback to gold-plugin and retry when the coredump resolved.
include/llvm/LTO/Caching.h | ||
---|---|---|
51 | Note that when we return and Output is destroyed, the existing entry pulled from cache and passed back to client via CallBack. | |
57 | Note that here, when Output is destroyed, the cache entry is updated in the cache and CallBack invoked to pass the contents back to the client. | |
72 | or pulls it out of the cache if it was already there, and calls AddBuffer. | |
91 | Remove "!CacheDirectoryPath.empty()" | |
lib/LTO/Caching.cpp | ||
54 | Combine the above 2 comment lines | |
lib/LTO/LTO.cpp | ||
88 | Ok got it. |
Yeah, I think it is a good habit, I really like such overview when I have to read code I don't know about, but I always forget to do the same on my code. Do no hesitate to point it out in future reviews :)
Still needs fix for coredump (see my earlier response this morning on why that happens).
lib/LTO/Caching.cpp | ||
---|---|---|
72 | My gold testing is hitting this assert many times. The reason is that when we call back to AddOutput, we don't know whether this is a regular or thin LTO call, so if there is a CacheDir specified we always create a CacheObjectOutput. If it is regularLTO, then we will eventually call getStream() from codegen without previously calling tryLoadFromCache. This is happening in a couple of instances:
Even with the above two scenarios fixed, we want to support a mixed Thin and Regular LTO compilation model, so it needs to be handled. The easiest way seems to be to add a parameter to the AddOutput callback that takes an indication of which type of LTO this file is. |
lib/LTO/Caching.cpp | ||
---|---|---|
72 | Right, I was supporting this at some point and it got lost after some reviews... I'd like to *not* change the AddOutput callback, and go back to what I was doing before, i.e. either:
This would allow the monolithic LTO compilation to be cached as well in the future. |
lib/LTO/Caching.cpp | ||
---|---|---|
72 | IIRC correctly, what it did before was to return a nullptr when getStream() was called with an EmptyPath, which wouldn't work either.
I assume you mean when EntryPath is empty? In that case CacheObjectOutput::~CacheObjectOutput would also have to handle the empty EntryPath (load from temporary and call AddBuffer but don't cache I guess?).
This would be similar I guess? |
lib/LTO/Caching.cpp | ||
---|---|---|
72 | I don't remember at which point it was lost, but at some point I had a call with an empty key in the regular LTO path, it is possible that it never made it to phab. |
This now works with the support I added to gold-plugin (will commit as a follow-on) with the following change. This is required because append() will cause EntryPath to be CacheDirectoryPath+"/" and the test later in the destructor will not think they are equal.
diff --git a/lib/LTO/Caching.cpp b/lib/LTO/Caching.cpp
index 11e9fd6..9bba3ab 100644
- a/lib/LTO/Caching.cpp
+++ b/lib/LTO/Caching.cpp
@@ -93,10 +93,12 @@ std::unique_ptr<raw_pwrite_stream> CacheObjectOutput::getStream() {
bool CacheObjectOutput::tryLoadFromCache(StringRef Key) {
assert(!CacheDirectoryPath.empty() && "CacheObjectOutput was initialized without a cache path");
- sys::path::append(EntryPath, CacheDirectoryPath, Key);
- if (Key.empty())
- // Client didn't compute a valid key. EntryPath has been set to
+ if (Key.empty()) {
+ // Client didn't compute a valid key. Set EntryPath to
// CacheDirectoryPath.
+ EntryPath = CacheDirectoryPath;
return false;
+ }
+ sys::path::append(EntryPath, CacheDirectoryPath, Key);
return sys::fs::exists(EntryPath);
}
lib/LTO/LTOBackend.cpp | ||
---|---|---|
248 ↗ | (On Diff #68909) | Consider refactoring this so the above block from the AddOutput call through the codegen call are in a helper passed a ThreadId, then it can be called here and in splitCodeGen. |
Is it really optional? It seems like we should always have a TempFilename if we tried to write to the cache via getStream(). Maybe: "Path to temporary file used to buffer output that will be written to a cache entry when this object is destroyed."