[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/AliasingTracker.cpp:1
+#include "AliasingTracker.h"
+
----------------
Missing header. please check all other files.


================
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)
----------------
gchatelet wrote:
> courbet wrote:
> > 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.
> Yes, that's part of the redesign I intend to do. I added some FIXME in the header file.
My point is: where else is AC used ? If it's used outside of the ctor, it's probably wrong, because AC was moved from. If it's use donly in the ctor, why not make it local to the ctor now ? 


================
Comment at: tools/llvm-exegesis/lib/Assembler.cpp:1
-//===-- InMemoryAssembler.cpp -----------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#include "InMemoryAssembler.h"
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
+#include "Assembler.h"
+
----------------
file header.


================
Comment at: tools/llvm-exegesis/lib/Assembler.h:66
+
+// Creates a temporary `void foo()` function from the provided TM. Adds
+// Instructions into it and runs a set of llvm Passes to provide a prologue and
----------------
s/TM/Instructions ?


================
Comment at: tools/llvm-exegesis/lib/Assembler.h:68
+// Instructions into it and runs a set of llvm Passes to provide a prologue and
+// epilogue. Once the MachineFunction is ready, it is assembled to AsmStream,
+// the temporary function is then discarded.
----------------
"it is assembled (for TM) to AsmStream,..."


================
Comment at: tools/llvm-exegesis/lib/Assembler.h:74
+
+// Creates an ObjectFile from assembly code held in Buffer.
+// Note: the resulting object keeps a copy of Buffer so it can be discarded once
----------------
"Creates an ObjectFile in the format  understood by TM"


================
Comment at: tools/llvm-exegesis/lib/BenchmarkResult.h:46
   static InstructionBenchmark readYamlOrDie(llvm::StringRef Filename);
-  static std::vector<InstructionBenchmark> readYamlsOrDie(llvm::StringRef Filename);
+  static std::vector<InstructionBenchmark>
+  readYamlsOrDie(llvm::StringRef Filename);
----------------
Did you clang-format your patch ?


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:36
+      MCRegisterInfo(State.getRegInfo()),
+      ATC(MCRegisterInfo, getReservedRegs(State)) {}
 BenchmarkRunner::~BenchmarkRunner() = default;
----------------
let's inline getReservedRegs() here.


Repository:
  rL LLVM

https://reviews.llvm.org/D46821