[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:
> 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.


https://reviews.llvm.org/D23599