ThinLTO: handles modules with empty summaries

[mehdi_amini]

Summary

We need to add an entry in the combined-index for modules that have
a hash but otherwise empty summary, this is needed so that we can
get the hash for the module.

Also, if no entry is present in the combined index for a module, we
need to skip it when trying to compute a cache entry.

[mehdi_amini]

Forgot to git add the test

[tejohnson]

We need to add an entry in the combined-index for modules that have
a hash but otherwise empty summary, this is needed so that we can
get the hash for the module.

I'm confused because it appears the patch will actually prevent adding an entry in the combined index if the module has an empty summary (ThinLTOCodeGenerator::linkCombinedIndex).

lib/Bitcode/Reader/BitcodeReader.cpp

6151

I think we should just do the addModulePath unconditionally at the top of this routine if we have an index. Then we could assert here that we have module paths. And the other places where we addModulePath would just change to assert/access.

But why is this needed if we are later going to prevent adding this to the combined index since it has an empty summary section?

lib/LTO/LTO.cpp

381

Leftover debugging message

lib/LTO/ThinLTOCodeGenerator.cpp

278

Leftover debugging message.

512

Is a similar change required in LTO::addThinLTO (guarding the call to mergeFrom())?

test/ThinLTO/X86/empty_module_with_cache.ll

14

Why do we have 1 cache entry for llvm-lto2 but 2 for llvm-lto?

tools/llvm-lto/llvm-lto.cpp

393

Leftover debugging message.

[mehdi_amini]

Early comments (still working on the rest)

lib/Bitcode/Reader/BitcodeReader.cpp

6151

I think we should just do the addModulePath unconditionally at the top of this routine if we have an index. T

I tried this before, but I believe I ran in trouble because of the combined index: it means we'll add an entry for the combined index in the combined index itself when loading it.

test/ThinLTO/X86/empty_module_with_cache.ll

14

We haven't hooked the cache pruning apparently.

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

512

This test was spurious, I removed it.

[mehdi_amini]

Remove spurious test for empty index

[tejohnson]

Also, if no entry is present in the combined index for a module, we
need to skip it when trying to compute a cache entry.

The empty summary section just means we won't do any ThinLTO index-based optimization on the module (including importing/exporting). Why can't we still cache it? I.e. with a empty ImportList, ExportList, ResolvedODR, DefinedGlobals?

lib/Bitcode/Reader/BitcodeReader.cpp

6151

Ok that makes sense on why to do it here.

My other question is addressed by the change now added to mergeFrom() and removed from ThinLTOCodeGenerator.cpp, since now we are adding this to the combined index.

lib/LTO/ThinLTOCodeGenerator.cpp

512

Ok, the new comment should be removed then.

test/ThinLTO/X86/empty_module_with_cache.ll

14

I would expect llvm-lto2 to have more files in the cache without pruning. I guess with the current patch we should actually only have 1 since you are preventing caching for the empty module that has an empty summary (although I have a high-level question about that earlier)? So I guess the question is why we still get 2 for llvm-lto?

[mehdi_amini]

Cleanup! (spurious comment/format changes)

Also fix the test: the bitcode didn't have the module hash.

[tejohnson]

Still have question about why we can't cache if there is no module summary (assuming we do have a hash).

test/ThinLTO/X86/empty_module_with_cache.ll

14

Oh got it, llvm-lto produces the timestamp due to pruning, that is the extra file.

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

239

Is your question referring to this line?

If there is no entry for the module, it means it does not have a hash.

test/ThinLTO/X86/empty_module_with_cache.ll

14

The pruning adds a timestamp file.

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

239

Yes and I asked the wrong question, since with this patch modules without a summary will now get a hash and be able to be cached.

So I guess my remaining question is when do we not have an entry here after this patch (as opposed to a 0 valued hash)?

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

239

A module with an empty summary and no hash.

[tejohnson]

LGTM with new test described below.

lib/LTO/ThinLTOCodeGenerator.cpp

239

Ok, can you add that case to the new test?

lib/Bitcode/Reader/BitcodeReader.cpp

case BitstreamEntry::Record: {
         }
         /// MODULE_CODE_HASH: [5*i32]
         case bitc::MODULE_CODE_HASH: {
           if (Record.size() != 5)
             return error("Invalid hash length " + Twine(Record.size()).str());
           if (!TheIndex)
             break;
           if (TheIndex->modulePaths().empty())
-            // Does not have any summary emitted.
-            break;
+            // We always seed the index with the module.
+            TheIndex->addModulePath(Buffer->getBufferIdentifier(), 0);
           if (TheIndex->modulePaths().size() != 1)
             return error("Don't expect multiple modules defined?");
           auto &Hash = TheIndex->modulePaths().begin()->second.second;
           int Pos = 0;
           for (auto &Val : Record) {
             assert(!(Val >> 32) && "Unexpected high bits set");
             Hash[Pos++] = Val;
           }

lib/LTO/LTO.cpp

 // Add a ThinLTO object to the link.
 Error LTO::addThinLTO(std::unique_ptr<InputFile> Input,
                       ArrayRef<SymbolResolution> Res) {
   Module &M = Input->Obj->getModule();
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
   MemoryBufferRef MBRef = Input->Obj->getMemoryBufferRef();
+  errs() << "Load buffer " << MBRef.getBufferIdentifier() << "\n";
   ErrorOr<std::unique_ptr<object::ModuleSummaryIndexObjectFile>>
       SummaryObjOrErr =
           object::ModuleSummaryIndexObjectFile::create(MBRef, Conf.DiagHandler);
   if (!SummaryObjOrErr)
     return errorCodeToError(SummaryObjOrErr.getError());
   ThinLTO.CombinedIndex.mergeFrom((*SummaryObjOrErr)->takeIndex(),
                                   ThinLTO.ModuleMap.size());
 
auto RunThinBackend = [&](AddStreamFn AddStream) {
       ErrorOr<std::unique_ptr<Module>> MOrErr =
           parseBitcodeFile(MBRef, BackendContext);
       assert(MOrErr && "Unable to load module in thread?");
 
       return thinBackend(Conf, Task, AddStream, **MOrErr, CombinedIndex,
                          ImportList, DefinedGlobals, ModuleMap);
     };
 
-    if (!Cache)
+    auto ModuleID = MBRef.getBufferIdentifier();
+    if (!Cache || !CombinedIndex.modulePaths().count(ModuleID))
+      // Cache disabled or no entry for this module in the combined index
       return RunThinBackend(AddStream);
 
     SmallString<40> Key;
     // The module may be cached, this helps handling it.
-    computeCacheKey(Key, CombinedIndex, MBRef.getBufferIdentifier(),
-                    ImportList, ExportList, ResolvedODR, DefinedGlobals);
+    computeCacheKey(Key, CombinedIndex, ModuleID, ImportList, ExportList,
+                    ResolvedODR, DefinedGlobals);
     if (AddStreamFn CacheAddStream = Cache(Task, Key))
       return RunThinBackend(CacheAddStream);
 
     return Error();
   }
 
   Error start(
       unsigned Task, MemoryBufferRef MBRef,

lib/LTO/ThinLTOCodeGenerator.cpp

ModuleCacheEntry(
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
       const GVSummaryMapTy &DefinedFunctions,
       const DenseSet<GlobalValue::GUID> &PreservedSymbols) {
     if (CachePath.empty())
       return;
 
+    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.
 
     SHA1 Hasher;
 
     // Start with the compiler revision
for (auto &Entry : ResolvedODR) {
       Hasher.update(ArrayRef<uint8_t>((const uint8_t *)&Entry.first,
                                       sizeof(GlobalValue::GUID)));
       Hasher.update(ArrayRef<uint8_t>((const uint8_t *)&Entry.second,
                                       sizeof(GlobalValue::LinkageTypes)));
     }
 
     // Include the hash for the preserved symbols.
     for (auto &Entry : PreservedSymbols) {
-      if (DefinedFunctions.count(Entry))
+      if (DefinedFunctions.count(Entry)) {
+        errs() << "PreservedSymbols " << Entry << "\n";
         Hasher.update(
             ArrayRef<uint8_t>((const uint8_t *)&Entry, sizeof(GlobalValue::GUID)));
       }
+    }
 
     sys::path::append(EntryPath, CachePath, toHex(Hasher.result()));
   }
 
   // Access the path to this entry in the cache.
   StringRef getEntryPath() { return EntryPath; }
 
   // Try loading the buffer for this cache entry.
ErrorOr<std::unique_ptr<object::ModuleSummaryIndexObjectFile>> ObjOrErr =
                                                      diagnosticHandler);
     if (std::error_code EC = ObjOrErr.getError()) {
       // FIXME diagnose
       errs() << "error: can't create ModuleSummaryIndexObjectFile for buffer: "
              << EC.message() << "\n";
       return nullptr;
     }
     auto Index = (*ObjOrErr)->takeIndex();
-    if (CombinedIndex) {
+    // If the index is not empty, add it to the combined index
+    if (Index->begin() != Index->end()) {
+      if (CombinedIndex)
         CombinedIndex->mergeFrom(std::move(Index), ++NextModuleId);
-    } else {
+      else
         CombinedIndex = std::move(Index);
     }
   }
   return CombinedIndex;
 }
 
 /**
  * Perform promotion and renaming of exported internal functions.
  * Index is updated to reflect linkage changes from weak resolution.

test/ThinLTO/X86/Inputs/empty_module_with_cache.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+
+define i32 @main() {
+entry:
+  ret i32 0
+}

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 enabling caching is working, even if the module is empty
+; 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
+
+; Verify that enabling caching is working 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
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"

tools/llvm-lto/llvm-lto.cpp

 
 std::vector<std::unique_ptr<MemoryBuffer>>
 loadAllFilesForIndex(const ModuleSummaryIndex &Index) {
   std::vector<std::unique_ptr<MemoryBuffer>> InputBuffers;
 
   for (auto &ModPath : Index.modulePaths()) {
     const auto &Filename = ModPath.first();
     auto CurrentActivity = "loading file '" + Filename + "'";
+    errs() << CurrentActivity << "\n";
     auto InputOrErr = MemoryBuffer::getFile(Filename);
     error(InputOrErr, "error " + CurrentActivity);
     InputBuffers.push_back(std::move(*InputOrErr));
   }
   return InputBuffers;
 }
 
 std::unique_ptr<ModuleSummaryIndex> loadCombinedIndex() {
for (unsigned i = 0; i < InputFilenames.size(); ++i) {
       StringRef CurrentActivity = "loading file '" + Filename + "'";
       auto InputOrErr = MemoryBuffer::getFile(Filename);
       error(InputOrErr, "error " + CurrentActivity);
       InputBuffers.push_back(std::move(*InputOrErr));
       ThinGenerator.addModule(Filename, InputBuffers.back()->getBuffer());
     }
 
     auto CombinedIndex = ThinGenerator.linkCombinedIndex();
+    if (!CombinedIndex)
+      report_fatal_error("ThinLink didn't create an index");
     std::error_code EC;
     raw_fd_ostream OS(OutputFilename, EC, sys::fs::OpenFlags::F_None);
     error(EC, "error opening the file '" + OutputFilename + "'");
     WriteIndexToFile(*CombinedIndex, OS);
     return;
   }
 
   /// Load the combined index from disk, then compute and generate