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

[Mehdi AMINI via llvm-commits]

mehdi_amini 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
----------------
tejohnson wrote:
> 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?
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.


https://reviews.llvm.org/D23599