[PATCH] D47639: [llvm-exegesis] Analysis: Show inconsistencies between checked-in and measured data.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet requested changes to this revision.
gchatelet added inline comments.
This revision now requires changes to proceed.


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:178
+  for (const auto &Measurement :
+       Points[Clusters[0].getPointIds()[0]].Measurements) {
     OS << "<th>";
----------------
Can you `assert(!Clusters.empty());`


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:305
+  // Latency case.
+  const std::string &Mode = Clustering.getPoints()[0].Key.Mode;
+  if (Mode == "latency") { // FIXME: use an enum.
----------------
Can you `assert(!Clustering.getPoints().empty());`?


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:314
+    ClusterCenterPoint[0].Value = Representative[0].avg();
+    SchedClassPoint[0].Value = 0.0; // FIXME: implement latency.
+  } else if (Mode == "uops") {
----------------
I would just return an error here and not let the user think the tool ran correctly.


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:337
+  } else {
+    return false;
+  }
----------------
Add an error msg


================
Comment at: tools/llvm-exegesis/lib/Analysis.cpp:476
   for (const auto &SchedClassAndPoints : makePointsPerSchedClass()) {
-    std::unordered_set<size_t> ClustersForSchedClass;
+    const auto &SchedModel = SubtargetInfo_->getSchedModel();
+    const llvm::MCSchedClassDesc *const SCDesc =
----------------
Can you create variables for `SchedClassAndPoints.first` and `SchedClassAndPoints.second`?


================
Comment at: tools/llvm-exegesis/lib/Analysis.h:45
+  using ClusterId = InstructionBenchmarkClustering::ClusterId;
+  // An llvm::MCSchedClassDesc augmented with some additional data.
+  struct SchedClass {
----------------
line feed


================
Comment at: tools/llvm-exegesis/lib/Analysis.h:63
+    const std::vector<size_t> &getPointIds() const { return PointIds; }
+    const std::vector<BenchmarkMeasureStats> &getRepresentative() const {
+      return Representative;
----------------
doc?


================
Comment at: tools/llvm-exegesis/lib/Analysis.h:72
+
+    void addPoint(size_t PointId,
+                  const InstructionBenchmarkClustering &Clustering) {
----------------
Put the definition out of the declaration?


================
Comment at: tools/llvm-exegesis/lib/Analysis.h:107
 
+  // Returns true of the data for this cluster matches the checked-in llvm data
+  // for this class.
----------------
What is this doc for?


Repository:
  rL LLVM

https://reviews.llvm.org/D47639