[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 ↗

(On Diff #131375)

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 ↗

(On Diff #131375)

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

897 ↗

(On Diff #131909)

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

897 ↗

(On Diff #131909)

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 ↗

(On Diff #132006)

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 ↗

(On Diff #132006)

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

[tejohnson]

LGTM

llvm/tools/gold/gold-plugin.cpp

936 ↗

(On Diff #132011)

nit: remove unnecessary braces

[vitalybuka]

no braces

llvm/trunk/include/llvm/LTO/LTO.h

 /// running the individual backend jobs. This backend is for distributed builds
 /// where separate processes will invoke the real backends.
 ///
 /// To find the path to write the index to, the backend checks if the path has a
 /// prefix of OldPrefix; if so, it replaces that prefix with NewPrefix. It then
 /// appends ".thinlto.bc" and writes the index to that path. If
 /// ShouldEmitImportsFiles is true it also writes a list of imported files to a
 /// similar path with ".imports" appended instead.
+/// OnWrite is callback which receives module identifier and notifies LTO user
+/// that index file for the module (and optionally imports file) was created.
+using IndexWriteCallback = std::function<void(const std::string &)>;
 ThinBackend createWriteIndexesThinBackend(std::string OldPrefix,
                                           std::string NewPrefix,
                                           bool ShouldEmitImportsFiles,
-                                          std::string LinkedObjectsFile);
+                                          std::string LinkedObjectsFile,
+                                          IndexWriteCallback OnWrite);
 
 /// This class implements a resolution-based interface to LLVM's LTO
 /// functionality. It supports regular LTO, parallel LTO code generation and
 /// ThinLTO. You can use it from a linker in the following way:
 /// - Set hooks and code generation options (see lto::Config struct defined in
 ///   Config.h), and use the lto::Config object to create an lto::LTO object.
 /// - Create lto::InputFile objects using lto::InputFile::create(), then use
 ///   the symbols() function to enumerate its symbols and compute a resolution

llvm/trunk/lib/LTO/LTO.cpp

 namespace {
 class WriteIndexesThinBackend : public ThinBackendProc {
   std::string OldPrefix, NewPrefix;
   bool ShouldEmitImportsFiles;
 
   std::string LinkedObjectsFileName;
   std::unique_ptr<llvm::raw_fd_ostream> LinkedObjectsFile;
 
+  lto::IndexWriteCallback OnWrite;
+
 public:
   WriteIndexesThinBackend(
       Config &Conf, ModuleSummaryIndex &CombinedIndex,
       const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
       std::string OldPrefix, std::string NewPrefix, bool ShouldEmitImportsFiles,
-      std::string LinkedObjectsFileName)
+      std::string LinkedObjectsFileName, lto::IndexWriteCallback OnWrite)
       : ThinBackendProc(Conf, CombinedIndex, ModuleToDefinedGVSummaries),
         OldPrefix(OldPrefix), NewPrefix(NewPrefix),
         ShouldEmitImportsFiles(ShouldEmitImportsFiles),
-        LinkedObjectsFileName(LinkedObjectsFileName) {}
+        LinkedObjectsFileName(LinkedObjectsFileName), OnWrite(OnWrite) {}
 
   Error start(
       unsigned Task, BitcodeModule BM,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
       MapVector<StringRef, BitcodeModule> &ModuleMap) override {
     StringRef ModulePath = BM.getModuleIdentifier();
gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
                                      ImportList, ModuleToSummariesForIndex);
 
     raw_fd_ostream OS(NewModulePath + ".thinlto.bc", EC,
                       sys::fs::OpenFlags::F_None);
     if (EC)
       return errorCodeToError(EC);
     WriteIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex);
 
-    if (ShouldEmitImportsFiles)
-      return errorCodeToError(
-          EmitImportsFiles(ModulePath, NewModulePath + ".imports", ImportList));
+    if (ShouldEmitImportsFiles) {
+      EC = EmitImportsFiles(ModulePath, NewModulePath + ".imports", ImportList);
+      if (EC)
+        return errorCodeToError(EC);
+    }
+
+    if (OnWrite)
+      OnWrite(ModulePath);
     return Error::success();
   }
 
   Error wait() override { return Error::success(); }
 };
 } // end anonymous namespace
 
 ThinBackend lto::createWriteIndexesThinBackend(std::string OldPrefix,
                                                std::string NewPrefix,
                                                bool ShouldEmitImportsFiles,
-                                               std::string LinkedObjectsFile) {
+                                               std::string LinkedObjectsFile,
+                                               IndexWriteCallback OnWrite) {
   return [=](Config &Conf, ModuleSummaryIndex &CombinedIndex,
              const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
              AddStreamFn AddStream, NativeObjectCache Cache) {
     return llvm::make_unique<WriteIndexesThinBackend>(
         Conf, CombinedIndex, ModuleToDefinedGVSummaries, OldPrefix, NewPrefix,
-        ShouldEmitImportsFiles, LinkedObjectsFile);
+        ShouldEmitImportsFiles, LinkedObjectsFile, OnWrite);
   };
 }
 
 Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache) {
   if (ThinLTO.ModuleMap.empty())
     return Error::success();
 
   if (Conf.CombinedIndexHook && !Conf.CombinedIndexHook(ThinLTO.CombinedIndex))

llvm/trunk/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/trunk/tools/gold/gold-plugin.cpp

 /// \p NewPrefix strings, if it was specified.
 static void getThinLTOOldAndNewPrefix(std::string &OldPrefix,
                                       std::string &NewPrefix) {
   StringRef PrefixReplace = options::thinlto_prefix_replace;
   assert(PrefixReplace.empty() || PrefixReplace.find(";") != StringRef::npos);
   std::tie(OldPrefix, NewPrefix) = PrefixReplace.split(';');
 }
 
-static std::unique_ptr<LTO> createLTO() {
+static std::unique_ptr<LTO> createLTO(IndexWriteCallback OnIndexWrite) {
   Config Conf;
   ThinBackend Backend;
 
   Conf.CPU = options::mcpu;
   Conf.Options = InitTargetOptionsFromCodeGenFlags();
 
   // Disable the new X86 relax relocations since gold might not support them.
   // FIXME: Check the gold version or add a new option to enable them.
static std::unique_ptr<LTO> createLTO(IndexWriteCallback OnIndexWrite) {
   Conf.OptLevel = options::OptLevel;
   if (options::Parallelism)
     Backend = createInProcessThinBackend(options::Parallelism);
   if (options::thinlto_index_only) {
     std::string OldPrefix, NewPrefix;
     getThinLTOOldAndNewPrefix(OldPrefix, NewPrefix);
     Backend = createWriteIndexesThinBackend(
         OldPrefix, NewPrefix, options::thinlto_emit_imports_files,
-        options::thinlto_linked_objects_file);
+        options::thinlto_linked_objects_file, OnIndexWrite);
   }
 
   Conf.OverrideTriple = options::triple;
   Conf.DefaultTriple = sys::getDefaultTargetTriple();
 
   Conf.DiagHandler = diagnosticHandler;
 
   switch (options::TheOutputType) {

 }
 
 // Write empty files that may be expected by a distributed build
 // system when invoked with thinlto_index_only. This is invoked when
 // the linker has decided not to include the given module in the
 // final link. Frequently the distributed build system will want to
 // confirm that all expected outputs are created based on all of the
 // modules provided to the linker.
-static void writeEmptyDistributedBuildOutputs(std::string &ModulePath,
-                                              std::string &OldPrefix,
-                                              std::string &NewPrefix) {
+static void writeEmptyDistributedBuildOutputs(const std::string &ModulePath,
+                                              const std::string &OldPrefix,
+                                              const std::string &NewPrefix) {
   std::string NewModulePath =
       getThinLTOOutputFile(ModulePath, OldPrefix, NewPrefix);
   std::error_code EC;
   {
     raw_fd_ostream OS(NewModulePath + ".thinlto.bc", EC,
                       sys::fs::OpenFlags::F_None);
     if (EC)
       message(LDPL_FATAL, "Failed to write '%s': %s",
static ld_plugin_status allSymbolsReadHook() {
 
   // Map to own RAII objects that manage the file opening and releasing
   // interfaces with gold. This is needed only for ThinLTO mode, since
   // unlike regular LTO, where addModule will result in the opened file
   // being merged into a new combined module, we need to keep these files open
   // through Lto->run().
   DenseMap<void *, std::unique_ptr<PluginInputFile>> HandleToInputFile;
 
-  std::unique_ptr<LTO> Lto = createLTO();
+  // Owns string objects and tells if index file was already created.
+  StringMap<bool> ObjectToIndexFileState;
+
+  std::unique_ptr<LTO> Lto =
+      createLTO([&ObjectToIndexFileState](const std::string &Identifier) {
+        ObjectToIndexFileState[Identifier] = true;
+      });
 
   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);
+    auto ObjFilename = ObjectToIndexFileState.insert({Identifier, false});
     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 (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";
static ld_plugin_status allSymbolsReadHook() {
   };
 
   NativeObjectCache Cache;
   if (!options::cache_dir.empty())
     Cache = check(localCache(options::cache_dir, AddBuffer));
 
   check(Lto->run(AddStream, Cache));
 
+  // Write empty output files that may be expected by the distributed build
+  // system.
+  if (options::thinlto_index_only)
+    for (auto &Identifier : ObjectToIndexFileState)
+      if (!Identifier.getValue())
+        writeEmptyDistributedBuildOutputs(Identifier.getKey(), OldPrefix,
+                                          NewPrefix);
+
   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();

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

static int run(int argc, char **argv) {
   if (FileType.getNumOccurrences())
     Conf.CGFileType = FileType;
 
   Conf.OverrideTriple = OverrideTriple;
   Conf.DefaultTriple = DefaultTriple;
 
   ThinBackend Backend;
   if (ThinLTODistributedIndexes)
-    Backend = createWriteIndexesThinBackend("", "", true, "");
+    Backend = createWriteIndexesThinBackend("", "", true, "", {});
   else
     Backend = createInProcessThinBackend(Threads);
   LTO Lto(std::move(Conf), std::move(Backend));
 
   bool HasErrors = false;
   for (std::string F : InputFilenames) {
     std::unique_ptr<MemoryBuffer> MB = check(MemoryBuffer::getFile(F), F);
     std::unique_ptr<InputFile> Input =