ThinLTO: don't perform incremental LTO on module without a hash

[mehdi_amini]

Summary

Clang always emit a hash for ThinLTO, but as other frontend are
starting to use ThinLTO, this could be a serious bug.

[tejohnson]

LGTM, but looks like this is on top of D25300, so I guess that one needs to go in first?

[mehdi_amini]

It is conceptually independent and could go in now, but yes I'll wait for the other one so that I don't have to fix the conflict.

[tejohnson]

In D25379#565109, @mehdi_amini wrote:

It is conceptually independent and could go in now, but yes I'll wait for the other one so that I don't have to fix the conflict.

I don't have the other approach started yet, and this is needed to be able to fix the bug fixed by D25300.

[tejohnson]

In D25379#565121, @tejohnson wrote:

In D25379#565109, @mehdi_amini wrote:

It is conceptually independent and could go in now, but yes I'll wait for the other one so that I don't have to fix the conflict.

I don't have the other approach started yet, and this is needed to be able to fix the bug fixed by D25300.

Nevermind, wrong patch! That's what happens when I try to review on my phone...

llvm/trunk/lib/LTO/LTO.cpp

auto RunThinBackend = [&](AddStreamFn AddStream) {
           parseBitcodeFile(MBRef, BackendContext);
       assert(MOrErr && "Unable to load module in thread?");
 
       return thinBackend(Conf, Task, AddStream, **MOrErr, CombinedIndex,
                          ImportList, DefinedGlobals, ModuleMap);
     };
 
     auto ModuleID = MBRef.getBufferIdentifier();
-    if (!Cache || !CombinedIndex.modulePaths().count(ModuleID))
-      // Cache disabled or no entry for this module in the combined index
+
+    if (!Cache || !CombinedIndex.modulePaths().count(ModuleID) ||
+        all_of(CombinedIndex.getModuleHash(ModuleID),
+               [](uint32_t V) { return V == 0; }))
+      // Cache disabled or no entry for this module in the combined index or
+      // no module hash.
       return RunThinBackend(AddStream);
 
     SmallString<40> Key;
     // The module may be cached, this helps handling it.
     computeCacheKey(Key, CombinedIndex, ModuleID, ImportList, ExportList,
                     ResolvedODR, DefinedGlobals);
     if (AddStreamFn CacheAddStream = Cache(Task, Key))
       return RunThinBackend(CacheAddStream);

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

if (!Index.modulePaths().count(ModuleID))
       // The module does not have an entry, it can't have a hash at all
       return;
 
     // Compute the unique hash for this entry
     // This is based on the current compiler version, the module itself, the
     // export list, the hash for every single module in the import list, the
     // list of ResolvedODR for the module, and the list of preserved symbols.
 
+    // Include the hash for the current module
+    auto ModHash = Index.getModuleHash(ModuleID);
+
+    if (all_of(ModHash, [](uint32_t V) { return V == 0; }))
+      // No hash entry, no caching!
+      return;
+
     SHA1 Hasher;
 
     // Start with the compiler revision
     Hasher.update(LLVM_VERSION_STRING);
 #ifdef HAVE_LLVM_REVISION
     Hasher.update(LLVM_REVISION);
 #endif
 
-    // Include the hash for the current module
-    auto ModHash = Index.getModuleHash(ModuleID);
     Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
     for (auto F : ExportList)
       // The export list can impact the internalization, be conservative here
       Hasher.update(ArrayRef<uint8_t>((uint8_t *)&F, sizeof(F)));
 
     // Include the hash for every module we import functions from
     for (auto &Entry : ImportList) {
       auto ModHash = Index.getModuleHash(Entry.first());

llvm/trunk/test/ThinLTO/X86/cache.ll

+; Verify first that *without* hash, we don't use the cache.
+
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/cache.ll -o %t2.bc
 
+; Verify that enabling caching is ignoring module without hash
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc  %t.bc -thinlto-cache-dir %t.cache
+; RUN: ls %t.cache/llvmcache.timestamp
+; RUN: ls %t.cache | count 1
+
+; Verify that enabling caching is ignoring module without hash with llvm-lto2
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: llvm-lto2 -o %t.o %t2.bc  %t.bc -cache-dir %t.cache \
+; RUN:  -r=%t2.bc,_main,plx \
+; RUN:  -r=%t2.bc,_globalfunc,lx \
+; RUN:  -r=%t.bc,_globalfunc,plx
+; RUN: ls %t.cache | count 0
+
+
+; Repeat again, *with* hash this time.
+
+; RUN: opt -module-hash -module-summary %s -o %t.bc
+; RUN: opt -module-hash -module-summary %p/Inputs/cache.ll -o %t2.bc
+
 ; Verify that enabling caching is working
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc  %t.bc -thinlto-cache-dir %t.cache
 ; RUN: ls %t.cache/llvmcache.timestamp
 ; RUN: ls %t.cache | count 3
 
 ; Verify that enabling caching is working with llvm-lto2
 ; RUN: rm -Rf %t.cache && mkdir %t.cache

llvm/trunk/test/ThinLTO/X86/empty_module_with_cache.ll

 
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/empty_module_with_cache.ll -o %t2.bc
 
 ; Verify that caching is disabled for module without hash
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; RUN: llvm-lto -thinlto-action=run %t2.bc  %t.bc -exported-symbol=main -thinlto-cache-dir %t.cache
 ; RUN: ls %t.cache/llvmcache.timestamp
-; RUN: ls %t.cache | count 2
+; RUN: ls %t.cache | count 1
 
 ; Verify that caching is disabled for module without hash, with llvm-lto2
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; RUN: llvm-lto2 -o %t.o %t2.bc  %t.bc -cache-dir %t.cache \
 ; RUN:  -r=%t2.bc,_main,plx
-; RUN: ls %t.cache | count 1
+; RUN: ls %t.cache | count 0
 
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"