Prototype: Reduce llvm-profdata merge memory usage further

[dblaikie]

Summary

The InstrProfWriter already stores the name and hash of the record in
the nested maps it uses for lookup while merging - this data is
duplicated in the value within the maps.

Refactor the InstrProfRecord to use a nested struct for the counters
themselves so that InstrProfWriter can use this nested struct alone
without the name or hash duplicated there.

This work is incomplete, but enough to demonstrate the value (around a
50% decrease in memory usage for a large test case (10GB -> 5GB)).
Though most of that decrease is probably from removing the
SoftInstrProfError as well, but I haven't implemented a replacement for
it yet. (it needs to go with the counters, because the operations on the
counters - merging, etc, are where the failures are - unlike the
name/hash which are totally unused by those counter-related operations
and thus easy to split out)

Ongoing discussion about removing SoftInstrProfError as a field of the
InstrProfRecord is happening on the thread that added it - including
the possibility of moving back towards an earlier version of that
proposed patch that passed SoftInstrProfError through the various APIs,
rather than as a member of InstrProfRecord.

[dblaikie]

I should say this doesn't currently pass the llvm-profdata regression tests - in part because it totally breaks/ignores the error handling (as mentioned in the description, there's ongoing list discussion about how to handle/address that). But enough of a rough prototype to handle some review feedback like naming, etc, I think. I haven't done a byte-for-byte comparison, but it seems to do similar work to the ToT version in my (error-free) test case.

[davidxl]

include/llvm/ProfileData/InstrProf.h

600 ↗

(On Diff #104748)

nit: maybe just Counters? Holder does not provide additional information.

lib/ProfileData/InstrProf.cpp

504 ↗

(On Diff #104748)

Keep this until the discussion about its fate is resolved.

lib/ProfileData/InstrProfWriter.cpp

152 ↗

(On Diff #104748)

Summary Builder does not need name, hash etc. so perfhaps fixing API is the right way.

[dblaikie]

Address code review feedback, including leaving the error handling in, fixing the SummaryBuilder API, and fixing a few bugs I hadn't addressed - check-all now works (& I have a patch for the clang API fixes needed).

[davidxl]

Instead of exposing Counter members widely (and fixing tests), it is better to add wrapper methods in InstrProfRecord to forward the call to the nested member struct.

[davidxl]

In summary, I think this patch should be minimized by

1. sinking the minimal number of interfaces into Counter subclass
2. for any sunk interface, provide a wrapper in the parent class InstrProfRecord
3. avoid exposing internal members to client side other than InstrProfWriters (may be making those method private and making Writer a friend ?)
4. split out unrelated changes

include/llvm/ProfileData/InstrProf.h

253 ↗

(On Diff #104783)

unrelated format change?

701 ↗

(On Diff #104783)

format change?

include/llvm/ProfileData/InstrProfData.inc

317 ↗

(On Diff #104783)

Why this change? IMO, we should reduce exposing CounterHolder if possible and maintain/use APIs at the InstrProfRecord level.

361 ↗

(On Diff #104783)

Same here.

530 ↗

(On Diff #104783)

Can this be split into different patch?

include/llvm/ProfileData/InstrProfWriter.h

66 ↗

(On Diff #104783)

There is no need to change this interface. You are passing name, hash etc, so might as well create a InstrProfRecord.

lib/ProfileData/InstrProf.cpp

641 ↗

(On Diff #104783)

These naming changes are not necessary

651 ↗

(On Diff #104783)

The interface change here is not necessary.

lib/ProfileData/InstrProfWriter.cpp

152 ↗

(On Diff #104748)

On second thought, to reduce exposing implementation details and minimizing interface changes, it is cleaner to just do

SummaryBuilder->addRecord(InstrProfRecord(K, ProfileData.first, Counters));

Similarly for the SerializeFrom call below and ValueProfData::getSize() call above.

[dblaikie]

Try inverting the API change - move the name/hash out into NamedInstrProfRecord

[davidxl]

Limit NamedInstrProfRecord to the reader file seems ok -- but this class also looks like a useful convenience class. We can probably discuss that in a different thread if it is desirable.

lib/ProfileData/InstrProfWriter.cpp

180 ↗

(On Diff #105374)

I don't find the comment here helping. Perhaps just mention the move construction happens later when assigned to the Dest?

[dblaikie]

Refactor code accessing a potentially std::move'd object to avoid the need for a comment.

[davidxl]

lgtm

llvm/trunk/include/llvm/ProfileData/InstrProf.h

 /// Annotate up to \p MaxMDCount (default 3) number of records per value site.
 void annotateValueSite(Module &M, Instruction &Inst,
                        const InstrProfRecord &InstrProfR,
                        InstrProfValueKind ValueKind, uint32_t SiteIndx,
                        uint32_t MaxMDCount = 3);
 
 /// Same as the above interface but using an ArrayRef, as well as \p Sum.
 void annotateValueSite(Module &M, Instruction &Inst,
-                       ArrayRef<InstrProfValueData> VDs,
-                       uint64_t Sum, InstrProfValueKind ValueKind,
-                       uint32_t MaxMDCount);
+                       ArrayRef<InstrProfValueData> VDs, uint64_t Sum,
+                       InstrProfValueKind ValueKind, uint32_t MaxMDCount);
 
 /// Extract the value profile data from \p Inst which is annotated with
 /// value profile meta data. Return false if there is no value data annotated,
 /// otherwise  return true.
 bool getValueProfDataFromInst(const Instruction &Inst,
                               InstrProfValueKind ValueKind,
                               uint32_t MaxNumValueData,
                               InstrProfValueData ValueData[],
struct InstrProfValueSiteRecord {
   void merge(SoftInstrProfErrors &SIPE, InstrProfValueSiteRecord &Input,
              uint64_t Weight = 1);
   /// Scale up value profile data counts.
   void scale(SoftInstrProfErrors &SIPE, uint64_t Weight);
 };
 
 /// Profiling information for a single function.
 struct InstrProfRecord {
-  StringRef Name;
-  uint64_t Hash;
   std::vector<uint64_t> Counts;
   SoftInstrProfErrors SIPE;
 
   InstrProfRecord() = default;
-  InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t> Counts)
-      : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}
+  InstrProfRecord(std::vector<uint64_t> Counts) : Counts(std::move(Counts)) {}
   InstrProfRecord(InstrProfRecord &&) = default;
   InstrProfRecord(const InstrProfRecord &RHS)
-      : Name(RHS.Name), Hash(RHS.Hash), Counts(RHS.Counts), SIPE(RHS.SIPE),
+      : Counts(RHS.Counts),
         ValueData(RHS.ValueData
                       ? llvm::make_unique<ValueProfData>(*RHS.ValueData)
                       : nullptr) {}
   InstrProfRecord &operator=(InstrProfRecord &&) = default;
   InstrProfRecord &operator=(const InstrProfRecord &RHS) {
-    Name = RHS.Name;
-    Hash = RHS.Hash;
     Counts = RHS.Counts;
-    SIPE = RHS.SIPE;
     if (!RHS.ValueData) {
       ValueData = nullptr;
       return *this;
     }
     if (!ValueData)
       ValueData = llvm::make_unique<ValueProfData>(*RHS.ValueData);
     else
       *ValueData = *RHS.ValueData;
     return *this;
   }
 
   using ValueMapType = std::vector<std::pair<uint64_t, uint64_t>>;
 
   /// Return the number of value profile kinds with non-zero number
   /// of profile sites.
   inline uint32_t getNumValueKinds() const;
-
   /// Return the number of instrumented sites for ValueKind.
   inline uint32_t getNumValueSites(uint32_t ValueKind) const;
 
   /// Return the total number of ValueData for ValueKind.
   inline uint32_t getNumValueData(uint32_t ValueKind) const;
 
   /// Return the number of value data collected for ValueKind at profiling
   /// site: Site.
private:
   // Scale merged value counts by \p Weight.
   void mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
                           uint64_t Weight);
 
   // Scale up value profile data count.
   void scaleValueProfData(uint32_t ValueKind, uint64_t Weight);
 };
 
+struct NamedInstrProfRecord : InstrProfRecord {
+  StringRef Name;
+  uint64_t Hash;
+
+  NamedInstrProfRecord() = default;
+  NamedInstrProfRecord(StringRef Name, uint64_t Hash,
+                       std::vector<uint64_t> Counts)
+      : InstrProfRecord(std::move(Counts)), Name(Name), Hash(Hash) {}
+};
+
 uint32_t InstrProfRecord::getNumValueKinds() const {
   uint32_t NumValueKinds = 0;
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     NumValueKinds += !(getValueSitesForKind(Kind).empty());
   return NumValueKinds;
 }
 
 uint32_t InstrProfRecord::getNumValueData(uint32_t ValueKind) const {

llvm/trunk/include/llvm/ProfileData/InstrProfReader.h

 #include <vector>
 
 namespace llvm {
 
 class InstrProfReader;
 
 /// A file format agnostic iterator over profiling data.
 class InstrProfIterator : public std::iterator<std::input_iterator_tag,
-                                               InstrProfRecord> {
+                                               NamedInstrProfRecord> {
   InstrProfReader *Reader = nullptr;
-  InstrProfRecord Record;
+  value_type Record;
 
   void Increment();
 
 public:
   InstrProfIterator() = default;
   InstrProfIterator(InstrProfReader *Reader) : Reader(Reader) { Increment(); }
 
   InstrProfIterator &operator++() { Increment(); return *this; }
   bool operator==(const InstrProfIterator &RHS) { return Reader == RHS.Reader; }
   bool operator!=(const InstrProfIterator &RHS) { return Reader != RHS.Reader; }
-  InstrProfRecord &operator*() { return Record; }
-  InstrProfRecord *operator->() { return &Record; }
+  value_type &operator*() { return Record; }
+  value_type *operator->() { return &Record; }
 };
 
 /// Base class and interface for reading profiling data of any known instrprof
-/// format. Provides an iterator over InstrProfRecords.
+/// format. Provides an iterator over NamedInstrProfRecords.
 class InstrProfReader {
   instrprof_error LastError = instrprof_error::success;
 
 public:
   InstrProfReader() = default;
   virtual ~InstrProfReader() = default;
 
   /// Read the header.  Required before reading first record.
   virtual Error readHeader() = 0;
 
   /// Read a single record.
-  virtual Error readNextRecord(InstrProfRecord &Record) = 0;
+  virtual Error readNextRecord(NamedInstrProfRecord &Record) = 0;
 
   /// Iterator over profile data.
   InstrProfIterator begin() { return InstrProfIterator(this); }
   InstrProfIterator end() { return InstrProfIterator(); }
 
   virtual bool isIRLevelProfile() const = 0;
 
   /// Return the PGO symtab. There are three different readers:
public:
   static bool hasFormat(const MemoryBuffer &Buffer);
 
   bool isIRLevelProfile() const override { return IsIRLevelProfile; }
 
   /// Read the header.
   Error readHeader() override;
 
   /// Read a single record.
-  Error readNextRecord(InstrProfRecord &Record) override;
+  Error readNextRecord(NamedInstrProfRecord &Record) override;
 
   InstrProfSymtab &getSymtab() override {
     assert(Symtab.get());
     return *Symtab.get();
   }
 };
 
 /// Reader for the raw instrprof binary format from runtime.

 public:
   RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)
       : DataBuffer(std::move(DataBuffer)) {}
   RawInstrProfReader(const RawInstrProfReader &) = delete;
   RawInstrProfReader &operator=(const RawInstrProfReader &) = delete;
 
   static bool hasFormat(const MemoryBuffer &DataBuffer);
   Error readHeader() override;
-  Error readNextRecord(InstrProfRecord &Record) override;
+  Error readNextRecord(NamedInstrProfRecord &Record) override;
 
   bool isIRLevelProfile() const override {
     return (Version & VARIANT_MASK_IR_PROF) != 0;
   }
 
   InstrProfSymtab &getSymtab() override {
     assert(Symtab.get());
     return *Symtab.get();
support::endianness getDataEndianness() const {
     else
       return support::little;
   }
 
   inline uint8_t getNumPaddingBytes(uint64_t SizeInBytes) {
     return 7 & (sizeof(uint64_t) - SizeInBytes % sizeof(uint64_t));
   }
 
-  Error readName(InstrProfRecord &Record);
-  Error readFuncHash(InstrProfRecord &Record);
+  Error readName(NamedInstrProfRecord &Record);
+  Error readFuncHash(NamedInstrProfRecord &Record);
   Error readRawCounts(InstrProfRecord &Record);
   Error readValueProfilingData(InstrProfRecord &Record);
   bool atEnd() const { return Data == DataEnd; }
 
   void advanceData() {
     Data++;
     ValueDataStart += CurValueDataSize;
   }

 
 enum class HashT : uint32_t;
 
 } // end namespace IndexedInstrProf
 
 /// Trait for lookups into the on-disk hash table for the binary instrprof
 /// format.
 class InstrProfLookupTrait {
-  std::vector<InstrProfRecord> DataBuffer;
+  std::vector<NamedInstrProfRecord> DataBuffer;
   IndexedInstrProf::HashT HashType;
   unsigned FormatVersion;
   // Endianness of the input value profile data.
   // It should be LE by default, but can be changed
   // for testing purpose.
   support::endianness ValueProfDataEndianness = support::little;
 
 public:
   InstrProfLookupTrait(IndexedInstrProf::HashT HashType, unsigned FormatVersion)
       : HashType(HashType), FormatVersion(FormatVersion) {}
 
-  using data_type = ArrayRef<InstrProfRecord>;
+  using data_type = ArrayRef<NamedInstrProfRecord>;
 
   using internal_key_type = StringRef;
   using external_key_type = StringRef;
   using hash_value_type = uint64_t;
   using offset_type = uint64_t;
 
   static bool EqualKey(StringRef A, StringRef B) { return A == B; }
   static StringRef GetInternalKey(StringRef K) { return K; }
public:
   }
 };
 
 struct InstrProfReaderIndexBase {
   virtual ~InstrProfReaderIndexBase() = default;
 
   // Read all the profile records with the same key pointed to the current
   // iterator.
-  virtual Error getRecords(ArrayRef<InstrProfRecord> &Data) = 0;
+  virtual Error getRecords(ArrayRef<NamedInstrProfRecord> &Data) = 0;
 
   // Read all the profile records with the key equal to FuncName
   virtual Error getRecords(StringRef FuncName,
-                                     ArrayRef<InstrProfRecord> &Data) = 0;
+                                     ArrayRef<NamedInstrProfRecord> &Data) = 0;
   virtual void advanceToNextKey() = 0;
   virtual bool atEnd() const = 0;
   virtual void setValueProfDataEndianness(support::endianness Endianness) = 0;
   virtual uint64_t getVersion() const = 0;
   virtual bool isIRLevelProfile() const = 0;
   virtual Error populateSymtab(InstrProfSymtab &) = 0;
 };
 

 
 public:
   InstrProfReaderIndex(const unsigned char *Buckets,
                        const unsigned char *const Payload,
                        const unsigned char *const Base,
                        IndexedInstrProf::HashT HashType, uint64_t Version);
   ~InstrProfReaderIndex() override = default;
 
-  Error getRecords(ArrayRef<InstrProfRecord> &Data) override;
+  Error getRecords(ArrayRef<NamedInstrProfRecord> &Data) override;
   Error getRecords(StringRef FuncName,
-                   ArrayRef<InstrProfRecord> &Data) override;
+                   ArrayRef<NamedInstrProfRecord> &Data) override;
   void advanceToNextKey() override { RecordIterator++; }
 
   bool atEnd() const override {
     return RecordIterator == HashTable->data_end();
   }
 
   void setValueProfDataEndianness(support::endianness Endianness) override {
     HashTable->getInfoObj().setValueProfDataEndianness(Endianness);
public:
   bool isIRLevelProfile() const override { return Index->isIRLevelProfile(); }
 
   /// Return true if the given buffer is in an indexed instrprof format.
   static bool hasFormat(const MemoryBuffer &DataBuffer);
 
   /// Read the file header.
   Error readHeader() override;
   /// Read a single record.
-  Error readNextRecord(InstrProfRecord &Record) override;
+  Error readNextRecord(NamedInstrProfRecord &Record) override;
 
-  /// Return the pointer to InstrProfRecord associated with FuncName
-  /// and FuncHash
+  /// Return the NamedInstrProfRecord associated with FuncName and FuncHash
   Expected<InstrProfRecord> getInstrProfRecord(StringRef FuncName,
                                                uint64_t FuncHash);
 
   /// Fill Counts with the profile data for the given function name.
   Error getFunctionCounts(StringRef FuncName, uint64_t FuncHash,
                           std::vector<uint64_t> &Counts);
 
   /// Return the maximum of all known function counts.

llvm/trunk/include/llvm/ProfileData/InstrProfWriter.h

 
 /// Writer for instrumentation based profile data.
 class InstrProfRecordWriterTrait;
 class ProfOStream;
 class raw_fd_ostream;
 
 class InstrProfWriter {
 public:
-  using ProfilingData = SmallDenseMap<uint64_t, InstrProfRecord, 1>;
+  using ProfilingData =
+      SmallDenseMap<uint64_t, InstrProfRecord, 1>;
   enum ProfKind { PF_Unknown = 0, PF_FE, PF_IRLevel };
 
 private:
   bool Sparse;
   StringMap<ProfilingData> FunctionData;
   ProfKind ProfileKind = PF_Unknown;
   // Use raw pointer here for the incomplete type object.
   InstrProfRecordWriterTrait *InfoObj;
 
 public:
   InstrProfWriter(bool Sparse = false);
   ~InstrProfWriter();
 
   /// Add function counts for the given function. If there are already counts
   /// for this function and the hash and number of counts match, each counter is
   /// summed. Optionally scale counts by \p Weight.
-  Error addRecord(InstrProfRecord &&I, uint64_t Weight = 1);
+  Error addRecord(NamedInstrProfRecord &&I, uint64_t Weight = 1);
 
   /// Merge existing function counts from the given writer.
   Error mergeRecordsFromWriter(InstrProfWriter &&IPW);
 
   /// Write the profile to \c OS
   void write(raw_fd_ostream &OS);
 
   /// Write the profile in text format to \c OS
   Error writeText(raw_fd_ostream &OS);
 
   /// Write \c Record in text format to \c OS
-  static void writeRecordInText(const InstrProfRecord &Record,
+  static void writeRecordInText(StringRef Name, uint64_t Hash,
+                                const InstrProfRecord &Counters,
                                 InstrProfSymtab &Symtab, raw_fd_ostream &OS);
 
   /// Write the profile, returning the raw data. For testing.
   std::unique_ptr<MemoryBuffer> writeBuffer();
 
   /// Set the ProfileKind. Report error if mixing FE and IR level profiles.
   Error setIsIRLevelProfile(bool IsIRLevel) {
     if (ProfileKind == PF_Unknown) {
       ProfileKind = IsIRLevel ? PF_IRLevel: PF_FE;
       return Error::success();
     }
     return (IsIRLevel == (ProfileKind == PF_IRLevel))
                ? Error::success()
                : make_error<InstrProfError>(
                      instrprof_error::unsupported_version);
   }
 
   // Internal interface for testing purpose only.
   void setValueProfDataEndianness(support::endianness Endianness);
   void setOutputSparse(bool Sparse);
 
 private:
+  Error addRecord(StringRef Name, uint64_t Hash, InstrProfRecord &&I,
+                  uint64_t Weight = 1);
   bool shouldEncodeData(const ProfilingData &PD);
   void writeImpl(ProfOStream &OS);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_PROFILEDATA_INSTRPROFWRITER_H

llvm/trunk/include/llvm/ProfileData/ProfileCommon.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_PROFILEDATA_PROFILECOMMON_H
 #define LLVM_PROFILEDATA_PROFILECOMMON_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/IR/ProfileSummary.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/Error.h"
 #include <algorithm>
 #include <cstdint>
 #include <functional>
 #include <map>
 #include <memory>
 #include <vector>
 
 namespace llvm {
 
-struct InstrProfRecord;
-
 namespace sampleprof {
 
 class FunctionSamples;
 
 } // end namespace sampleprof
 
 inline const char *getHotSectionPrefix() { return ".hot"; }
 inline const char *getUnlikelySectionPrefix() { return ".unlikely"; }

llvm/trunk/lib/ProfileData/InstrProfReader.cpp

#define VP_READ_ADVANCE(Val)                                                   \
   }
   return success();
 
 #undef CHECK_LINE_END
 #undef READ_NUM
 #undef VP_READ_ADVANCE
 }
 
-Error TextInstrProfReader::readNextRecord(InstrProfRecord &Record) {
+Error TextInstrProfReader::readNextRecord(NamedInstrProfRecord &Record) {
   // Skip empty lines and comments.
   while (!Line.is_at_end() && (Line->empty() || Line->startswith("#")))
     ++Line;
   // If we hit EOF while looking for a name, we're done.
   if (Line.is_at_end()) {
     Symtab->finalizeSymtab();
     return error(instrprof_error::eof);
   }
Error RawInstrProfReader<IntPtrT>::readHeader(
   if (Error E = createSymtab(*NewSymtab.get()))
     return E;
 
   Symtab = std::move(NewSymtab);
   return success();
 }
 
 template <class IntPtrT>
-Error RawInstrProfReader<IntPtrT>::readName(InstrProfRecord &Record) {
+Error RawInstrProfReader<IntPtrT>::readName(NamedInstrProfRecord &Record) {
   Record.Name = getName(Data->NameRef);
   return success();
 }
 
 template <class IntPtrT>
-Error RawInstrProfReader<IntPtrT>::readFuncHash(InstrProfRecord &Record) {
+Error RawInstrProfReader<IntPtrT>::readFuncHash(NamedInstrProfRecord &Record) {
   Record.Hash = swap(Data->FuncHash);
   return success();
 }
 
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readRawCounts(
     InstrProfRecord &Record) {
   uint32_t NumCounters = swap(Data->NumCounters);
Error RawInstrProfReader<IntPtrT>::readValueProfilingData(
   // indirect call targets.  The function pointers from the raw profile are
   // remapped into function name hashes.
   VDataPtrOrErr.get()->deserializeTo(Record, &Symtab->getAddrHashMap());
   CurValueDataSize = VDataPtrOrErr.get()->getSize();
   return success();
 }
 
 template <class IntPtrT>
-Error RawInstrProfReader<IntPtrT>::readNextRecord(InstrProfRecord &Record) {
+Error RawInstrProfReader<IntPtrT>::readNextRecord(NamedInstrProfRecord &Record) {
   if (atEnd())
     // At this point, ValueDataStart field points to the next header.
     if (Error E = readNextHeader(getNextHeaderPos()))
       return E;
 
   // Read name ad set it in Record.
   if (Error E = readName(Record))
     return E;
if (GET_VERSION(FormatVersion) > IndexedInstrProf::ProfVersion::Version2 &&
       return data_type();
     }
   }
   return DataBuffer;
 }
 
 template <typename HashTableImpl>
 Error InstrProfReaderIndex<HashTableImpl>::getRecords(
-    StringRef FuncName, ArrayRef<InstrProfRecord> &Data) {
+    StringRef FuncName, ArrayRef<NamedInstrProfRecord> &Data) {
   auto Iter = HashTable->find(FuncName);
   if (Iter == HashTable->end())
     return make_error<InstrProfError>(instrprof_error::unknown_function);
 
   Data = (*Iter);
   if (Data.empty())
     return make_error<InstrProfError>(instrprof_error::malformed);
 
   return Error::success();
 }
 
 template <typename HashTableImpl>
 Error InstrProfReaderIndex<HashTableImpl>::getRecords(
-    ArrayRef<InstrProfRecord> &Data) {
+    ArrayRef<NamedInstrProfRecord> &Data) {
   if (atEnd())
     return make_error<InstrProfError>(instrprof_error::eof);
 
   Data = *RecordIterator;
 
   if (Data.empty())
     return make_error<InstrProfError>(instrprof_error::malformed);
 
this->Summary = llvm::make_unique<ProfileSummary>(
         SummaryData->get(Summary::TotalNumFunctions));
     return Cur + SummarySize;
   } else {
     // For older version of profile data, we need to compute on the fly:
     using namespace IndexedInstrProf;
 
     InstrProfSummaryBuilder Builder(ProfileSummaryBuilder::DefaultCutoffs);
     // FIXME: This only computes an empty summary. Need to call addRecord for
-    // all InstrProfRecords to get the correct summary.
+    // all NamedInstrProfRecords to get the correct summary.
     this->Summary = Builder.getSummary();
     return Cur;
   }
 }
 
 Error IndexedInstrProfReader::readHeader() {
   using namespace support;
 
InstrProfSymtab &IndexedInstrProfReader::getSymtab() {
 
   Symtab = std::move(NewSymtab);
   return *Symtab.get();
 }
 
 Expected<InstrProfRecord>
 IndexedInstrProfReader::getInstrProfRecord(StringRef FuncName,
                                            uint64_t FuncHash) {
-  ArrayRef<InstrProfRecord> Data;
+  ArrayRef<NamedInstrProfRecord> Data;
   Error Err = Index->getRecords(FuncName, Data);
   if (Err)
     return std::move(Err);
   // Found it. Look for counters with the right hash.
   for (unsigned I = 0, E = Data.size(); I < E; ++I) {
     // Check for a match and fill the vector if there is one.
     if (Data[I].Hash == FuncHash) {
       return std::move(Data[I]);
     }
   }
   return error(instrprof_error::hash_mismatch);
 }
 
 Error IndexedInstrProfReader::getFunctionCounts(StringRef FuncName,
                                                 uint64_t FuncHash,
                                                 std::vector<uint64_t> &Counts) {
   Expected<InstrProfRecord> Record = getInstrProfRecord(FuncName, FuncHash);
   if (Error E = Record.takeError())
     return error(std::move(E));
 
   Counts = Record.get().Counts;
   return success();
 }
 
-Error IndexedInstrProfReader::readNextRecord(InstrProfRecord &Record) {
+Error IndexedInstrProfReader::readNextRecord(NamedInstrProfRecord &Record) {
   static unsigned RecordIndex = 0;
 
-  ArrayRef<InstrProfRecord> Data;
+  ArrayRef<NamedInstrProfRecord> Data;
 
   Error E = Index->getRecords(Data);
   if (E)
     return error(std::move(E));
 
   Record = Data[RecordIndex++];
   if (RecordIndex >= Data.size()) {
     Index->advanceToNextKey();
     RecordIndex = 0;
   }
   return success();
 }

llvm/trunk/lib/ProfileData/InstrProfWriter.cpp

void InstrProfWriter::setValueProfDataEndianness(
     support::endianness Endianness) {
   InfoObj->ValueProfDataEndianness = Endianness;
 }
 
 void InstrProfWriter::setOutputSparse(bool Sparse) {
   this->Sparse = Sparse;
 }
 
-Error InstrProfWriter::addRecord(InstrProfRecord &&I, uint64_t Weight) {
-  auto &ProfileDataMap = FunctionData[I.Name];
+Error InstrProfWriter::addRecord(NamedInstrProfRecord &&I, uint64_t Weight) {
+  auto Name = I.Name;
+  auto Hash = I.Hash;
+  return addRecord(Name, Hash, std::move(I), Weight);
+}
+
+Error InstrProfWriter::addRecord(StringRef Name, uint64_t Hash,
+                                 InstrProfRecord &&I, uint64_t Weight) {
+  auto &ProfileDataMap = FunctionData[Name];
 
   bool NewFunc;
   ProfilingData::iterator Where;
   std::tie(Where, NewFunc) =
-      ProfileDataMap.insert(std::make_pair(I.Hash, InstrProfRecord()));
+      ProfileDataMap.insert(std::make_pair(Hash, InstrProfRecord()));
   InstrProfRecord &Dest = Where->second;
 
   if (NewFunc) {
     // We've never seen a function with this name and hash, add it.
     Dest = std::move(I);
-    // Fix up the name to avoid dangling reference.
-    Dest.Name = FunctionData.find(Dest.Name)->getKey();
     if (Weight > 1)
       Dest.scale(Weight);
   } else {
     // We're updating a function we've seen before.
     Dest.merge(I, Weight);
   }
 
   Dest.sortValueData();
 
   return Dest.takeError();
 }
 
 Error InstrProfWriter::mergeRecordsFromWriter(InstrProfWriter &&IPW) {
   for (auto &I : IPW.FunctionData)
     for (auto &Func : I.getValue())
-      if (Error E = addRecord(std::move(Func.second), 1))
+      if (Error E = addRecord(I.getKey(), Func.first, std::move(Func.second)))
         return E;
   return Error::success();
 }
 
 bool InstrProfWriter::shouldEncodeData(const ProfilingData &PD) {
   if (!Sparse)
     return true;
   for (const auto &Func : PD) {
std::unique_ptr<MemoryBuffer> InstrProfWriter::writeBuffer() {
   return MemoryBuffer::getMemBufferCopy(Data);
 }
 
 static const char *ValueProfKindStr[] = {
 #define VALUE_PROF_KIND(Enumerator, Value) #Enumerator,
 #include "llvm/ProfileData/InstrProfData.inc"
 };
 
-void InstrProfWriter::writeRecordInText(const InstrProfRecord &Func,
+void InstrProfWriter::writeRecordInText(StringRef Name, uint64_t Hash,
+                                        const InstrProfRecord &Func,
                                         InstrProfSymtab &Symtab,
                                         raw_fd_ostream &OS) {
-  OS << Func.Name << "\n";
-  OS << "# Func Hash:\n" << Func.Hash << "\n";
+  OS << Name << "\n";
+  OS << "# Func Hash:\n" << Hash << "\n";
   OS << "# Num Counters:\n" << Func.Counts.size() << "\n";
   OS << "# Counter Values:\n";
   for (uint64_t Count : Func.Counts)
     OS << Count << "\n";
 
   uint32_t NumValueKinds = Func.getNumValueKinds();
   if (!NumValueKinds) {
     OS << "\n";
for (const auto &I : FunctionData)
     if (shouldEncodeData(I.getValue()))
       if (Error E = Symtab.addFuncName(I.getKey()))
         return E;
   Symtab.finalizeSymtab();
 
   for (const auto &I : FunctionData)
     if (shouldEncodeData(I.getValue()))
       for (const auto &Func : I.getValue())
-        writeRecordInText(Func.second, Symtab, OS);
+        writeRecordInText(I.getKey(), Func.first, Func.second, Symtab, OS);
   return Error::success();
 }

llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp

for (const auto &Func : *Reader) {
     bool Show =
         ShowAllFunctions || (!ShowFunction.empty() &&
                              Func.Name.find(ShowFunction) != Func.Name.npos);
 
     bool doTextFormatDump = (Show && ShowCounts && TextFormat);
 
     if (doTextFormatDump) {
       InstrProfSymtab &Symtab = Reader->getSymtab();
-      InstrProfWriter::writeRecordInText(Func, Symtab, OS);
+      InstrProfWriter::writeRecordInText(Func.Name, Func.Hash, Func, Symtab,
+                                         OS);
       continue;
     }
 
     assert(Func.Counts.size() > 0 && "function missing entry counter");
     Builder.addRecord(Func);
 
     if (Show) {
 

llvm/trunk/unittests/ProfileData/InstrProfTest.cpp

 static const char callee1[] = "callee1";
 static const char callee2[] = "callee2";
 static const char callee3[] = "callee3";
 static const char callee4[] = "callee4";
 static const char callee5[] = "callee5";
 static const char callee6[] = "callee6";
 
 TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write) {
-  InstrProfRecord Record1("caller", 0x1234, {1, 2});
+  NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
 
   // 4 value sites.
   Record1.reserveSites(IPVK_IndirectCallTarget, 4);
   InstrProfValueData VD0[] = {
       {(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
   Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
   // No value profile data at the second site.
   Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write) {
   ASSERT_EQ(6U, TotalC);
 
   ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
   ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
   ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
 }
 
 TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
-  InstrProfRecord Record("caller", 0x1234, {1, 2});
+  NamedInstrProfRecord Record("caller", 0x1234, {1, 2});
   Record.reserveSites(IPVK_IndirectCallTarget, 1);
   InstrProfValueData VD0[] = {{1000, 1}, {2000, 2}, {3000, 3}, {5000, 5},
                               {4000, 4}, {6000, 6}};
   Record.addValueData(IPVK_IndirectCallTarget, 0, VD0, 6, nullptr);
   NoError(Writer.addRecord(std::move(Record)));
   auto Profile = Writer.writeBuffer();
   readProfile(std::move(Profile));
   Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
   ASSERT_EQ(3U, ValueData[1].Count);
   ASSERT_EQ(5000U, ValueData[2].Value);
   ASSERT_EQ(2U, ValueData[2].Count);
   ASSERT_EQ(6000U, ValueData[3].Value);
   ASSERT_EQ(1U, ValueData[3].Count);
 }
 
 TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write_with_weight) {
-  InstrProfRecord Record1("caller", 0x1234, {1, 2});
+  NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
 
   // 4 value sites.
   Record1.reserveSites(IPVK_IndirectCallTarget, 4);
   InstrProfValueData VD0[] = {
       {(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
   Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
   // No value profile data at the second site.
   Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write_with_weight) {
   ASSERT_EQ(60U, TotalC);
 
   ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
   ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
   ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
 }
 
 TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write_big_endian) {
-  InstrProfRecord Record1("caller", 0x1234, {1, 2});
+  NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
 
   // 4 value sites.
   Record1.reserveSites(IPVK_IndirectCallTarget, 4);
   InstrProfValueData VD0[] = {
       {(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
   Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
   // No value profile data at the second site.
   Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write_big_endian) {
   ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
 
   // Restore little endian default:
   Writer.setValueProfDataEndianness(support::little);
 }
 
 TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) {
   static const char caller[] = "caller";
-  InstrProfRecord Record11(caller, 0x1234, {1, 2});
-  InstrProfRecord Record12(caller, 0x1234, {1, 2});
+  NamedInstrProfRecord Record11(caller, 0x1234, {1, 2});
+  NamedInstrProfRecord Record12(caller, 0x1234, {1, 2});
 
   // 5 value sites.
   Record11.reserveSites(IPVK_IndirectCallTarget, 5);
   InstrProfValueData VD0[] = {{uint64_t(callee1), 1},
                               {uint64_t(callee2), 2},
                               {uint64_t(callee3), 3},
                               {uint64_t(callee4), 4}};
   Record11.addValueData(IPVK_IndirectCallTarget, 0, VD0, 4, nullptr);
TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1_saturation) {
   auto Result2 = Writer.addRecord({"foo", 0x1234, {Max}});
   ASSERT_EQ(InstrProfError::take(std::move(Result2)),
             instrprof_error::counter_overflow);
 
   auto Result3 = Writer.addRecord({bar, 0x9012, {8}});
   ASSERT_EQ(InstrProfError::take(std::move(Result3)),
             instrprof_error::success);
 
-  InstrProfRecord Record4("baz", 0x5678, {3, 4});
+  NamedInstrProfRecord Record4("baz", 0x5678, {3, 4});
   Record4.reserveSites(IPVK_IndirectCallTarget, 1);
   InstrProfValueData VD4[] = {{uint64_t(bar), 1}};
   Record4.addValueData(IPVK_IndirectCallTarget, 0, VD4, 1, nullptr);
   auto Result4 = Writer.addRecord(std::move(Record4));
   ASSERT_EQ(InstrProfError::take(std::move(Result4)),
             instrprof_error::success);
 
   // Verify value data counter overflow.
-  InstrProfRecord Record5("baz", 0x5678, {5, 6});
+  NamedInstrProfRecord Record5("baz", 0x5678, {5, 6});
   Record5.reserveSites(IPVK_IndirectCallTarget, 1);
   InstrProfValueData VD5[] = {{uint64_t(bar), Max}};
   Record5.addValueData(IPVK_IndirectCallTarget, 0, VD5, 1, nullptr);
   auto Result5 = Writer.addRecord(std::move(Record5));
   ASSERT_EQ(InstrProfError::take(std::move(Result5)),
             instrprof_error::counter_overflow);
 
   auto Profile = Writer.writeBuffer();

 }
 
 // This test tests that when there are too many values
 // for a given site, the merged results are properly
 // truncated.
 TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge_site_trunc) {
   static const char caller[] = "caller";
 
-  InstrProfRecord Record11(caller, 0x1234, {1, 2});
-  InstrProfRecord Record12(caller, 0x1234, {1, 2});
+  NamedInstrProfRecord Record11(caller, 0x1234, {1, 2});
+  NamedInstrProfRecord Record12(caller, 0x1234, {1, 2});
 
   // 2 value sites.
   Record11.reserveSites(IPVK_IndirectCallTarget, 2);
   InstrProfValueData VD0[255];
   for (int I = 0; I < 255; I++) {
     VD0[I].Value = 2 * I;
     VD0[I].Count = 2 * I + 1000;
   }
static void addValueProfData(InstrProfRecord &Record) {
   Record.addValueData(IPVK_IndirectCallTarget, 2, VD2, 3, nullptr);
   InstrProfValueData VD3[] = {{uint64_t(callee2), 1800},
                               {uint64_t(callee3), 2000}};
   Record.addValueData(IPVK_IndirectCallTarget, 3, VD3, 2, nullptr);
   Record.addValueData(IPVK_IndirectCallTarget, 4, nullptr, 0, nullptr);
 }
 
 TEST_P(MaybeSparseInstrProfTest, value_prof_data_read_write) {
-  InstrProfRecord SrcRecord("caller", 0x1234, {1ULL << 31, 2});
+  InstrProfRecord SrcRecord({1ULL << 31, 2});
   addValueProfData(SrcRecord);
   std::unique_ptr<ValueProfData> VPData =
       ValueProfData::serializeFrom(SrcRecord);
 
-  InstrProfRecord Record("caller", 0x1234, {1ULL << 31, 2});
+  InstrProfRecord Record({1ULL << 31, 2});
   VPData->deserializeTo(Record, nullptr);
 
   // Now read data from Record and sanity check the data
   ASSERT_EQ(5U, Record.getNumValueSites(IPVK_IndirectCallTarget));
   ASSERT_EQ(5U, Record.getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
   ASSERT_EQ(4U, Record.getNumValueDataForSite(IPVK_IndirectCallTarget, 1));
   ASSERT_EQ(3U, Record.getNumValueDataForSite(IPVK_IndirectCallTarget, 2));
   ASSERT_EQ(2U, Record.getNumValueDataForSite(IPVK_IndirectCallTarget, 3));
TEST_P(MaybeSparseInstrProfTest, value_prof_data_read_write) {
   ASSERT_EQ(StringRef((const char *)VD_3[0].Value, 7), StringRef("callee3"));
   ASSERT_EQ(2000U, VD_3[0].Count);
   ASSERT_EQ(StringRef((const char *)VD_3[1].Value, 7), StringRef("callee2"));
   ASSERT_EQ(1800U, VD_3[1].Count);
 }
 
 TEST_P(MaybeSparseInstrProfTest, value_prof_data_read_write_mapping) {
 
-  InstrProfRecord SrcRecord("caller", 0x1234, {1ULL << 31, 2});
+  NamedInstrProfRecord SrcRecord("caller", 0x1234, {1ULL << 31, 2});
   addValueProfData(SrcRecord);
   std::unique_ptr<ValueProfData> VPData =
       ValueProfData::serializeFrom(SrcRecord);
 
-  InstrProfRecord Record("caller", 0x1234, {1ULL << 31, 2});
+  NamedInstrProfRecord Record("caller", 0x1234, {1ULL << 31, 2});
   InstrProfSymtab Symtab;
   Symtab.mapAddress(uint64_t(callee1), 0x1000ULL);
   Symtab.mapAddress(uint64_t(callee2), 0x2000ULL);
   Symtab.mapAddress(uint64_t(callee3), 0x3000ULL);
   Symtab.mapAddress(uint64_t(callee4), 0x4000ULL);
   // Missing mapping for callee5
   Symtab.finalizeSymtab();