[NFC][llvm-exegesis] Refactor Analysis::SchedClassCluster::measurementsMatch()

[lebedev.ri]

Summary

The diff looks scary but it really isn't:

1. I moved the check for the number of measurements into SchedClassClusterCentroid::validate()
2. While there, added a check that we can only have a single inverse throughput measurement. I missed that when adding it initially.
3. In Analysis::SchedClassCluster::measurementsMatch() is called with the current LLVM values from schedule class and the values from Centroid.

3.1. The values from centroid we can already get from SchedClassClusterCentroid::getAsPoint().

This isn't 100% a NFC, because previously for inverse throughput we used `min()`. I have asked whether i have done that correctly in
https://reviews.llvm.org/D57647?id=184939#inline-510384 but did not hear back. I think `avg()` should be used too, thus it is a fix.

3.2. Finally, refactor the computation of the LLVM-specified values into Analysis::SchedClassCluster::getSchedClassPoint()

I will need that function for [[ https://bugs.llvm.org/show_bug.cgi?id=41275 | PR41275 ]]

[lebedev.ri]

Fixup last one error check, switch one more loop to llvm::zip().

[courbet]

Thanks Roman.

This isn't 100% a NFC, because previously for inverse throughput we used min(). I have asked whether i have done that correctly in

https://reviews.llvm.org/D57647?id=184939#inline-510384 but did not hear back. I think avg() should be used too, thus it is a fix.

It should not matter if we assume that the output from the NumIterations measurements is accurate enough, so that's fine.

[lebedev.ri]

In D59951#1447500, @courbet wrote:

Thanks Roman.

Thank you for the speedy review.

This isn't 100% a NFC, because previously for inverse throughput we used min(). I have asked whether i have done that correctly in

https://reviews.llvm.org/D57647?id=184939#inline-510384 but did not hear back. I think avg() should be used too, thus it is a fix.

It should not matter if we assume that the output from the NumIterations measurements is accurate enough, so that's fine.

Yes, that is what i thought, too. (median would be better, but avg is pretty good too.)

llvm/trunk/tools/llvm-exegesis/lib/Analysis.h

public:
     const std::vector<size_t> &getPointIds() const { return PointIds; }
 
     void addPoint(size_t PointId,
                   const InstructionBenchmarkClustering &Clustering);
 
     // Return the cluster centroid.
     const SchedClassClusterCentroid &getCentroid() const { return Centroid; }
 
+    std::vector<BenchmarkMeasure>
+    getSchedClassPoint(InstructionBenchmark::ModeE Mode,
+                       const llvm::MCSubtargetInfo &STI,
+                       const ResolvedSchedClass &SC,
+                       ArrayRef<PerInstructionStats> Representative) const;
+
     // Returns true if the cluster representative measurements match that of SC.
     bool
     measurementsMatch(const llvm::MCSubtargetInfo &STI,
                       const ResolvedSchedClass &SC,
                       const InstructionBenchmarkClustering &Clustering,
                       const double AnalysisInconsistencyEpsilonSquared_) const;
 
   private:

llvm/trunk/tools/llvm-exegesis/lib/Analysis.cpp

static unsigned findProcResIdx(const llvm::MCSubtargetInfo &STI,
   const auto &SchedModel = STI.getSchedModel();
   for (int I = 0, E = SchedModel.getNumProcResourceKinds(); I < E; ++I) {
     if (NameOrId == SchedModel.getProcResource(I)->Name)
       return I;
   }
   return 0;
 }
 
-bool Analysis::SchedClassCluster::measurementsMatch(
-    const llvm::MCSubtargetInfo &STI, const ResolvedSchedClass &RSC,
-    const InstructionBenchmarkClustering &Clustering,
-    const double AnalysisInconsistencyEpsilonSquared_) const {
-  ArrayRef<PerInstructionStats> Representative = Centroid.getStats();
+std::vector<BenchmarkMeasure> Analysis::SchedClassCluster::getSchedClassPoint(
+    InstructionBenchmark::ModeE Mode, const llvm::MCSubtargetInfo &STI,
+    const ResolvedSchedClass &RSC,
+    ArrayRef<PerInstructionStats> Representative) const {
   const size_t NumMeasurements = Representative.size();
-  std::vector<BenchmarkMeasure> ClusterCenterPoint(NumMeasurements);
+
   std::vector<BenchmarkMeasure> SchedClassPoint(NumMeasurements);
-  // Latency case.
-  assert(!Clustering.getPoints().empty());
-  const InstructionBenchmark::ModeE Mode = Clustering.getPoints()[0].Mode;
+
   if (Mode == InstructionBenchmark::Latency) {
-    if (NumMeasurements != 1) {
-      llvm::errs()
-          << "invalid number of measurements in latency mode: expected 1, got "
-          << NumMeasurements << "\n";
-      return false;
-    }
+    assert(NumMeasurements == 1 && "Latency is a single measure.");
+    BenchmarkMeasure &LatencyMeasure = SchedClassPoint[0];
+
     // Find the latency.
-    SchedClassPoint[0].PerInstructionValue = 0.0;
+    LatencyMeasure.PerInstructionValue = 0.0;
+
     for (unsigned I = 0; I < RSC.SCDesc->NumWriteLatencyEntries; ++I) {
       const llvm::MCWriteLatencyEntry *const WLE =
           STI.getWriteLatencyEntry(RSC.SCDesc, I);
-      SchedClassPoint[0].PerInstructionValue =
-          std::max<double>(SchedClassPoint[0].PerInstructionValue, WLE->Cycles);
+      LatencyMeasure.PerInstructionValue =
+          std::max<double>(LatencyMeasure.PerInstructionValue, WLE->Cycles);
     }
-    ClusterCenterPoint[0].PerInstructionValue = Representative[0].avg();
   } else if (Mode == InstructionBenchmark::Uops) {
-    for (int I = 0, E = Representative.size(); I < E; ++I) {
-      const auto Key = Representative[I].key();
+    for (const auto &I : llvm::zip(SchedClassPoint, Representative)) {
+      BenchmarkMeasure &Measure = std::get<0>(I);
+      const PerInstructionStats &Stats = std::get<1>(I);
+
+      StringRef Key = Stats.key();
       uint16_t ProcResIdx = findProcResIdx(STI, Key);
       if (ProcResIdx > 0) {
         // Find the pressure on ProcResIdx `Key`.
         const auto ProcResPressureIt =
             std::find_if(RSC.IdealizedProcResPressure.begin(),
                          RSC.IdealizedProcResPressure.end(),
                          [ProcResIdx](const std::pair<uint16_t, float> &WPR) {
                            return WPR.first == ProcResIdx;
                          });
-        SchedClassPoint[I].PerInstructionValue =
+        Measure.PerInstructionValue =
             ProcResPressureIt == RSC.IdealizedProcResPressure.end()
                 ? 0.0
                 : ProcResPressureIt->second;
       } else if (Key == "NumMicroOps") {
-        SchedClassPoint[I].PerInstructionValue = RSC.SCDesc->NumMicroOps;
+        Measure.PerInstructionValue = RSC.SCDesc->NumMicroOps;
       } else {
         llvm::errs() << "expected `key` to be either a ProcResIdx or a ProcRes "
                         "name, got "
                      << Key << "\n";
-        return false;
+        return {};
       }
-      ClusterCenterPoint[I].PerInstructionValue = Representative[I].avg();
     }
   } else if (Mode == InstructionBenchmark::InverseThroughput) {
-    for (int I = 0, E = Representative.size(); I < E; ++I) {
-      SchedClassPoint[I].PerInstructionValue =
-          MCSchedModel::getReciprocalThroughput(STI, *RSC.SCDesc);
-      ClusterCenterPoint[I].PerInstructionValue = Representative[I].min();
-    }
+    assert(NumMeasurements == 1 && "Inverse Throughput is a single measure.");
+    BenchmarkMeasure &RThroughputMeasure = SchedClassPoint[0];
+
+    RThroughputMeasure.PerInstructionValue =
   } else {
     llvm_unreachable("unimplemented measurement matching mode");
-    return false;
   }
+
+  return SchedClassPoint;
+}
+
+bool Analysis::SchedClassCluster::measurementsMatch(
+    const llvm::MCSubtargetInfo &STI, const ResolvedSchedClass &RSC,
+    const InstructionBenchmarkClustering &Clustering,
+    const double AnalysisInconsistencyEpsilonSquared_) const {
+  assert(!Clustering.getPoints().empty());
+  const InstructionBenchmark::ModeE Mode = Clustering.getPoints()[0].Mode;
+
+  if (!Centroid.validate(Mode))
+    return false;
+
+  const std::vector<BenchmarkMeasure> ClusterCenterPoint =
+      Centroid.getAsPoint();
+
+  const std::vector<BenchmarkMeasure> SchedClassPoint =
+      getSchedClassPoint(Mode, STI, RSC, Centroid.getStats());
+  if (SchedClassPoint.empty())
+    return false; // In Uops mode validate() may not be enough.
+
+  assert(ClusterCenterPoint.size() == SchedClassPoint.size() &&
+         "Expected measured/sched data dimensions to match.");
+
   return Clustering.isNeighbour(ClusterCenterPoint, SchedClassPoint,
                                 AnalysisInconsistencyEpsilonSquared_);
 }
 
 void Analysis::printSchedClassDescHtml(const ResolvedSchedClass &RSC,
                                        llvm::raw_ostream &OS) const {
   OS << "<table class=\"sched-class-desc\">";
   OS << "<tr><th>Valid</th><th>Variant</th><th>NumMicroOps</th><th>Latency</"

llvm/trunk/tools/llvm-exegesis/lib/Clustering.h

public:
   const std::vector<PerInstructionStats> &getStats() const {
     return Representative;
   }
 
   std::vector<BenchmarkMeasure> getAsPoint() const;
 
   void addPoint(ArrayRef<BenchmarkMeasure> Point);
 
+  bool validate(InstructionBenchmark::ModeE Mode) const;
+
 private:
   // Measurement stats for the points in the SchedClassCluster.
   std::vector<PerInstructionStats> Representative;
 };
 
 } // namespace exegesis
 } // namespace llvm
 
 #endif // LLVM_TOOLS_LLVM_EXEGESIS_CLUSTERING_H

llvm/trunk/tools/llvm-exegesis/lib/Clustering.cpp

 
 std::vector<BenchmarkMeasure> SchedClassClusterCentroid::getAsPoint() const {
   std::vector<BenchmarkMeasure> ClusterCenterPoint(Representative.size());
   for (const auto &I : llvm::zip(ClusterCenterPoint, Representative))
     std::get<0>(I).PerInstructionValue = std::get<1>(I).avg();
   return ClusterCenterPoint;
 }
 
+bool SchedClassClusterCentroid::validate(
+    InstructionBenchmark::ModeE Mode) const {
+  size_t NumMeasurements = Representative.size();
+  switch (Mode) {
+  case InstructionBenchmark::Latency:
+    if (NumMeasurements != 1) {
+      llvm::errs()
+          << "invalid number of measurements in latency mode: expected 1, got "
+          << NumMeasurements << "\n";
+      return false;
+    }
+    break;
+  case InstructionBenchmark::Uops:
+    // Can have many measurements.
+    break;
+  case InstructionBenchmark::InverseThroughput:
+    if (NumMeasurements != 1) {
+      llvm::errs() << "invalid number of measurements in inverse throughput "
+                      "mode: expected 1, got "
+                   << NumMeasurements << "\n";
+      return false;
+    }
+    break;
+  default:
+    llvm_unreachable("unimplemented measurement matching mode");
+    return false;
+  }
+
+  return true; // All good.
+}
+
 } // namespace exegesis
 } // namespace llvm