[llvm-profdata] Add merge() to InstrProfRecord

[slingn]

Summary

This change refactors two aspects of InstrProfRecord:

1. Add a merge() method to InstrProfRecord (previously InstrProfWriter combineInstrProfRecords()) in order to better encapsulate this functionality and to make the InstrProfRecord and SampleRecord APIs more consistent.

2. Make InstrProfRecord mergeValueProfData() a private method since it is only ever called internally by merge().

[slingn]

Fix max function count calculation.

[vsk]

Thanks, some inline comments --

include/llvm/ProfileData/InstrProf.h

323 ↗

(On Diff #40556)

clang-format?

332 ↗

(On Diff #40556)

Can you rangify this loop while we're at it?

417 ↗

(On Diff #40556)

Rangify this one too.

lib/ProfileData/InstrProfWriter.cpp

107 ↗

(On Diff #40557)

Is there a need to keep the iterator object around, since we have the value?

[vsk]

I suppose it would be awkward to use ranged-for loops since it's indexing into two containers.

[davidxl]

lib/ProfileData/InstrProfWriter.cpp

104 ↗

(On Diff #40557)

Better to do insert here and check if insertion actually happens or not.

[slingn]

include/llvm/ProfileData/InstrProf.h

332 ↗

(On Diff #40557)

As vsk commented separately, a for-range loop would be awkward to use here (accessing two vectors in parallel by index).

417 ↗

(On Diff #40557)

Same issue here with parallel vector access by index.

lib/ProfileData/InstrProfWriter.cpp

104 ↗

(On Diff #40557)

Yes - much better. And also helps with addressing vsk's comment below.

[slingn]

Address davidxl and vsk comments.

[davidxl]

lib/ProfileData/InstrProfWriter.cpp

104 ↗

(On Diff #40568)

use std:make_pair(I.hash, InstrProfRecord()) to avoid side effect of making copies.

111 ↗

(On Diff #40568)

else {

Dest = std::forward<InstrProfRecord>(I);

}

the std::forward is missing in the old code too which is less efficient.

[davidxl]

lib/ProfileData/InstrProfWriter.cpp

111 ↗

(On Diff #40568)

Sorry, should be

Dest = std::move(I) or
Dest = static_cast<InstrProfRecord&&>(I)

[silvas]

lib/ProfileData/InstrProfWriter.cpp

104 ↗

(On Diff #40568)

small style nit: use std::tie to make this more readable.

[slingn]

Updated for davidxl and ssilva comments.

[davidxl]

LGTM

[silvas]

LGTM

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

struct InstrProfRecord {
   inline std::unique_ptr<InstrProfValueData[]>
   getValueForSite(uint32_t ValueKind, uint32_t Site) const;
   /// Reserve space for NumValueSites sites.
   inline void reserveSites(uint32_t ValueKind, uint32_t NumValueSites);
   /// Add ValueData for ValueKind at value Site.
   inline void addValueData(uint32_t ValueKind, uint32_t Site,
                            InstrProfValueData *VData, uint32_t N,
                            ValueMapType *HashKeys);
-  /// Merge Value Profile data from Src record to this record for ValueKind.
-  inline instrprof_error mergeValueProfData(uint32_t ValueKind,
-                                            InstrProfRecord &Src);
+
+  /// Merge the counts in \p Other into this one.
+  inline instrprof_error merge(InstrProfRecord &Other);
 
   /// Used by InstrProfWriter: update the value strings to commoned strings in
   /// the writer instance.
   inline void updateStrings(InstrProfStringTable *StrTab);
 
   /// Clear value data entries
   inline void clearValueData() {
     for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
case IPVK_IndirectCallTarget: {
                               uint64_t RHS) { return LHS.first < RHS; });
       if (Result != HashKeys->end())
         Value = (uint64_t)Result->second;
       break;
     }
     }
     return Value;
   }
+
+  // Merge Value Profile data from Src record to this record for ValueKind.
+  instrprof_error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src) {
+    uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
+    uint32_t OtherNumValueSites = Src.getNumValueSites(ValueKind);
+    if (ThisNumValueSites != OtherNumValueSites)
+      return instrprof_error::value_site_count_mismatch;
+    std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
+        getValueSitesForKind(ValueKind);
+    std::vector<InstrProfValueSiteRecord> &OtherSiteRecords =
+        Src.getValueSitesForKind(ValueKind);
+    for (uint32_t I = 0; I < ThisNumValueSites; I++)
+      ThisSiteRecords[I].mergeValueData(OtherSiteRecords[I]);
+    return instrprof_error::success;
+  }
 };
 
 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;
 }

 }
 
 void InstrProfRecord::reserveSites(uint32_t ValueKind, uint32_t NumValueSites) {
   std::vector<InstrProfValueSiteRecord> &ValueSites =
       getValueSitesForKind(ValueKind);
   ValueSites.reserve(NumValueSites);
 }
 
-instrprof_error InstrProfRecord::mergeValueProfData(uint32_t ValueKind,
-                                                    InstrProfRecord &Src) {
-  uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
-  uint32_t OtherNumValueSites = Src.getNumValueSites(ValueKind);
-  if (ThisNumValueSites != OtherNumValueSites)
-    return instrprof_error::value_site_count_mismatch;
-  std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
-      getValueSitesForKind(ValueKind);
-  std::vector<InstrProfValueSiteRecord> &OtherSiteRecords =
-      Src.getValueSitesForKind(ValueKind);
-  for (uint32_t I = 0; I < ThisNumValueSites; I++)
-    ThisSiteRecords[I].mergeValueData(OtherSiteRecords[I]);
-  return instrprof_error::success;
-}
-
 void InstrProfRecord::updateStrings(InstrProfStringTable *StrTab) {
   if (!StrTab)
     return;
 
   Name = StrTab->insertString(Name);
   for (auto &VSite : IndirectCallSites)
     for (auto &VData : VSite.ValueData)
       VData.Value = (uint64_t)StrTab->insertString((const char *)VData.Value);
 }
 
+instrprof_error InstrProfRecord::merge(InstrProfRecord &Other) {
+  // If the number of counters doesn't match we either have bad data
+  // or a hash collision.
+  if (Counts.size() != Other.Counts.size())
+    return instrprof_error::count_mismatch;
+
+  for (size_t I = 0, E = Other.Counts.size(); I < E; ++I) {
+    if (Counts[I] + Other.Counts[I] < Counts[I])
+      return instrprof_error::counter_overflow;
+    Counts[I] += Other.Counts[I];
+  }
+
+  for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) {
+    instrprof_error result = mergeValueProfData(Kind, Other);
+    if (result != instrprof_error::success)
+      return result;
+  }
+
+  return instrprof_error::success;
+}
+
 inline support::endianness getHostEndianness() {
   return sys::IsLittleEndianHost ? support::little : support::big;
 }
 
 /// This is the header of the data structure that defines the on-disk
 /// layout of the value profile data of a particular kind for one function.
 struct ValueProfRecord {
   // The kind of the value profile record.

llvm/trunk/lib/ProfileData/InstrProfWriter.cpp

for (const auto &ProfileData : *V) {
       uint32_t S = VDataPtr->getSize();
       VDataPtr->swapBytesFromHost(ValueProfDataEndianness);
       Out.write((const char *)VDataPtr.get(), S);
     }
   }
 };
 }
 
-static std::error_code combineInstrProfRecords(InstrProfRecord &Dest,
-                                               InstrProfRecord &Source,
-                                               uint64_t &MaxFunctionCount) {
-  // If the number of counters doesn't match we either have bad data
-  // or a hash collision.
-  if (Dest.Counts.size() != Source.Counts.size())
-    return instrprof_error::count_mismatch;
-
-  for (size_t I = 0, E = Source.Counts.size(); I < E; ++I) {
-    if (Dest.Counts[I] + Source.Counts[I] < Dest.Counts[I])
-      return instrprof_error::counter_overflow;
-    Dest.Counts[I] += Source.Counts[I];
-  }
-
-  for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) {
-    if (std::error_code EC = Dest.mergeValueProfData(Kind, Source))
-      return EC;
-  }
-
-  // We keep track of the max function count as we go for simplicity.
-  if (Dest.Counts[0] > MaxFunctionCount)
-    MaxFunctionCount = Dest.Counts[0];
-
-  return instrprof_error::success;
-}
-
 // Internal interface for testing purpose only.
 void InstrProfWriter::setValueProfDataEndianness(
     support::endianness Endianness) {
   ValueProfDataEndianness = Endianness;
 }
 
 void InstrProfWriter::updateStringTableReferences(InstrProfRecord &I) {
   I.updateStrings(&StringTable);
 }
 
 std::error_code InstrProfWriter::addRecord(InstrProfRecord &&I) {
   updateStringTableReferences(I);
   auto &ProfileDataMap = FunctionData[I.Name];
 
-  auto Where = ProfileDataMap.find(I.Hash);
-  if (Where == ProfileDataMap.end()) {
+  bool NewFunc;
+  ProfilingData::iterator Where;
+  std::tie(Where, NewFunc) =
+      ProfileDataMap.insert(std::make_pair(I.Hash, InstrProfRecord()));
+  InstrProfRecord &Dest = Where->second;
+  if (NewFunc) {
     // We've never seen a function with this name and hash, add it.
-    ProfileDataMap[I.Hash] = I;
+    Dest = std::move(I);
+  } else {
+    // We're updating a function we've seen before.
+    instrprof_error MergeResult = Dest.merge(I);
+    if (MergeResult != instrprof_error::success) {
+      return MergeResult;
+    }
+  }
 
-    // We keep track of the max function count as we go for simplicity.
-    if (I.Counts[0] > MaxFunctionCount)
-      MaxFunctionCount = I.Counts[0];
-    return instrprof_error::success;
-  }
+  if (Dest.Counts[0] > MaxFunctionCount)
+    MaxFunctionCount = Dest.Counts[0];
 
-  // We're updating a function we've seen before.
-  return combineInstrProfRecords(Where->second, I, MaxFunctionCount);
+  return instrprof_error::success;
 }
 
 std::pair<uint64_t, uint64_t> InstrProfWriter::writeImpl(raw_ostream &OS) {
   OnDiskChainedHashTableGenerator<InstrProfRecordTrait> Generator;
 
   // Populate the hash table generator.
   for (const auto &I : FunctionData)
     Generator.insert(I.getKey(), &I.getValue());