This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add caching to the new LTO API
ClosedPublic

Authored by mehdi_amini on Aug 16 2016, 10:01 PM.

Details

Summary

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.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to [ThinLTO] Add caching to the new LTO API.
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini added subscribers: llvm-commits, pcc.
dberris added inline comments.
include/llvm/LTO/Caching.h
38

Is it worth typedef'ing std::function<void(std::unique_ptr<MemoryBuffer>)> such that you don't repeat yourself here?

dberris added inline comments.Aug 16 2016, 10:31 PM
include/llvm/LTO/Caching.h
36

Incomplete documentation?

67

Unsure what this means in Doxygen, but should there be a closing @} somewhere else?

mehdi_amini marked 3 inline comments as done.

Address Dean's comments.

Thanks for catching this :)

include/llvm/LTO/Caching.h
67

It is for grouping (\defgroup), but here it is a bad copy/paste. I removed all this code.

Missed one...

Bugfix in Caching.cpp

tejohnson edited edge metadata.Aug 20 2016, 6:52 AM

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.

501

As per offline discussion, move this block under isCachingEnabled()

514

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.

mehdi_amini marked 12 inline comments as done.
mehdi_amini edited edge metadata.

Address comments.

I had a bit of trouble following the flow, so I added in some suggestions for more comments and some assertion checking.

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.

514

We need the output in the first place to be able to check if the caching is enabled or not.

mehdi_amini marked 2 inline comments as done.

Minor update to a comment

mehdi_amini marked 2 inline comments as done.Aug 20 2016, 2:03 PM

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

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.

Yes it is passing for me, I'll give a try with ASAN.

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.

Yes it is passing for me, I'll give a try with ASAN.

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.

mehdi_amini marked 12 inline comments as done.Aug 22 2016, 9:09 AM

The new comments help a lot.

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 :)

Address comments

Still needs fix for coredump (see my earlier response this morning on why that happens).

Make a copy of Path in the lambda.

Luckily you're a sharp reviewer :)

tejohnson added inline comments.Aug 22 2016, 11:28 AM
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:

  • since for ThinLTO when we have save-temps we setup a hook and are still unconditionally creating a combined LTO module and trying to code gen it
  • my initial round of gold-plugin support was being lazy just for testing and always setting a CacheDir to something hardcoded (this would obviously go away and be set via a plugin-opt)

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.

mehdi_amini added inline comments.Aug 22 2016, 11:48 AM
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:

  • make it optional to call tryLoadFromCache, in which case we are using a temporary.
  • Have LTO call tryLoadFromCache with an empty key, and have the logic handles the empty key as "uncached".

This would allow the monolithic LTO compilation to be cached as well in the future.

tejohnson added inline comments.Aug 22 2016, 1:12 PM
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.

make it optional to call tryLoadFromCache, in which case we are using a temporary.

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?).

Have LTO call tryLoadFromCache with an empty key, and have the logic handles the empty key as "uncached".

This would be similar I guess?

mehdi_amini added inline comments.Aug 22 2016, 1:33 PM
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.

Correctly handle mix of ThinLTO and monolithic LTO together.

mehdi_amini marked 4 inline comments as done.Aug 22 2016, 2:15 PM
tejohnson accepted this revision.Aug 23 2016, 1:20 PM
tejohnson edited edge metadata.

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.

This revision is now accepted and ready to land.Aug 23 2016, 1:20 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini marked an inline comment as done.Aug 23 2016, 2:38 PM