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 ↗

(On Diff #73718)

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 ↗

(On Diff #73718)

Leftover debugging message

lib/LTO/ThinLTOCodeGenerator.cpp

278 ↗

(On Diff #73718)

Leftover debugging message.

512 ↗

(On Diff #73718)

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

test/ThinLTO/X86/empty_module_with_cache.ll

14 ↗

(On Diff #73718)

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

tools/llvm-lto/llvm-lto.cpp

393 ↗

(On Diff #73718)

Leftover debugging message.

[mehdi_amini]

Early comments (still working on the rest)

lib/Bitcode/Reader/BitcodeReader.cpp

6151 ↗

(On Diff #73718)

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 ↗

(On Diff #73718)

We haven't hooked the cache pruning apparently.

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

512 ↗

(On Diff #73718)

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 ↗

(On Diff #73718)

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 ↗

(On Diff #73718)

Ok, the new comment should be removed then.

test/ThinLTO/X86/empty_module_with_cache.ll

14 ↗

(On Diff #73718)

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 ↗

(On Diff #73718)

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

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

239 ↗

(On Diff #73977)

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 ↗

(On Diff #73718)

The pruning adds a timestamp file.

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

239 ↗

(On Diff #73977)

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 ↗

(On Diff #73977)

A module with an empty summary and no hash.

[tejohnson]

LGTM with new test described below.

lib/LTO/ThinLTOCodeGenerator.cpp

239 ↗

(On Diff #73977)

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

llvm/trunk/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;
           }

llvm/trunk/lib/IR/ModuleSummaryIndex.cpp

 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/ADT/StringMap.h"
 using namespace llvm;
 
 // Create the combined module index/summary from multiple
 // per-module instances.
 void ModuleSummaryIndex::mergeFrom(std::unique_ptr<ModuleSummaryIndex> Other,
                                    uint64_t NextModuleId) {
+  if (Other->modulePaths().empty())
+    return;
+
+  assert(Other->modulePaths().size() == 1 &&
+         "Can only merge from an single-module index at that time");
+
+  StringRef OtherModPath = Other->modulePaths().begin()->first();
+  StringRef ModPath = addModulePath(OtherModPath, NextModuleId,
+                                    Other->getModuleHash(OtherModPath))
+                          ->first();
 
-  StringRef ModPath;
   for (auto &OtherGlobalValSummaryLists : *Other) {
     GlobalValue::GUID ValueGUID = OtherGlobalValSummaryLists.first;
     GlobalValueSummaryList &List = OtherGlobalValSummaryLists.second;
 
     // Assert that the value summary list only has one entry, since we shouldn't
     // have duplicate names within a single per-module index.
     assert(List.size() == 1);
     std::unique_ptr<GlobalValueSummary> Summary = std::move(List.front());
 
-    // Add the module path string ref for this module if we haven't already
-    // saved a reference to it.
-    if (ModPath.empty()) {
-      auto Path = Summary->modulePath();
-      ModPath = addModulePath(Path, NextModuleId, Other->getModuleHash(Path))
-                    ->first();
-    } else
-      assert(ModPath == Summary->modulePath() &&
-             "Each module in the combined map should have a unique ID");
-
     // Note the module path string ref was copied above and is still owned by
     // the original per-module index. Reset it to the new module path
     // string reference owned by the combined index.
     Summary->setModulePath(ModPath);
 
     // Add new value summary to existing list. There may be duplicates when
     // combining GlobalValueMap entries, due to COMDAT values. Any local
     // values were given unique global IDs.

llvm/trunk/lib/LTO/LTO.cpp

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,

llvm/trunk/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

llvm/trunk/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
+}

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

+; RUN: opt -module-hash -module-summary %s -o %t.bc
+; RUN: opt -module-hash -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 3
+
+; 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 2
+
+; Same, but without hash, the index will be empty and caching should not happen
+
+; 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
+
+; 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
+
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"

llvm/trunk/tools/llvm-lto/llvm-lto.cpp

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