[PATCH] D23599: [ThinLTO] Add caching to the new LTO API

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/LTO/Caching.cpp:71
@@ +70,3 @@
+  assert(!EntryPath.empty() && "API Violation: client didn't call "
+                               "tryLoadFromCache() before getStream()");
+  // Write to a temporary to avoid race condition
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > 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.
> 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.
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?


https://reviews.llvm.org/D23599