[PATCH] D44519: Add llvm-exegesis tool.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Some initial style comments - braces, assertion messages, StringRef and for-loop cleanup in particular need addressing throughout.



================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:19
+
+BenchmarkRunner::InstructionFilter::~InstructionFilter() = default;
+
----------------
Move to header?


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:21
+
+BenchmarkRunner::~BenchmarkRunner() = default;
+
----------------
Move to header?


================
Comment at: tools/llvm-exegesis/lib/InMemoryAssembler.cpp:54
+    return true;
+  }
+  std::string Banner = std::string("After ") + std::string(P->getPassName());
----------------
```
  if (!PI->getNormalCtor()) {
    llvm::errs() << " cannot create pass: " << PI->getPassName() << "\n";
    return true;
  }
  llvm::Pass *P = PI->getNormalCtor()();
```


================
Comment at: tools/llvm-exegesis/lib/InstructionSnippetGenerator.cpp:29
+      OS << Reg;
+    }
+    OS << ",";
----------------
(style) Remove braces where you can


================
Comment at: tools/llvm-exegesis/lib/InstructionSnippetGenerator.cpp:55
+  if (IsDef == false)
+    Var.IsUse = true;
+  Var.ExplicitOperands.push_back(OpIndex);
----------------
```
  Var.IsDef = IsDef;
  Var.IsUse = !IsDef;
```


================
Comment at: tools/llvm-exegesis/lib/InstructionSnippetGenerator.cpp:72
+    if (llvm::is_contained(Var.ExplicitOperands, OpIndex))
+      return Var;
+  }
----------------
use llvm::any_of ?


================
Comment at: tools/llvm-exegesis/lib/InstructionSnippetGenerator.cpp:86
+  llvm::SmallVector<int, 10> TiedToMap;
+  for (size_t I = 0; I < InstrInfo.getNumOperands(); ++I) {
+    TiedToMap.push_back(InstrInfo.getOperandConstraint(I, llvm::MCOI::TIED_TO));
----------------
(style)
```
for (size_t I = 0, E = InstrInfo.getNumOperands(); I <E; ++I) {
```


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:46
+  const auto GetRandomIndex = [&RandomEngine](size_t Size) {
+    assert(Size > 0);
+    return std::uniform_int_distribution<>(0, Size - 1)(RandomEngine);
----------------
(style) Add assertion message as well as condition


================
Comment at: tools/llvm-exegesis/lib/LlvmState.cpp:46
+  return *SubtargetInfo;
+}
+
----------------
Put all getters in the headers?


================
Comment at: tools/llvm-exegesis/lib/OperandGraph.h:59
+  void connect(const Node From, const Node To);
+  void disconnect(const Node From, const Node To);
+
----------------
Should these be pass by value or ref?


================
Comment at: tools/llvm-exegesis/lib/PerfHelper.cpp:79
+
+const char *PerfEvent::name() const { return EventString.c_str(); }
+
----------------
Return StringRef?


================
Comment at: tools/llvm-exegesis/lib/PerfHelper.h:45
+  // The pfm_event_string passed at construction time.
+  const char *name() const;
+
----------------
Return StringRef?


================
Comment at: tools/llvm-exegesis/lib/PerfHelper.h:55
+  // e.g. "snb_ep::INSTRUCTION_RETIRED:e=0:i=0:c=0:t=0:u=1:k=0:mg=0:mh=1"
+  const std::string &getPfmEventString() const;
+
----------------
Return StringRef?


================
Comment at: unittests/tools/llvm-exegesis/BenchmarkResultTest.cpp:37
+
+  const std::string Filename = "data.yaml";
+
----------------
StringRef?


Repository:
  rL LLVM

https://reviews.llvm.org/D44519