[ThinLTO/gold] Write empty imports even for modules with symbols

[vitalybuka]

Summary

ThinLTO may skip object for other reasons, e.g. if there is no summary.

[tejohnson]

I guess this is needed for the case when we need to split the module due to the existence of type metadata, but there is no module id so we can only write as regular LTO?

llvm/tools/gold/gold-plugin.cpp

893

It would be good to avoid writing all these files unnecessarily (the common case). You could presumably get back from LTO::add (via addModule below) whether any module in the file had a summary (from the BitcodeLTOInfo structure in LTO::addModule).

[vitalybuka]

avoid double writes

[vitalybuka]

llvm/tools/gold/gold-plugin.cpp

893

Done, but in a little bit different way. We can just inform the caller about what was actually written. This way we can avoid having in the gold plugin the logic which checks LTO properties and decides what should be written or skipped.

[tejohnson]

llvm/include/llvm/LTO/LTO.h

218 ↗

(On Diff #131909)

Document new param

llvm/tools/gold/gold-plugin.cpp

893

Should this have been removed now?

llvm/tools/llvm-lto2/llvm-lto2.cpp

245 ↗

(On Diff #131909)

I think the only change here is the addition of the braces - I assume that was from some debugging code that was since removed, so the braces and the change to this file can be removed.

[vitalybuka]

addressed comments

llvm/tools/gold/gold-plugin.cpp

893

Ugh, removing this was the point of the patch. Probably result of invalid local merges.

[vitalybuka]

addressed comments

[vitalybuka]

corrected doc string

[tejohnson]

llvm/tools/gold/gold-plugin.cpp

873

Thinking through this some more, this seems a little dicey, because it assumes that createLTO will never try to access these filename strings that are owned by this set after the callback. In fact, that doesn't even seem to be the case currently (ModulePath which is a StringRef of the module identifier is passed to EmitImportsFiles after this callback is invoked). Rather than try to fix that, since it would still be fragile, I suggest making this a StringMap and noting in the map whether the file was written.

llvm/tools/llvm-lto2/llvm-lto2.cpp

245 ↗

(On Diff #131909)

Oh I see, there was a change here. In any case, please remove the added braces which aren't necessary.

[vitalybuka]

fixed use of container for strings

[vitalybuka]

llvm/tools/gold/gold-plugin.cpp

873

Oh, I've totally missed "owning" part in the first place.

[tejohnson]

LGTM

llvm/tools/gold/gold-plugin.cpp

937

nit: remove unnecessary braces

[vitalybuka]

no braces

llvm/test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll

+; RUN: rm -f %t*.o.thinlto.bc
+; RUN: rm -f %t*.o.imports
+
 ; First generate bitcode with a module summary index for each file
-; RUN: opt -module-summary %s -o %t.o
+; RUN: opt -module-summary %s -o %t1.o
 ; RUN: opt -module-summary %p/Inputs/thinlto_emit_linked_objects.ll -o %t2.o
+; RUN: opt %s -o %t3.o
 
 ; Next do the ThinLink step, specifying thinlto-index-only so that the gold
 ; plugin exits after generating individual indexes. The objects the linker
 ; decided to include in the link should be emitted into the file specified
-; after 'thinlto-index-only='. In this version of the test, only %t.o will
+; after 'thinlto-index-only='. In this version of the test, only %t1.o will
 ; be included in the link, and not %t2.o since it is within
 ; a library (--start-lib/--end-lib pair) and not strongly referenced.
 ; Note that the support for detecting this is in gold v1.12.
-; RUN: rm -f %t.o.thinlto.bc
-; RUN: rm -f %t2.o.thinlto.bc
-; RUN: rm -f %t.o.imports
-; RUN: rm -f %t2.o.imports
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext \
 ; RUN:    --plugin-opt=thinlto \
-; RUN:    --plugin-opt=thinlto-index-only=%t3 \
+; RUN:    --plugin-opt=thinlto-index-only=%t.index \
 ; RUN:    --plugin-opt=thinlto-emit-imports-files \
 ; RUN:    -m elf_x86_64 \
 ; RUN:    -o %t4 \
-; RUN:    %t.o \
+; RUN:    %t1.o %t3.o \
 ; RUN:    --start-lib %t2.o --end-lib
 
 ; Ensure that the expected output files are created, even for the file
 ; the linker decided not to include in the link.
-; RUN: ls %t.o.thinlto.bc
+; RUN: ls %t1.o.thinlto.bc
 ; RUN: ls %t2.o.thinlto.bc
-; RUN: ls %t.o.imports
+; RUN: ls %t3.o.thinlto.bc
+; RUN: ls %t1.o.imports
 ; RUN: ls %t2.o.imports
+; RUN: ls %t3.o.imports
 
-; RUN: cat %t3 | FileCheck %s
-; CHECK: thinlto_emit_linked_objects.ll.tmp.o
+; RUN: cat %t.index | FileCheck %s
+; CHECK: thinlto_emit_linked_objects.ll.tmp1.o
 ; CHECK-NOT: thinlto_emit_linked_objects.ll.tmp2.o
+; CHECK-NOT: thinlto_emit_linked_objects.ll.tmp3.o
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define i32 @main() {
 entry:
   ret i32 0
 }

llvm/tools/gold/gold-plugin.cpp

static ld_plugin_status allSymbolsReadHook() {
   // through Lto->run().
   DenseMap<void *, std::unique_ptr<PluginInputFile>> HandleToInputFile;
 
   std::unique_ptr<LTO> Lto = createLTO();
 
   std::string OldPrefix, NewPrefix;
   if (options::thinlto_index_only)
     getThinLTOOldAndNewPrefix(OldPrefix, NewPrefix);
 
   std::string OldSuffix, NewSuffix;
   getThinLTOOldAndNewSuffix(OldSuffix, NewSuffix);
   // Set for owning string objects used as buffer identifiers.
   StringSet<> ObjectFilenames;
 
   for (claimed_file &F : Modules) {
     if (options::thinlto && !HandleToInputFile.count(F.leader_handle))
       HandleToInputFile.insert(std::make_pair(
           F.leader_handle, llvm::make_unique<PluginInputFile>(F.handle)));
-    const void *View = getSymbolsAndView(F);
     // In case we are thin linking with a minimized bitcode file, ensure
     // the module paths encoded in the index reflect where the backends
     // will locate the full bitcode files for compiling/importing.
     std::string Identifier =
         getThinLTOObjectFileName(F.name, OldSuffix, NewSuffix);
     auto ObjFilename = ObjectFilenames.insert(Identifier);
     assert(ObjFilename.second);
-    if (!View) {
-      if (options::thinlto_index_only)
-        // Write empty output files that may be expected by the distributed
-        // build system.
-        writeEmptyDistributedBuildOutputs(Identifier, OldPrefix, NewPrefix);
-      continue;
+    if (options::thinlto_index_only) {
     }
+    if (const void *View = getSymbolsAndView(F))
       addModule(*Lto, F, View, ObjFilename.first->first());
   }
 
   SmallString<128> Filename;
   // Note that getOutputFileName will append a unique ID for each task
   if (!options::obj_path.empty())
     Filename = options::obj_path;
   else if (options::TheOutputType == options::OT_SAVE_TEMPS)
     Filename = output_name + ".o";
if (!options::cache_dir.empty())
     Cache = check(localCache(options::cache_dir, AddBuffer));
 
   check(Lto->run(AddStream, Cache));
 
   if (options::TheOutputType == options::OT_DISABLE ||
       options::TheOutputType == options::OT_BC_ONLY)
     return LDPS_OK;
 
   if (options::thinlto_index_only) {
     if (llvm::AreStatisticsEnabled())
       llvm::PrintStatistics();
     cleanup_hook();
     exit(0);
   }
 
   for (unsigned I = 0; I != MaxTasks; ++I)
     if (!Filenames[I].empty())