[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

4846

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

[tejohnson]

lib/Bitcode/Reader/BitcodeReader.cpp

4846

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/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:
   /// the IR from assembly. The value of 'false' means we're reading summary
   /// from BC or YAML source. Affects the type of value stored in NameOrGV
   /// union.
   bool HaveGVs;
 
   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(HaveGVs))
                  .first;
   }
 
 public:
   // See HaveGVs variable comment.
-  ModuleSummaryIndex(bool HaveGVs) : HaveGVs(HaveGVs) {}
+  ModuleSummaryIndex(bool HaveGVs) : HaveGVs(HaveGVs), Saver(Alloc) {}
 
   bool haveGVs() const { return HaveGVs; }
 
   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(HaveGVs, I == GlobalValueMap.end() ? nullptr : &*I);
   }
 
   /// Return a ValueInfo for \p GUID.
   ValueInfo getOrInsertValueInfo(GlobalValue::GUID GUID) {
     return ValueInfo(HaveGVs, 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(!HaveGVs);
     auto VP = getOrInsertValuePtr(GUID);
     VP->second.U.Name = Name;
     return ValueInfo(HaveGVs, VP);
   }
 

lib/Analysis/ModuleSummaryAnalysis.cpp

 
 ModuleSummaryIndexWrapperPass::ModuleSummaryIndexWrapperPass()
     : ModulePass(ID) {
   initializeModuleSummaryIndexWrapperPassPass(*PassRegistry::getPassRegistry());
 }
 
 bool ModuleSummaryIndexWrapperPass::runOnModule(Module &M) {
   auto &PSI = *getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
-  Index = buildModuleSummaryIndex(
+  Index.emplace(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,

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