[ProfileData] Use SoftInstrProfErrors to count soft errors

[vsk]

Summary

This helper class will make the transition to Error/Expected easier.

The immediate benefit is that it keeps track of the number of soft errors encountered, which can help during debugging. The future benefit is that SoftInstrProfErrors can be made undroppable by adding a simple destructor: this lets us handle soft errors properly with a significantly reduced malloc overhead.

Another benefit is that it lets us delete some error-tracking code in the main "merge" and "scale" logic.

[vsk]

• Small simplification, nfc.

[lhames]

I like this. As you said - it's good balance between performance and error safety.

[davidxl]

include/llvm/ProfileData/InstrProf.h

287 ↗

(On Diff #56628)

Add some comments to the class.

lib/ProfileData/InstrProfWriter.cpp

169 ↗

(On Diff #56628)

Do we really need to pass down the instance reference through all the interfaces? I suppose there should be one singleton instance of error, so some global error update APIs will do?

[vsk]

lib/ProfileData/InstrProfWriter.cpp

169 ↗

(On Diff #56628)

Good point. Wdyt of:

1. Adding a SoftInstrProfErrors member to InstrProfRecord, along with a getError() method (eventually takeError()).
2. Passing a SoftInstrProfErrors& to the InstrProfValueSiteRecord constructor.

That should un-clutter the interfaces.

A global SoftInstrProfErrors instance could see contention, and I'd like to avoid that. It would also make it awkward to enforce soft error handling once we move to Error/Expected: who would be responsible for handling errors which come out of a given record?

[davidxl]

lib/ProfileData/InstrProfWriter.cpp

169 ↗

(On Diff #56628)

sounds good -- the error is per-instrRecord anyway.

[vsk]

lib/ProfileData/InstrProfWriter.cpp

169 ↗

(On Diff #56628)

I wasn't able to add a SoftInstrProfErrors member to InstrProfRecord.

Maintaining a reference to a SIPE instance in InstrProfValueSiteRecord seems to be brittle. If an InstrProfRecord is moved/copied, each entry in IndirectCallSites has to be updated. After adding move/copy constructors to handle this, I found other cases where &InstrProfRecord.SIPE != &IndirectCallSites[I].SIPE but didn't finish investigating the issue. Given that there's extra bookkeeping overhead with this approach (space for the references + time to correct the references per move/copy), I'd prefer keeping the initial version of this patch.

I considered adding a SoftInstrProfErrors _reference_ to InstrProfRecords, but this would cause a lot of churn in the unit tests. I'm open to other approaches.

[davidxl]

lib/ProfileData/InstrProfWriter.cpp

169 ↗

(On Diff #56628)

I think it is reasonable to make the InstrProfValueSiteRecord's interfaces to take SPIE reference as new parameter. Make SPIE member of InstrProfRecord and make its interface unchanged.

Would that work?

[vsk]

• Add some comments to the soft error counter class.
• Per David's suggestion: add an error counter to InstrProfRecord, and pass it by reference into the InstrProfValueSiteRecord methods.

[davidxl]

lgtm

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

enum class instrprof_error {
   compress_failed,
   uncompress_failed
 };
 
 inline std::error_code make_error_code(instrprof_error E) {
   return std::error_code(static_cast<int>(E), instrprof_category());
 }
 
-inline instrprof_error MergeResult(instrprof_error &Accumulator,
-                                   instrprof_error Result) {
-  // Prefer first error encountered as later errors may be secondary effects of
-  // the initial problem.
-  if (Accumulator == instrprof_error::success &&
-      Result != instrprof_error::success)
-    Accumulator = Result;
-  return Accumulator;
+class SoftInstrProfErrors {
+  /// Count the number of soft instrprof_errors encountered and keep track of
+  /// the first such error for reporting purposes.
+
+  /// The first soft error encountered.
+  instrprof_error FirstError;
+
+  /// The number of hash mismatches.
+  unsigned NumHashMismatches;
+
+  /// The number of count mismatches.
+  unsigned NumCountMismatches;
+
+  /// The number of counter overflows.
+  unsigned NumCounterOverflows;
+
+  /// The number of value site count mismatches.
+  unsigned NumValueSiteCountMismatches;
+
+public:
+  SoftInstrProfErrors()
+      : FirstError(instrprof_error::success), NumHashMismatches(0),
+        NumCountMismatches(0), NumCounterOverflows(0),
+        NumValueSiteCountMismatches(0) {}
+
+  /// Track a soft error (\p IE) and increment its associated counter.
+  void addError(instrprof_error IE);
+
+  /// Get the number of hash mismatches.
+  unsigned getNumHashMismatches() const { return NumHashMismatches; }
+
+  /// Get the number of count mismatches.
+  unsigned getNumCountMismatches() const { return NumCountMismatches; }
+
+  /// Get the number of counter overflows.
+  unsigned getNumCounterOverflows() const { return NumCounterOverflows; }
+
+  /// Get the number of value site count mismatches.
+  unsigned getNumValueSiteCountMismatches() const {
+    return NumValueSiteCountMismatches;
   }
 
+  /// Return an error code for the first encountered error.
+  std::error_code getError() const { return make_error_code(FirstError); }
+};
+
 namespace object {
 class SectionRef;
 }
 
 namespace IndexedInstrProf {
 uint64_t ComputeHash(StringRef K);
 }
 
ValueData.sort(
           return left.Value < right.Value;
         });
   }
   /// Sort ValueData Descending by Count
   inline void sortByCount();
 
   /// Merge data from another InstrProfValueSiteRecord
   /// Optionally scale merged counts by \p Weight.
-  instrprof_error merge(InstrProfValueSiteRecord &Input, uint64_t Weight = 1);
+  void merge(SoftInstrProfErrors &SIPE, InstrProfValueSiteRecord &Input,
+             uint64_t Weight = 1);
   /// Scale up value profile data counts.
-  instrprof_error scale(uint64_t Weight);
+  void scale(SoftInstrProfErrors &SIPE, uint64_t Weight);
 };
 
 /// Profiling information for a single function.
 struct InstrProfRecord {
-  InstrProfRecord() {}
+  InstrProfRecord() : SIPE() {}
   InstrProfRecord(StringRef Name, uint64_t Hash, std::vector<uint64_t> Counts)
-      : Name(Name), Hash(Hash), Counts(std::move(Counts)) {}
+      : Name(Name), Hash(Hash), Counts(std::move(Counts)), SIPE() {}
   StringRef Name;
   uint64_t Hash;
   std::vector<uint64_t> Counts;
+  SoftInstrProfErrors SIPE;
 
   typedef std::vector<std::pair<uint64_t, uint64_t>> ValueMapType;
 
   /// 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;
struct InstrProfRecord {
   inline void reserveSites(uint32_t ValueKind, uint32_t NumValueSites);
   /// Add ValueData for ValueKind at value Site.
   void addValueData(uint32_t ValueKind, uint32_t Site,
                     InstrProfValueData *VData, uint32_t N,
                     ValueMapType *ValueMap);
 
   /// Merge the counts in \p Other into this one.
   /// Optionally scale merged counts by \p Weight.
-  instrprof_error merge(InstrProfRecord &Other, uint64_t Weight = 1);
+  void merge(InstrProfRecord &Other, uint64_t Weight = 1);
 
   /// Scale up profile counts (including value profile data) by
   /// \p Weight.
-  instrprof_error scale(uint64_t Weight);
+  void scale(uint64_t Weight);
 
   /// Sort value profile data (per site) by count.
   void sortValueData() {
     for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) {
       std::vector<InstrProfValueSiteRecord> &SiteRecords =
           getValueSitesForKind(Kind);
       for (auto &SR : SiteRecords)
         SR.sortByCount();
     }
   }
   /// Clear value data entries
   void clearValueData() {
     for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
       getValueSitesForKind(Kind).clear();
   }
 
+  /// Get the error contained within the record's soft error counter.
+  std::error_code getError() const { return SIPE.getError(); }
+
 private:
   std::vector<InstrProfValueSiteRecord> IndirectCallSites;
   const std::vector<InstrProfValueSiteRecord> &
   getValueSitesForKind(uint32_t ValueKind) const {
     switch (ValueKind) {
     case IPVK_IndirectCallTarget:
       return IndirectCallSites;
     default:
private:
   }
 
   // Map indirect call target name hash to name string.
   uint64_t remapValue(uint64_t Value, uint32_t ValueKind,
                       ValueMapType *HashKeys);
 
   // Merge Value Profile data from Src record to this record for ValueKind.
   // Scale merged value counts by \p Weight.
-  instrprof_error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
-                                     uint64_t Weight);
+  void mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
   // Scale up value profile data count.
-  instrprof_error scaleValueProfData(uint32_t ValueKind, uint64_t Weight);
+  void scaleValueProfData(uint32_t ValueKind, uint64_t Weight);
 };
 
 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;
 }

llvm/trunk/lib/ProfileData/InstrProf.cpp

 static ManagedStatic<InstrProfErrorCategoryType> ErrorCategory;
 
 const std::error_category &llvm::instrprof_category() {
   return *ErrorCategory;
 }
 
 namespace llvm {
 
+void SoftInstrProfErrors::addError(instrprof_error IE) {
+  if (IE == instrprof_error::success)
+    return;
+
+  if (FirstError == instrprof_error::success)
+    FirstError = IE;
+
+  switch (IE) {
+  case instrprof_error::hash_mismatch:
+    ++NumHashMismatches;
+    break;
+  case instrprof_error::count_mismatch:
+    ++NumCountMismatches;
+    break;
+  case instrprof_error::counter_overflow:
+    ++NumCounterOverflows;
+    break;
+  case instrprof_error::value_site_count_mismatch:
+    ++NumValueSiteCountMismatches;
+    break;
+  default:
+    llvm_unreachable("Not a soft error");
+  }
+}
+
 std::string getPGOFuncName(StringRef RawFuncName,
                            GlobalValue::LinkageTypes Linkage,
                            StringRef FileName,
                            uint64_t Version LLVM_ATTRIBUTE_UNUSED) {
   return GlobalValue::getGlobalIdentifier(RawFuncName, Linkage, FileName);
 }
 
 // Return the PGOFuncName. This function has some special handling when called
while (P < EndP) {
 
     while (P < EndP && *P == 0)
       P++;
   }
   Symtab.finalizeSymtab();
   return make_error_code(instrprof_error::success);
 }
 
-instrprof_error InstrProfValueSiteRecord::merge(InstrProfValueSiteRecord &Input,
-                                                uint64_t Weight) {
+void InstrProfValueSiteRecord::merge(SoftInstrProfErrors &SIPE,
+                                     InstrProfValueSiteRecord &Input,
   this->sortByTargetValues();
   Input.sortByTargetValues();
   auto I = ValueData.begin();
   auto IE = ValueData.end();
-  instrprof_error Result = instrprof_error::success;
   for (auto J = Input.ValueData.begin(), JE = Input.ValueData.end(); J != JE;
        ++J) {
     while (I != IE && I->Value < J->Value)
       ++I;
     if (I != IE && I->Value == J->Value) {
       bool Overflowed;
       I->Count = SaturatingMultiplyAdd(J->Count, Weight, I->Count, &Overflowed);
       if (Overflowed)
-        Result = instrprof_error::counter_overflow;
+        SIPE.addError(instrprof_error::counter_overflow);
       ++I;
       continue;
     }
     ValueData.insert(I, *J);
   }
-  return Result;
 }
 
-instrprof_error InstrProfValueSiteRecord::scale(uint64_t Weight) {
-  instrprof_error Result = instrprof_error::success;
+void InstrProfValueSiteRecord::scale(SoftInstrProfErrors &SIPE,
+                                     uint64_t Weight) {
   for (auto I = ValueData.begin(), IE = ValueData.end(); I != IE; ++I) {
     bool Overflowed;
     I->Count = SaturatingMultiply(I->Count, Weight, &Overflowed);
     if (Overflowed)
-      Result = instrprof_error::counter_overflow;
+      SIPE.addError(instrprof_error::counter_overflow);
   }
-  return Result;
 }
 
 // Merge Value Profile data from Src record to this record for ValueKind.
 // Scale merged value counts by \p Weight.
-instrprof_error InstrProfRecord::mergeValueProfData(uint32_t ValueKind,
-                                                    InstrProfRecord &Src,
-                                                    uint64_t Weight) {
+void InstrProfRecord::mergeValueProfData(uint32_t ValueKind,
   uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
   uint32_t OtherNumValueSites = Src.getNumValueSites(ValueKind);
-  if (ThisNumValueSites != OtherNumValueSites)
-    return instrprof_error::value_site_count_mismatch;
+  if (ThisNumValueSites != OtherNumValueSites) {
+    SIPE.addError(instrprof_error::value_site_count_mismatch);
+    return;
+  }
   std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
       getValueSitesForKind(ValueKind);
   std::vector<InstrProfValueSiteRecord> &OtherSiteRecords =
       Src.getValueSitesForKind(ValueKind);
-  instrprof_error Result = instrprof_error::success;
   for (uint32_t I = 0; I < ThisNumValueSites; I++)
-    MergeResult(Result, ThisSiteRecords[I].merge(OtherSiteRecords[I], Weight));
-  return Result;
+    ThisSiteRecords[I].merge(SIPE, OtherSiteRecords[I], Weight);
 }
 
-instrprof_error InstrProfRecord::merge(InstrProfRecord &Other,
-                                       uint64_t Weight) {
+void InstrProfRecord::merge(InstrProfRecord &Other, uint64_t Weight) {
   // 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;
-
-  instrprof_error Result = instrprof_error::success;
+  if (Counts.size() != Other.Counts.size()) {
+    SIPE.addError(instrprof_error::count_mismatch);
+    return;
+  }
 
   for (size_t I = 0, E = Other.Counts.size(); I < E; ++I) {
     bool Overflowed;
     Counts[I] =
         SaturatingMultiplyAdd(Other.Counts[I], Weight, Counts[I], &Overflowed);
     if (Overflowed)
-      Result = instrprof_error::counter_overflow;
+      SIPE.addError(instrprof_error::counter_overflow);
   }
 
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
-    MergeResult(Result, mergeValueProfData(Kind, Other, Weight));
-
-  return Result;
+    mergeValueProfData(Kind, Other, Weight);
 }
 
-instrprof_error InstrProfRecord::scaleValueProfData(uint32_t ValueKind,
-                                                    uint64_t Weight) {
+void InstrProfRecord::scaleValueProfData(uint32_t ValueKind, uint64_t Weight) {
   uint32_t ThisNumValueSites = getNumValueSites(ValueKind);
   std::vector<InstrProfValueSiteRecord> &ThisSiteRecords =
       getValueSitesForKind(ValueKind);
-  instrprof_error Result = instrprof_error::success;
   for (uint32_t I = 0; I < ThisNumValueSites; I++)
-    MergeResult(Result, ThisSiteRecords[I].scale(Weight));
-  return Result;
+    ThisSiteRecords[I].scale(SIPE, Weight);
 }
 
-instrprof_error InstrProfRecord::scale(uint64_t Weight) {
-  instrprof_error Result = instrprof_error::success;
+void InstrProfRecord::scale(uint64_t Weight) {
   for (auto &Count : this->Counts) {
     bool Overflowed;
     Count = SaturatingMultiply(Count, Weight, &Overflowed);
-    if (Overflowed && Result == instrprof_error::success) {
-      Result = instrprof_error::counter_overflow;
-    }
+    if (Overflowed)
+      SIPE.addError(instrprof_error::counter_overflow);
   }
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
-    MergeResult(Result, scaleValueProfData(Kind, Weight));
-
-  return Result;
+    scaleValueProfData(Kind, Weight);
 }
 
 // Map indirect call target name hash to name string.
 uint64_t InstrProfRecord::remapValue(uint64_t Value, uint32_t ValueKind,
                                      ValueMapType *ValueMap) {
   if (!ValueMap)
     return Value;
   switch (ValueKind) {

llvm/trunk/lib/ProfileData/InstrProfWriter.cpp

std::error_code InstrProfWriter::addRecord(InstrProfRecord &&I,
   auto &ProfileDataMap = FunctionData[I.Name];
 
   bool NewFunc;
   ProfilingData::iterator Where;
   std::tie(Where, NewFunc) =
       ProfileDataMap.insert(std::make_pair(I.Hash, InstrProfRecord()));
   InstrProfRecord &Dest = Where->second;
 
-  instrprof_error Result = instrprof_error::success;
   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)
-      Result = Dest.scale(Weight);
+      Dest.scale(Weight);
   } else {
     // We're updating a function we've seen before.
-    Result = Dest.merge(I, Weight);
+    Dest.merge(I, Weight);
   }
 
   Dest.sortValueData();
 
-  return Result;
+  return Dest.getError();
 }
 
 bool InstrProfWriter::shouldEncodeData(const ProfilingData &PD) {
   if (!Sparse)
     return true;
   for (const auto &Func : PD) {
     const InstrProfRecord &IPR = Func.second;
     if (std::any_of(IPR.Counts.begin(), IPR.Counts.end(),