[ThinLTO] Add string saver onto index for value names

[tejohnson]

Summary

Adds a string saver to the ModuleSummaryIndex so it can store value
names in the case of adding a ValueInfo for a GUID when we don't
have the name stored in a Module string table. This is motivated
by the upcoming summary parser patch, where we will read value names
from the summary entry and want to store them, even when a Module
is not available.

Currently this allows us to store the name in the legacy bitcode case,
and I have added a test to show that.

[tejohnson]

I forgot to add, StringSaver is not copyable (due to BumpPtrAllocator reference), therefore this required changes to the Index value on the ModuleSummaryIndexWrapperPass to hold a unique_ptr instead of Optional, so that it could be initialized without a copy.

[tejohnson]

ping

[evgeny777]

lib/Bitcode/Reader/BitcodeReader.cpp

4841

Probably reflect the purpose of this in comment above? Otherwise it's misleading.

[tejohnson]

lib/Bitcode/Reader/BitcodeReader.cpp

4841

Updated comment to reflect the new behavior.

[tejohnson]

Address comment.

[tejohnson]

My patch update accidentally pulled in a separate patch, let me fix that now...

[tejohnson]

Fixed patch and also rebased to HEAD

[tejohnson]

As discussed on D47905, make the allocator and saver Optional. Also revert
change to make the ModuleSummaryIndexWrapperPass a unique_ptr instead of
Optional, as the emplace() method can be used to avoid the issues my change
introduced there.

[dexonsmith]

LGTM.

[pcc]

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

[tejohnson]

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Are you sure that is the case for Alloc? The BumpPtrAllocatorImpl has a number of data members.

[pcc]

In D47842#1142887, @tejohnson wrote:

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Are you sure that is the case for Alloc? The BumpPtrAllocatorImpl has a number of data members.

Yes, the purpose of the AlignedCharArrayUnion in OptionalStorage is to be at least as large and aligned as T. And Optional contains a field of type OptionalStorage.

[dexonsmith]

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

[pcc]

In D47842#1142956, @dexonsmith wrote:

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

It doesn't appear to do that. BumpPtrAllocator's default constructor (http://llvm-cs.pcc.me.uk/include/llvm/Support/Allocator.h#152) uses = default;.

[dexonsmith]

In D47842#1142966, @pcc wrote:

In D47842#1142956, @dexonsmith wrote:

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

It doesn't appear to do that. BumpPtrAllocator's default constructor (http://llvm-cs.pcc.me.uk/include/llvm/Support/Allocator.h#152) uses = default;.

Okay. Then I don't see a benefit of Optional vs just using them directly.

[tejohnson]

In D47842#1142953, @pcc wrote:

In D47842#1142887, @tejohnson wrote:

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Are you sure that is the case for Alloc? The BumpPtrAllocatorImpl has a number of data members.

Yes, the purpose of the AlignedCharArrayUnion in OptionalStorage is to be at least as large and aligned as T. And Optional contains a field of type OptionalStorage.

Ok I see. Per the follow on discussion, I will revert this part of the change before committing.

[tejohnson]

Revert change to make Alloc/Saver Optional.

include/llvm/Analysis/ModuleSummaryAnalysis.h

 //===- ModuleSummaryAnalysis.h - Module summary index builder ---*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 /// \file
 /// This is the interface to build a ModuleSummaryIndex for a module.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ANALYSIS_MODULESUMMARYANALYSIS_H
 #define LLVM_ANALYSIS_MODULESUMMARYANALYSIS_H
 
-#include "llvm/ADT/Optional.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 #include <functional>
 
 namespace llvm {
 
 class BlockFrequencyInfo;

 public:
   using Result = ModuleSummaryIndex;
 
   Result run(Module &M, ModuleAnalysisManager &AM);
 };
 
 /// Legacy wrapper pass to provide the ModuleSummaryIndex object.
 class ModuleSummaryIndexWrapperPass : public ModulePass {
-  Optional<ModuleSummaryIndex> Index;
+  std::unique_ptr<ModuleSummaryIndex> Index;
 
 public:
   static char ID;
 
   ModuleSummaryIndexWrapperPass();
 
   /// Get the index built by pass
   ModuleSummaryIndex &getIndex() { return *Index; }

include/llvm/IR/ModuleSummaryIndex.h

 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Module.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/ScaledNumber.h"
+#include "llvm/Support/StringSaver.h"
 #include <algorithm>
 #include <array>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <map>
 #include <memory>
 #include <set>
private:
   /// If true then we're performing analysis of IR module, filling summary
   /// accordingly. The value of 'false' means we're reading summary from
   /// BC or YAML source. Affects the type of value stored in NameOrGV union
   bool IsAnalysis;
 
   std::set<std::string> CfiFunctionDefs;
   std::set<std::string> CfiFunctionDecls;
 
+  // Used in cases where we want to record the name of a global, but
+  // don't have the string owned elsewhere (e.g. the Strtab on a module).
+  StringSaver Saver;
+  BumpPtrAllocator Alloc;
+
   // YAML I/O support.
   friend yaml::MappingTraits<ModuleSummaryIndex>;
 
   GlobalValueSummaryMapTy::value_type *
   getOrInsertValuePtr(GlobalValue::GUID GUID) {
     return &*GlobalValueMap.emplace(GUID, GlobalValueSummaryInfo(IsAnalysis)).first;
   }
 
 public:
   // See IsAnalysis variable comment.
   ModuleSummaryIndex(bool IsPerformingAnalysis)
-      : IsAnalysis(IsPerformingAnalysis) {}
+      : IsAnalysis(IsPerformingAnalysis), Saver(Alloc) {}
 
   bool isPerformingAnalysis() const { return IsAnalysis; }
 
   gvsummary_iterator begin() { return GlobalValueMap.begin(); }
   const_gvsummary_iterator begin() const { return GlobalValueMap.begin(); }
   gvsummary_iterator end() { return GlobalValueMap.end(); }
   const_gvsummary_iterator end() const { return GlobalValueMap.end(); }
   size_t size() const { return GlobalValueMap.size(); }
ValueInfo getValueInfo(GlobalValue::GUID GUID) const {
     return ValueInfo(IsAnalysis, I == GlobalValueMap.end() ? nullptr : &*I);
   }
 
   /// Return a ValueInfo for \p GUID.
   ValueInfo getOrInsertValueInfo(GlobalValue::GUID GUID) {
     return ValueInfo(IsAnalysis, getOrInsertValuePtr(GUID));
   }
 
+  // Save a string in the Index. Use before passing Name to
+  // getOrInsertValueInfo when the string isn't owned elsewhere (e.g. on the
+  // module's Strtab).
+  StringRef saveString(std::string String) { return Saver.save(String); }
+
   /// Return a ValueInfo for \p GUID setting value \p Name.
   ValueInfo getOrInsertValueInfo(GlobalValue::GUID GUID, StringRef Name) {
     assert(!IsAnalysis);
     auto VP = getOrInsertValuePtr(GUID);
     VP->second.U.Name = Name;
     return ValueInfo(IsAnalysis, VP);
   }
 
public:
   }
 
   std::set<std::string> &cfiFunctionDefs() { return CfiFunctionDefs; }
   const std::set<std::string> &cfiFunctionDefs() const { return CfiFunctionDefs; }
 
   std::set<std::string> &cfiFunctionDecls() { return CfiFunctionDecls; }
   const std::set<std::string> &cfiFunctionDecls() const { return CfiFunctionDecls; }
 
+  /// Add a global value summary for a value.
+  void addGlobalValueSummary(const GlobalValue &GV,
+                             std::unique_ptr<GlobalValueSummary> Summary) {
+    addGlobalValueSummary(getOrInsertValueInfo(&GV), std::move(Summary));
+  }
+
   /// Add a global value summary for a value of the given name.
   void addGlobalValueSummary(StringRef ValueName,
                              std::unique_ptr<GlobalValueSummary> Summary) {
     addGlobalValueSummary(getOrInsertValueInfo(GlobalValue::getGUID(ValueName)),
                           std::move(Summary));
   }
 
   /// Add a global value summary for the given ValueInfo.
GlobalValueSummary *findSummaryInModule(GlobalValue::GUID ValueGUID,
     return Summary->get();
   }
 
   /// Returns the first GlobalValueSummary for \p GV, asserting that there
   /// is only one if \p PerModuleIndex.
   GlobalValueSummary *getGlobalValueSummary(const GlobalValue &GV,
                                             bool PerModuleIndex = true) const {
     assert(GV.hasName() && "Can't get GlobalValueSummary for GV with no name");
-    return getGlobalValueSummary(GlobalValue::getGUID(GV.getName()),
-                                 PerModuleIndex);
+    return getGlobalValueSummary(GV.getGUID(), PerModuleIndex);
   }
 
   /// Returns the first GlobalValueSummary for \p ValueGUID, asserting that
   /// there
   /// is only one if \p PerModuleIndex.
   GlobalValueSummary *getGlobalValueSummary(GlobalValue::GUID ValueGUID,
                                             bool PerModuleIndex = true) const;
 

lib/Analysis/ModuleSummaryAnalysis.cpp

computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
   auto FuncSummary = llvm::make_unique<FunctionSummary>(
       Flags, NumInsts, FunFlags, RefEdges.takeVector(),
       CallGraphEdges.takeVector(), TypeTests.takeVector(),
       TypeTestAssumeVCalls.takeVector(), TypeCheckedLoadVCalls.takeVector(),
       TypeTestAssumeConstVCalls.takeVector(),
       TypeCheckedLoadConstVCalls.takeVector());
   if (NonRenamableLocal)
     CantBePromoted.insert(F.getGUID());
-  Index.addGlobalValueSummary(F.getName(), std::move(FuncSummary));
+  Index.addGlobalValueSummary(F, std::move(FuncSummary));
 }
 
 static void
 computeVariableSummary(ModuleSummaryIndex &Index, const GlobalVariable &V,
                        DenseSet<GlobalValue::GUID> &CantBePromoted) {
   SetVector<ValueInfo> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
   findRefEdges(Index, &V, RefEdges, Visited);
   bool NonRenamableLocal = isNonRenamableLocal(V);
   GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal,
                                     /* Live = */ false, V.isDSOLocal());
   auto GVarSummary =
       llvm::make_unique<GlobalVarSummary>(Flags, RefEdges.takeVector());
   if (NonRenamableLocal)
     CantBePromoted.insert(V.getGUID());
-  Index.addGlobalValueSummary(V.getName(), std::move(GVarSummary));
+  Index.addGlobalValueSummary(V, std::move(GVarSummary));
 }
 
 static void
 computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A,
                     DenseSet<GlobalValue::GUID> &CantBePromoted) {
   bool NonRenamableLocal = isNonRenamableLocal(A);
   GlobalValueSummary::GVFlags Flags(A.getLinkage(), NonRenamableLocal,
                                     /* Live = */ false, A.isDSOLocal());
   auto AS = llvm::make_unique<AliasSummary>(Flags);
   auto *Aliasee = A.getBaseObject();
   auto *AliaseeSummary = Index.getGlobalValueSummary(*Aliasee);
   assert(AliaseeSummary && "Alias expects aliasee summary to be parsed");
   AS->setAliasee(AliaseeSummary);
   if (NonRenamableLocal)
     CantBePromoted.insert(A.getGUID());
-  Index.addGlobalValueSummary(A.getName(), std::move(AS));
+  Index.addGlobalValueSummary(A, std::move(AS));
 }
 
 // Set LiveRoot flag on entries matching the given value name.
 static void setLiveRoot(ModuleSummaryIndex &Index, StringRef Name) {
   if (ValueInfo VI = Index.getValueInfo(GlobalValue::getGUID(Name)))
     for (auto &Summary : VI.getSummaryList())
       Summary->setLive(true);
 }
ModuleSymbolTable::CollectAsmSymbols(
           GlobalValue *GV = M.getNamedValue(Name);
           if (!GV)
             return;
           assert(GV->isDeclaration() && "Def in module asm already has definition");
           GlobalValueSummary::GVFlags GVFlags(GlobalValue::InternalLinkage,
                                               /* NotEligibleToImport = */ true,
                                               /* Live = */ true,
                                               /* Local */ GV->isDSOLocal());
-          CantBePromoted.insert(GlobalValue::getGUID(Name));
+          CantBePromoted.insert(GV->getGUID());
           // Create the appropriate summary type.
           if (Function *F = dyn_cast<Function>(GV)) {
             std::unique_ptr<FunctionSummary> Summary =
                 llvm::make_unique<FunctionSummary>(
                     GVFlags, 0,
                     FunctionSummary::FFlags{
                         F->hasFnAttribute(Attribute::ReadNone),
                         F->hasFnAttribute(Attribute::ReadOnly),
                         F->hasFnAttribute(Attribute::NoRecurse),
                         F->returnDoesNotAlias()},
                     ArrayRef<ValueInfo>{}, ArrayRef<FunctionSummary::EdgeTy>{},
                     ArrayRef<GlobalValue::GUID>{},
                     ArrayRef<FunctionSummary::VFuncId>{},
                     ArrayRef<FunctionSummary::VFuncId>{},
                     ArrayRef<FunctionSummary::ConstVCall>{},
                     ArrayRef<FunctionSummary::ConstVCall>{});
-            Index.addGlobalValueSummary(Name, std::move(Summary));
+            Index.addGlobalValueSummary(*GV, std::move(Summary));
           } else {
             std::unique_ptr<GlobalVarSummary> Summary =
                 llvm::make_unique<GlobalVarSummary>(GVFlags,
                                                     ArrayRef<ValueInfo>{});
-            Index.addGlobalValueSummary(Name, std::move(Summary));
+            Index.addGlobalValueSummary(*GV, std::move(Summary));
           }
         });
   }
 
   // Compute summaries for all functions defined in module, and save in the
   // index.
   for (auto &F : M) {
     if (F.isDeclaration())

 
 ModuleSummaryIndexWrapperPass::ModuleSummaryIndexWrapperPass()
     : ModulePass(ID) {
   initializeModuleSummaryIndexWrapperPassPass(*PassRegistry::getPassRegistry());
 }
 
 bool ModuleSummaryIndexWrapperPass::runOnModule(Module &M) {
   auto &PSI = *getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
-  Index = buildModuleSummaryIndex(
+  Index = llvm::make_unique<ModuleSummaryIndex>(buildModuleSummaryIndex(
       M,
       [this](const Function &F) {
         return &(this->getAnalysis<BlockFrequencyInfoWrapperPass>(
                          *const_cast<Function *>(&F))
                      .getBFI());
       },
-      &PSI);
+      &PSI));
   return false;
 }
 
 bool ModuleSummaryIndexWrapperPass::doFinalization(Module &M) {
   Index.reset();
   return false;
 }
 
 void ModuleSummaryIndexWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();
   AU.addRequired<BlockFrequencyInfoWrapperPass>();
   AU.addRequired<ProfileSummaryInfoWrapperPass>();
 }

lib/Bitcode/Reader/BitcodeReader.cpp

std::string GlobalId =
       GlobalValue::getGlobalIdentifier(ValueName, Linkage, SourceFileName);
   auto ValueGUID = GlobalValue::getGUID(GlobalId);
   auto OriginalNameID = ValueGUID;
   if (GlobalValue::isLocalLinkage(Linkage))
     OriginalNameID = GlobalValue::getGUID(ValueName);
   if (PrintSummaryGUIDs)
     dbgs() << "GUID " << ValueGUID << "(" << OriginalNameID << ") is "
            << ValueName << "\n";
-  
   // UseStrtab is false for legacy summary formats and value names are
-  // created on stack. We can't use them outside of parseValueSymbolTable.
+  // created on stack. In that case we save the name in a string saver in
+  // the index so that the value name can be recorded.
   ValueIdToValueInfoMap[ValueID] = std::make_pair(
-      TheIndex.getOrInsertValueInfo(ValueGUID, UseStrtab ? ValueName : ""),
+      TheIndex.getOrInsertValueInfo(
+          ValueGUID,
+          UseStrtab ? ValueName : TheIndex.saveString(ValueName.str())),
       OriginalNameID);
 }
 
 // Specialized value symbol table parser used when reading module index
 // blocks where we don't actually create global values. The parsed information
 // is saved in the bitcode reader for use when later parsing summaries.
 Error ModuleSummaryIndexBitcodeReader::parseValueSymbolTable(
     uint64_t Offset,

lib/Bitcode/Writer/BitcodeWriter.cpp

void ModuleBitcodeWriterBase::writePerModuleFunctionSummaryRecord(
   NameVals.clear();
 }
 
 // Collect the global value references in the given variable's initializer,
 // and emit them in a summary record.
 void ModuleBitcodeWriterBase::writeModuleLevelReferences(
     const GlobalVariable &V, SmallVector<uint64_t, 64> &NameVals,
     unsigned FSModRefsAbbrev) {
-  auto VI = Index->getValueInfo(GlobalValue::getGUID(V.getName()));
+  auto VI = Index->getValueInfo(V.getGUID());
   if (!VI || VI.getSummaryList().empty()) {
     // Only declarations should not have a summary (a declaration might however
     // have a summary if the def was in module level asm).
     assert(V.isDeclaration());
     return;
   }
   auto *Summary = VI.getSummaryList()[0].get();
   NameVals.push_back(VE.getValueID(&V));
void ModuleBitcodeWriterBase::writePerModuleGlobalValueSummary() {
   // Iterate over the list of functions instead of the Index to
   // ensure the ordering is stable.
   for (const Function &F : M) {
     // Summary emission does not support anonymous functions, they have to
     // renamed using the anonymous function renaming pass.
     if (!F.hasName())
       report_fatal_error("Unexpected anonymous function when writing summary");
 
-    ValueInfo VI = Index->getValueInfo(GlobalValue::getGUID(F.getName()));
+    ValueInfo VI = Index->getValueInfo(F.getGUID());
     if (!VI || VI.getSummaryList().empty()) {
       // Only declarations should not have a summary (a declaration might
       // however have a summary if the def was in module level asm).
       assert(F.isDeclaration());
       continue;
     }
     auto *Summary = VI.getSummaryList()[0].get();
     writePerModuleFunctionSummaryRecord(NameVals, Summary, VE.getValueID(&F),

test/ThinLTO/X86/autoupgrade.ll

 ; Verify that auto-upgrading intrinsics works with Lazy loaded bitcode
 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %p/Inputs/autoupgrade.bc
 
 ; We can't use llvm-dis here, because it would do the autoupgrade itself.
 
 ; RUN: llvm-link  -summary-index=%t3.bc \
 ; RUN:            -import=globalfunc1:%p/Inputs/autoupgrade.bc %t.bc \
 ; RUN:     | llvm-bcanalyzer -dump | FileCheck %s
 
 ; CHECK: <STRTAB_BLOCK
 ; CHECK-NEXT: blob data = 'mainglobalfunc1llvm.invariant.start.p0i8{{.*}}'
 
+; Check that the summary is able to print the names despite the lack of
+; string table in the legacy bitcode.
+; RUN: llvm-dis %p/Inputs/autoupgrade.bc -o - \
+; RUN:     | FileCheck %s --check-prefix=SUMMARYNAMES
+; SUMMARYNAMES: ^2 = gv: (name: "globalfunc2",
+; SUMMARYNAMES: ^3 = gv: (name: "globalfunc1"
+
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 define i32 @main() #0 {
 entry:
   call void (...) @globalfunc1()
   ret i32 0
 }
 
 declare void @globalfunc1(...) #1