[ThinLTO] Always emit a summary when compiling in ThinLTO mode

[tejohnson]

Summary

Emit an empty summary section, instead of no summary section, when
there are no global variables in the index. This ensures that LTO
will treat these files as ThinLTO inputs, instead of as regular
LTO inputs.

In addition to not being what the user likely intended when
compiling with -flto=thin, the current behavior is problematic for
distributed build systems that expect to get ThinLTO index and imports
files back for each input compiled with -flto=thin. Combining into
a single regular LTO module also reduces the backend parallelism.
And in the case where the index was suppressed due to uses in
inline assembly, combining into a single LTO module could provoke
renaming of duplicates that we were trying to prevent by suppressing
the index.

This change required a couple of fixes to handle the empty summary
section.

[mehdi_amini]

Can you add a simple test that checks that a summary has been emitted? (So that we don't depend on the distributed thing to test this).
I believe we already have test for opt -module-summary, using llvm-bcanalyzer IIRC.

[tejohnson]

Add llvm-bcanalyzer based test

[mehdi_amini]

Thanks!

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

case BitstreamEntry::SubBlock:
         // is no summary section.
         assert(((SeenValueSymbolTable && VSTOffset > 0) ||
                 !SeenGlobalValSummary) &&
                "Expected early VST parse via VSTOffset record");
         if (Stream.SkipBlock())
           return error("Invalid record");
         break;
       case bitc::GLOBALVAL_SUMMARY_BLOCK_ID:
-        assert(VSTOffset > 0 && "Expected non-zero VST offset");
         assert(!SeenValueSymbolTable &&
                "Already read VST when parsing summary block?");
+        // We might not have a VST if there were no values in the
+        // summary. An empty summary block generated when we are
+        // performing ThinLTO compiles so we don't later invoke
+        // the regular LTO process on them.
+        if (VSTOffset > 0) {
           if (std::error_code EC =
                   parseValueSymbolTable(VSTOffset, ValueIdToLinkageMap))
             return EC;
           SeenValueSymbolTable = true;
+        }
         SeenGlobalValSummary = true;
         if (std::error_code EC = parseEntireSummary())
           return EC;
         break;
       case bitc::MODULE_STRTAB_BLOCK_ID:
         if (std::error_code EC = parseModuleStringTable())
           return EC;
         break;

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

 // Current version for the summary.
 // This is bumped whenever we introduce changes in the way some record are
 // interpreted, like flags for instance.
 static const uint64_t INDEX_VERSION = 1;
 
 /// Emit the per-module summary section alongside the rest of
 /// the module's bitcode.
 void ModuleBitcodeWriter::writePerModuleGlobalValueSummary() {
-  if (Index->begin() == Index->end())
-    return;
-
   Stream.EnterSubblock(bitc::GLOBALVAL_SUMMARY_BLOCK_ID, 4);
 
   Stream.EmitRecord(bitc::FS_VERSION, ArrayRef<uint64_t>{INDEX_VERSION});
 
+  if (Index->begin() == Index->end()) {
+    Stream.ExitBlock();
+    return;
+  }
+
   // Abbrev for FS_PERMODULE.
   BitCodeAbbrev *Abbv = new BitCodeAbbrev();
   Abbv->Add(BitCodeAbbrevOp(bitc::FS_PERMODULE));
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));   // valueid
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // flags
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 8));   // instcount
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 4));   // numrefs
   // numrefs x valueid, n x (valueid, callsitecount)

llvm/trunk/lib/LTO/LTO.cpp

if (Conf.CombinedIndexHook && !Conf.CombinedIndexHook(ThinLTO.CombinedIndex))
     return Error();
 
   // Collect for each module the list of function it defines (GUID ->
   // Summary).
   StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
       ModuleToDefinedGVSummaries(ThinLTO.ModuleMap.size());
   ThinLTO.CombinedIndex.collectDefinedGVSummariesPerModule(
       ModuleToDefinedGVSummaries);
+  // Create entries for any modules that didn't have any GV summaries
+  // (either they didn't have any GVs to start with, or we suppressed
+  // generation of the summaries because they e.g. had inline assembly
+  // uses that couldn't be promoted/renamed on export). This is so
+  // InProcessThinBackend::start can still launch a backend thread, which
+  // is passed the map of summaries for the module, without any special
+  // handling for this case.
+  for (auto &Mod : ThinLTO.ModuleMap)
+    if (!ModuleToDefinedGVSummaries.count(Mod.first))
+      ModuleToDefinedGVSummaries.try_emplace(Mod.first);
 
   StringMap<FunctionImporter::ImportMapTy> ImportLists(
       ThinLTO.ModuleMap.size());
   StringMap<FunctionImporter::ExportSetTy> ExportLists(
       ThinLTO.ModuleMap.size());
   ComputeCrossModuleImport(ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
                            ImportLists, ExportLists);
 

llvm/trunk/test/Bitcode/thinlto-empty-summary-section.ll

+; Ensure we get a summary block even when the file is empty.
+; RUN: opt -module-summary %s -o %t.o
+; RUN: llvm-bcanalyzer -dump %t.o | FileCheck %s
+
+; CHECK: <GLOBALVAL_SUMMARY_BLOCK
+; CHECK: <VERSION op0=
+; CHECK: </GLOBALVAL_SUMMARY_BLOCK>

llvm/trunk/test/ThinLTO/X86/Inputs/emit_imports.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
 define void @g() {
 entry:
   ret void
 }

llvm/trunk/test/ThinLTO/X86/Inputs/empty.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"

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

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/emit_imports.ll -o %t2.bc
-; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc
-; RUN: llvm-lto -thinlto-action=emitimports -thinlto-index %t.index.bc %t1.bc %t2.bc
+; Include a file with an empty module summary index, to ensure that the expected
+; output files are created regardless, for a distributed build system.
+; RUN: opt -module-summary %p/Inputs/empty.ll -o %t3.bc
+; RUN: rm -f %t3.bc.imports
+; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc %t3.bc
+; RUN: llvm-lto -thinlto-action=emitimports -thinlto-index %t.index.bc %t1.bc %t2.bc %t3.bc
 
 ; The imports file for this module contains the bitcode file for
 ; Inputs/emit_imports.ll
 ; RUN: cat %t1.bc.imports | count 1
 ; RUN: cat %t1.bc.imports | FileCheck %s --check-prefix=IMPORTS1
 ; IMPORTS1: emit_imports.ll.tmp2.bc
 
 ; The imports file for Input/emit_imports.ll is empty as it does not import anything.
 ; RUN: cat %t2.bc.imports | count 0
 
+; The imports file for Input/empty.ll is empty but should exist.
+; RUN: cat %t3.bc.imports | count 0
+
 ; RUN: rm -f %t1.thinlto.bc %t1.bc.imports
 ; RUN: rm -f %t2.thinlto.bc %t2.bc.imports
-; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o \
+; RUN: rm -f %t3.bc.thinlto.bc %t3.bc.imports
+; RUN: llvm-lto2 %t1.bc %t2.bc %t3.bc -o %t.o -save-temps \
 ; RUN:     -thinlto-distributed-indexes \
 ; RUN:     -r=%t1.bc,g, \
 ; RUN:     -r=%t1.bc,f,px \
 ; RUN:     -r=%t2.bc,g,px
 
 ; RUN: cat %t1.bc.imports | count 1
 ; RUN: cat %t1.bc.imports | FileCheck %s --check-prefix=IMPORTS1
 
 ; The imports file for Input/emit_imports.ll is empty as it does not import anything.
 ; RUN: cat %t2.bc.imports | count 0
 
+; The imports file for Input/empty.ll is empty but should exist.
+; RUN: cat %t3.bc.imports | count 0
+
+; The index file should be created even for the input with an empty summary.
+; RUN: ls %t3.bc.thinlto.bc
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
 declare void @g(...)
 
 define void @f() {
 entry:
   call void (...) @g()
   ret void
 }

llvm/trunk/test/tools/gold/X86/thinlto_emit_imports.ll

 ; Generate summary sections and test gold handling.
 ; RUN: opt -module-summary %s -o %t.o
 ; RUN: opt -module-summary %p/Inputs/thinlto.ll -o %t2.o
+; Include a file with an empty module summary index, to ensure that the expected
+; output files are created regardless, for a distributed build system.
+; RUN: opt -module-summary %p/Inputs/thinlto_empty.ll -o %t3.o
 
 ; Ensure gold generates imports files if requested for distributed backends.
+; RUN: rm -f %t3.o.imports %t3.o.thinlto.bc
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=thinlto \
 ; RUN:    --plugin-opt=thinlto-index-only \
 ; RUN:    --plugin-opt=thinlto-emit-imports-files \
-; RUN:    -shared %t.o %t2.o -o %t3
+; RUN:    -shared %t.o %t2.o %t3.o -o %t4
 
 ; The imports file for this module contains the bitcode file for
 ; Inputs/thinlto.ll
 ; RUN: cat %t.o.imports | count 1
 ; RUN: cat %t.o.imports | FileCheck %s --check-prefix=IMPORTS1
 ; IMPORTS1: test/tools/gold/X86/Output/thinlto_emit_imports.ll.tmp2.o
 
 ; The imports file for Input/thinlto.ll is empty as it does not import anything.
 ; RUN: cat %t2.o.imports | count 0
 
+; The imports file for Input/thinlto_empty.ll is empty but should exist.
+; RUN: cat %t3.o.imports | count 0
+
+; The index file should be created even for the input with an empty summary.
+; RUN: ls %t3.o.thinlto.bc
+
 declare void @g(...)
 
 define void @f() {
 entry:
   call void (...) @g()
   ret void
 }