[ThinLTO] Use std::map to get determistic imports files

[tejohnson]

Summary

I noticed that the .imports files emitted for distributed ThinLTO
backends do not have consistent ordering. This is because StringMap
iteration order is not guaranteed to be deterministic. Since we already
have a std::map with this information, used when emitting the individual
index files (ModuleToSummariesForIndex), use it for the imports files as
well.

This issue is likely causing some unnecessary rebuilds of the ThinLTO
backends in our distributed build system as the imports files are inputs
to those backends.

[steven_wu]

Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?

The other option is MapVector, which preserve the insertion order, which might be faster than std::map.

[tejohnson]

In D48783#1148412, @steven_wu wrote:

Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?

No effect on importing/symbol resolution etc. In fact, this is not an input to the compiler at all, it is used by the build system. The change in order will just make it look to the build system like it has changed when it hasn't.

The other option is MapVector, which preserve the insertion order, which might be faster than std::map.

The trickiness is that currently it may not be inserted in a deterministic order, because we use StringMap elsewhere in the thin link. So std::map is the most straightforward way to get the desired behavior.

[steven_wu]

In D48783#1148427, @tejohnson wrote:

In D48783#1148412, @steven_wu wrote:

Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?

No effect on importing/symbol resolution etc. In fact, this is not an input to the compiler at all, it is used by the build system. The change in order will just make it look to the build system like it has changed when it hasn't.

I understand. But if the order is used by the build system, it can also affect the output binary. If the only goal is to keep the thin link function import map constant when the input doesn't change, std::map is fine. Can you add a test case for this?

The other option is MapVector, which preserve the insertion order, which might be faster than std::map.

The trickiness is that currently it may not be inserted in a deterministic order, because we use StringMap elsewhere in the thin link. So std::map is the most straightforward way to get the desired behavior.

This is unfortunate. I can't think of anything can be wrong because of that now but might need to revisit.

[tejohnson]

In D48783#1148452, @steven_wu wrote:

In D48783#1148427, @tejohnson wrote:

In D48783#1148412, @steven_wu wrote:

Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?

No effect on importing/symbol resolution etc. In fact, this is not an input to the compiler at all, it is used by the build system. The change in order will just make it look to the build system like it has changed when it hasn't.

I understand. But if the order is used by the build system, it can also affect the output binary.

I'm not sure I understand how - additional ThinLTO backend processes could be unnecessarily redone, but they would have the same inputs and produce the same object file as before.

If the only goal is to keep the thin link function import map constant when the input doesn't change, std::map is fine. Can you add a test case for this?

I'll add a test that emits an imports file with multiple lines and check for them in order - it wouldn't necessarily fail without this change (we could get lucky), but it might catch any regression in this ordering.

The other option is MapVector, which preserve the insertion order, which might be faster than std::map.

The trickiness is that currently it may not be inserted in a deterministic order, because we use StringMap elsewhere in the thin link. So std::map is the most straightforward way to get the desired behavior.

This is unfortunate. I can't think of anything can be wrong because of that now but might need to revisit.

[tejohnson]

Add test with multiple imports files that are expected to be ordered

[tejohnson]

Ping

[steven_wu]

LGTM. Thanks!

[mehdi_amini]

std::map is usually fairly slow, have you considered sorting at the time you write to the file instead? That would add a slow step when writing the files but wouldn't affect any in-memory operation.

[tejohnson]

In D48783#1157681, @mehdi_amini wrote:

std::map is usually fairly slow, have you considered sorting at the time you write to the file instead? That would add a slow step when writing the files but wouldn't affect any in-memory operation.

Actually...looking at the code and what I even wrote here in the description, I'm not sure why it didn't occur to me to simply use the std::map ModuleToSummariesForIndex we are already building for the purposes of writing the distributed index files. At least in the new LTO API, that is done just before calling EmitImportsFiles, so we have it on hand! For the old LTO API, that std::map is not being done yet, but this is just llvm-lto testing code, so I can add a call to the same llvm::gatherImportedSummariesForModule before invoking EmitImportsFiles.

Let me change my patch to do this...

[tejohnson]

Use the existing ModuleToSummariesForIndex instead.

[mehdi_amini]

LGTM.

llvm/trunk/include/llvm/Transforms/IPO/FunctionImport.h

 /// stable order for bitcode emission.
 void gatherImportedSummariesForModule(
     StringRef ModulePath,
     const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
     const FunctionImporter::ImportMapTy &ImportList,
     std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
 
 /// Emit into \p OutputFilename the files module \p ModulePath will import from.
-std::error_code
-EmitImportsFiles(StringRef ModulePath, StringRef OutputFilename,
-                 const FunctionImporter::ImportMapTy &ModuleImports);
+std::error_code EmitImportsFiles(
+    StringRef ModulePath, StringRef OutputFilename,
+    const std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex);
 
 /// Resolve WeakForLinker values in \p TheModule based on the information
 /// recorded in the summaries during global summary-based analysis.
 void thinLTOResolveWeakForLinkerModule(Module &TheModule,
                                        const GVSummaryMapTy &DefinedGlobals);
 
 /// Internalize \p TheModule based on the information recorded in the summaries
 /// during global summary-based analysis.
 void thinLTOInternalizeModule(Module &TheModule,
                               const GVSummaryMapTy &DefinedGlobals);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_IPO_FUNCTIONIMPORT_H

llvm/trunk/lib/LTO/LTO.cpp

Error start(
     std::error_code EC;
     raw_fd_ostream OS(NewModulePath + ".thinlto.bc", EC,
                       sys::fs::OpenFlags::F_None);
     if (EC)
       return errorCodeToError(EC);
     WriteIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex);
 
     if (ShouldEmitImportsFiles) {
-      EC = EmitImportsFiles(ModulePath, NewModulePath + ".imports", ImportList);
+      EC = EmitImportsFiles(ModulePath, NewModulePath + ".imports",
+                            ModuleToSummariesForIndex);
       if (EC)
         return errorCodeToError(EC);
     }
 
     if (OnWrite)
       OnWrite(ModulePath);
     return Error::success();
   }

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

void ThinLTOCodeGenerator::emitImports(StringRef ModulePath,
   Index.collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // Generate import/export list
   StringMap<FunctionImporter::ImportMapTy> ImportLists(ModuleCount);
   StringMap<FunctionImporter::ExportSetTy> ExportLists(ModuleCount);
   ComputeCrossModuleImport(Index, ModuleToDefinedGVSummaries, ImportLists,
                            ExportLists);
 
+  std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
+  llvm::gatherImportedSummariesForModule(ModulePath, ModuleToDefinedGVSummaries,
+                                         ImportLists[ModulePath],
+                                         ModuleToSummariesForIndex);
+
   std::error_code EC;
-  if ((EC = EmitImportsFiles(ModulePath, OutputName, ImportLists[ModulePath])))
+  if ((EC =
+           EmitImportsFiles(ModulePath, OutputName, ModuleToSummariesForIndex)))
     report_fatal_error(Twine("Failed to open ") + OutputName +
                        " to save imports lists\n");
 }
 
 /**
  * Perform internalization. Index is updated to reflect linkage changes.
  */
 void ThinLTOCodeGenerator::internalize(Module &TheModule,

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

for (auto &GI : ILI.second) {
       assert(DS != DefinedGVSummaries.end() &&
              "Expected a defined summary for imported global value");
       SummariesForIndex[GI.first] = DS->second;
     }
   }
 }
 
 /// Emit the files \p ModulePath will import from into \p OutputFilename.
-std::error_code
-llvm::EmitImportsFiles(StringRef ModulePath, StringRef OutputFilename,
-                       const FunctionImporter::ImportMapTy &ModuleImports) {
+std::error_code llvm::EmitImportsFiles(
+    StringRef ModulePath, StringRef OutputFilename,
+    const std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex) {
   std::error_code EC;
   raw_fd_ostream ImportsOS(OutputFilename, EC, sys::fs::OpenFlags::F_None);
   if (EC)
     return EC;
-  for (auto &ILI : ModuleImports)
-    ImportsOS << ILI.first() << "\n";
+  for (auto &ILI : ModuleToSummariesForIndex)
+    // The ModuleToSummariesForIndex map includes an entry for the current
+    // Module (needed for writing out the index files). We don't want to
+    // include it in the imports file, however, so filter it out.
+    if (ILI.first != ModulePath)
+      ImportsOS << ILI.first << "\n";
   return std::error_code();
 }
 
 bool llvm::convertToDeclaration(GlobalValue &GV) {
   LLVM_DEBUG(dbgs() << "Converting to a declaration: `" << GV.getName()
                     << "\n");
   if (Function *F = dyn_cast<Function>(&GV)) {
     F->deleteBody();

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

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

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: opt -module-summary %p/Inputs/emit_imports2.ll -o %t2b.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
+; RUN: llvm-lto -thinlto-action=thinlink -o %t.index.bc %t1.bc %t2.bc %t2b.bc %t3.bc
+; RUN: llvm-lto -thinlto-action=emitimports -thinlto-index %t.index.bc %t1.bc %t2.bc %t2b.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 | count 2
 ; RUN: cat %t1.bc.imports | FileCheck %s --check-prefix=IMPORTS1
 ; IMPORTS1: emit_imports.ll.tmp2.bc
+; IMPORTS1: emit_imports.ll.tmp2b.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: rm -f %t3.bc.thinlto.bc %t3.bc.imports
-; RUN: llvm-lto2 run %t1.bc %t2.bc %t3.bc -o %t.o -save-temps \
+; RUN: llvm-lto2 run %t1.bc %t2.bc %t2b.bc %t3.bc -o %t.o -save-temps \
 ; RUN:     -thinlto-distributed-indexes \
 ; RUN:     -r=%t1.bc,g, \
+; RUN:     -r=%t1.bc,h, \
 ; RUN:     -r=%t1.bc,f,px \
-; RUN:     -r=%t2.bc,g,px
+; RUN:     -r=%t2.bc,g,px \
+; RUN:     -r=%t2b.bc,h,px
 
-; RUN: cat %t1.bc.imports | count 1
+; RUN: cat %t1.bc.imports | count 2
 ; 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(...)
+declare void @h(...)
 
 define void @f() {
 entry:
   call void (...) @g()
+  call void (...) @h()
   ret void
 }