[ThinLTO] Assign ValueId only once per GUID when writing combined summary

[tejohnson]

Summary

Minor efficiency improvement when writing combined summaries (e.g. for
distributed back ends). For symbols that had multiple summaries (e.g.
linkonce or weak), we were assigning a value id multiple times. Add a
new helper to walk only the unique GUID in the index, instead of all
summaries.

[pcc]

Have you benchmarked this change? It seems that it wouldn't make a difference for the WriteIndexesThinBackend because it always passes a ModuleToSummariesForIndex map.

[tejohnson]

In D33790#770815, @pcc wrote:

Have you benchmarked this change? It seems that it wouldn't make a difference for the WriteIndexesThinBackend because it always passes a ModuleToSummariesForIndex map.

No and you are right it won't really help in writing distributed indexes (I can fix that in the description). I had been hanging on to this change for a couple days and forgot why I wanted it. It just cleans up something strange I noticed when examining this code - that we will repeatedly assign value numbers to summaries with the same GUID (and you end up with non-contiguous value ids, which isn't a problem, just perhaps looks strange). The change will have more impact when writing out the whole combined index, although we don't do that in any production mode with ThinLTO (distributed or otherwise).

[pcc]

In D33790#770953, @tejohnson wrote:

In D33790#770815, @pcc wrote:

Have you benchmarked this change? It seems that it wouldn't make a difference for the WriteIndexesThinBackend because it always passes a ModuleToSummariesForIndex map.

No and you are right it won't really help in writing distributed indexes (I can fix that in the description). I had been hanging on to this change for a couple days and forgot why I wanted it. It just cleans up something strange I noticed when examining this code - that we will repeatedly assign value numbers to summaries with the same GUID (and you end up with non-contiguous value ids, which isn't a problem, just perhaps looks strange). The change will have more impact when writing out the whole combined index, although we don't do that in any production mode with ThinLTO (distributed or otherwise).

Improving performance of a non-production configuration doesn't seem compelling, to be honest. If you just wanted to address non-contiguous value ids, it seems that this would be simpler and would also address that issue in distributed indexes.

forEachSummary([&](GVInfo I) {
  auto &V = GUIDToValueIdMap[I.first];
  if (!V)
    V = ++GlobalValueId;
});

lib/Bitcode/Writer/BitcodeWriter.cpp

public:
   /// Constructs a IndexBitcodeWriter object for the given combined index,
   /// writing to the provided \p Buffer. When writing a subset of the index
   /// for a distributed backend, provide a \p ModuleToSummariesForIndex map.
   IndexBitcodeWriter(BitstreamWriter &Stream, const ModuleSummaryIndex &Index,
                      const std::map<std::string, GVSummaryMapTy>
                          *ModuleToSummariesForIndex = nullptr)
       : BitcodeWriterBase(Stream), Index(Index),
         ModuleToSummariesForIndex(ModuleToSummariesForIndex) {
-    // Assign unique value ids to all summaries to be written, for use
-    // in writing out the call graph edges. Save the mapping from GUID
+    // Assign unique value ids to all unique GUID with summaries to be written,
+    // for use in writing out the call graph edges. Save the mapping from GUID
     // to the new global value id to use when writing those edges, which
     // are currently saved in the index in terms of GUID.
-    forEachSummary([&](GVInfo I) {
-      GUIDToValueIdMap[I.first] = ++GlobalValueId;
+    forEachGUID([&](GlobalValue::GUID GUID) {
+      GUIDToValueIdMap[GUID] = ++GlobalValueId;
     });
   }
 
   /// The below iterator returns the GUID and associated summary.
   typedef std::pair<GlobalValue::GUID, GlobalValueSummary *> GVInfo;
 
+  /// Calls the callback for each value GUID with a summary to be written to
+  /// bitcode. This hides the details of whether they are being pulled from the
+  /// entire index or just those in a provided ModuleToSummariesForIndex map.
+  void forEachGUID(std::function<void(GlobalValue::GUID)> Callback) {
+    if (ModuleToSummariesForIndex) {
+      for (auto &M : *ModuleToSummariesForIndex)
+        for (auto &Summary : M.second)
+          Callback(Summary.first);
+    } else {
+      for (auto &Summaries : Index)
+        if (Summaries.second.SummaryList.size())
+          Callback(Summaries.first);
+    }
+  }
+
   /// Calls the callback for each value GUID and summary to be written to
   /// bitcode. This hides the details of whether they are being pulled from the
   /// entire index or just those in a provided ModuleToSummariesForIndex map.
   void forEachSummary(std::function<void(GVInfo)> Callback) {
     if (ModuleToSummariesForIndex) {
       for (auto &M : *ModuleToSummariesForIndex)
         for (auto &Summary : M.second)
           Callback(Summary);

test/Bitcode/Inputs/thinlto-function-summary-callgraph-linkonce.ll

+; ModuleID = 'thinlto-function-summary-callgraph-linkonce2.ll'
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define linkonce_odr void @func() #0 {
+entry:
+    ret void
+}

test/Bitcode/thinlto-function-summary-callgraph-linkonce.ll

+; Test to check for correct value numbering when there are multiple copies of a
+; (linkonce) function.
+; RUN: opt -module-summary %s -o %t.o
+; RUN: opt -module-summary %p/Inputs/thinlto-function-summary-callgraph-linkonce.ll -o %t2.o
+; RUN: llvm-lto -thinlto -o %t3 %t.o %t2.o
+; RUN: llvm-bcanalyzer -dump %t3.thinlto.bc | FileCheck %s --check-prefix=COMBINED
+
+; COMBINED:       <GLOBALVAL_SUMMARY_BLOCK
+; COMBINED-NEXT:    <VERSION
+; func:
+; COMBINED-NEXT:    <VALUE_GUID op0=1 op1=7289175272376759421/>
+; main:
+; COMBINED-NEXT:    <VALUE_GUID op0=2
+; Two summaries for func:
+; COMBINED-NEXT:    <COMBINED {{.*}} op0=1
+; COMBINED-NEXT:    <COMBINED {{.*}} op0=1
+; main should have an edge to func:
+; COMBINED-NEXT:    <COMBINED {{.*}} op5=1/>
+; COMBINED-NEXT:  </GLOBALVAL_SUMMARY_BLOCK>
+
+; ModuleID = 'thinlto-function-summary-callgraph-linkonce.ll'
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define linkonce_odr void @func() #0 {
+entry:
+      ret void
+}
+
+; Function Attrs: nounwind uwtable
+define void @main() #0 {
+entry:
+    call void () @func()
+    ret void
+}