[PATCH] D46821: Update llvm-exegesis to cover latency through another opcode.

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Assembler.cpp:209
   ExecEngine.reset(
-      llvm::EngineBuilder(std::move(FunctionContext.Module))
+      llvm::EngineBuilder(std::move(AC.Module))
           .setErrorStr(&Error)
----------------
Given that you're moving from AC, I don't think it makes sense to have AC be a member variable here. AC should not be accessible outside of this function.


================
Comment at: tools/llvm-exegesis/lib/Assembler.cpp:235
+                           llvm::sys::fs::OpenFlags::F_None);
+  if (ErrorCode)
+    llvm::report_fatal_error("Cannot write to file");
----------------
it'd be better if the error was handled by the caller. BTW I don't see who's calling that.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkResult.cpp:60
 InstructionBenchmark
 InstructionBenchmark::readYamlOrDie(llvm::StringRef Filename) {
+  std::unique_ptr<llvm::MemoryBuffer> MemBuffer =
----------------
Please rebase.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkResult.cpp:79
+
 void InstructionBenchmark::writeYamlOrDie(const llvm::StringRef Filename) {
   if (Filename == "-") {
----------------
what about letting the caller handle the error ?


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:28
 
+llvm::BitVector getReservedRegs(const LLVMState &State) {
+  const AssemblerFunction AF(State.createTargetMachine());
----------------
static + remove namespace


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:29
+llvm::BitVector getReservedRegs(const LLVMState &State) {
+  const AssemblerFunction AF(State.createTargetMachine());
+  return AF.ReservedRegs;
----------------
It does not really make sense to create a function to get the reserved registers. Could you refactor this ?


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:85
 
-  InstrBenchmark.Measurements =
-      runMeasurements(State, Function, NumRepetitions);
+  const ExecutableFunction EF(State.createTargetMachine(),
+                              getObjectFromFile(*ExpectedObjectPath));
----------------
let's avoid creating a target machine several times. It could be a State member.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:56
 
-private:
   virtual const char *getDisplayName() const = 0;
 
----------------
why not private ?


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:58
 
-  virtual llvm::Expected<std::vector<llvm::MCInst>>
-  createCode(const LLVMState &State, unsigned OpcodeIndex,
-             unsigned NumRepetitions,
-             const JitFunctionContext &Context) const = 0;
+  virtual llvm::Error
+  createSnippet(AliasingTrackerCache &ATC, unsigned Opcode,
----------------
Let's keep `llvm::Expected<std::vector<llvm::MCInst>>` if you don't mind.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:59
+  virtual llvm::Error
+  createSnippet(AliasingTrackerCache &ATC, unsigned Opcode,
+                std::vector<llvm::MCInst> &Snippet) const = 0;
----------------
ditto


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:63
   virtual std::vector<BenchmarkMeasure>
-  runMeasurements(const LLVMState &State, const JitFunction &Function,
-                  unsigned NumRepetitions) const = 0;
+  runMeasurements(const ExecutableFunction &EF,
+                  const unsigned NumRepetitions) const = 0;
----------------
ditto


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:22
 
+namespace {
+
----------------
static


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:64
+    std::vector<llvm::MCInst> &Snippet) const {
+  auto &OS = llvm::outs();
+  const llvm::MCInstrDesc &MCInstrDesc = MCInstrInfo.get(Opcode);
----------------
let's take the OS as a parameter. This will allow disabling debug or dumping it to a field of the benchmark results.


================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:1
+#ifndef LLVM_TOOLS_LLVM_EXEGESIS_MCINSTRDESCVIEW_H
+#define LLVM_TOOLS_LLVM_EXEGESIS_MCINSTRDESCVIEW_H
----------------
header + file doc


Repository:
  rL LLVM

https://reviews.llvm.org/D46821