[PATCH] D60851: [llvm-exegesis] Insert IACA markers

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:202
   llvm::object::OwningBinary<llvm::object::ObjectFile> ObjectFile;
+  // For debugging.
+  {
----------------
Please remove the debugging code.


================
Comment at: llvm/tools/llvm-exegesis/lib/InlineAsmMCExpr.h:2
+#ifndef THIRD_PARTY_LLVM_LLVM_TOOLS_LLVM_EXEGESIS_INLINEASMMCEXPR_H_
+#define THIRD_PARTY_LLVM_LLVM_TOOLS_LLVM_EXEGESIS_INLINEASMMCEXPR_H_
+
----------------
lebedev.ri wrote:
> Missing starting comment, wrong header guard
More specifically, every llvm file needs a header: https://llvm.org/docs/CodingStandards.html#file-headers.


================
Comment at: llvm/tools/llvm-exegesis/lib/InlineAsmMCExpr.h:12
+// Class for emitting arbitrary inline-asm.
+class InlineAsmMCExpr : public MCTargetExpr {
+public:
----------------
I would ditch this intermediate class and only have IacaMarkerBytesMCExpr.


================
Comment at: llvm/tools/llvm-exegesis/lib/InlineAsmMCExpr.h:30
+
+namespace iaca {
+
----------------
I don't think the namespace is really useful here.


================
Comment at: llvm/tools/llvm-exegesis/lib/InlineAsmMCExpr.h:45
+  void visitUsedExpr(MCStreamer &Streamer) const override {
+    /* Does nothing. */
+  }
----------------
I can see that :) Please remove the comment.


================
Comment at: llvm/tools/llvm-exegesis/lib/InlineAsmMCExpr.h:49
+  void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const override {
+    /* Does nothing. */
+  }
----------------
Ditto


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:10
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
----------------
The file header is missing.


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:13
 
+#include "third_party/absl/flags/flag.h"
+#include "third_party/llvm/llvm/include/llvm/MC/MCExpr.h"
----------------
lebedev.ri wrote:
> All these look wrong
Did you build and run the tests ? This should not be compiling on your local checkout.


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:13
 
+#include "third_party/absl/flags/flag.h"
+#include "third_party/llvm/llvm/include/llvm/MC/MCExpr.h"
----------------
courbet wrote:
> lebedev.ri wrote:
> > All these look wrong
> Did you build and run the tests ? This should not be compiling on your local checkout.
Hm, actually I see you're missing and entry in CMakeLists for this, so this is never built & run.


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:26
 
+class IacaBenchmarkRunnerTester : public BenchmarkRunner {
+public:
----------------
`TestIacaBenchmarkRunner` ?


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:51
+
+  static void SetUpTestCase() {}
+  static void TearDownTestCase() {}
----------------
I think you can remove these.


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:78
+  static void AssertIacaMagicBytesInstruction(const llvm::MCInst &Inst) {
+    const std::string InstructionDump = DumpToString(Inst);
+    EXPECT_EQ(TargetOpcode::INLINEASM, Inst.getOpcode())
----------------
I think testing form string equality is a bit brittle. What about:

```
const auto IsImmOperand = [](int64_t Imm) {
  return AllOf(Property(&MCOperand::isImm, Eq(true)), Property(&MCOperand::getImm, Eq(Imm)));
};

const auto IsIacaMarkerOperand = ...;

EXPECT_THAT(Inst, ElementsAre(IsIacaMarkerOperand, IsImmOperand(1)))

```


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:88
+
+  // Wrapper function that simply passes through the arguments to the real
+  // function.
----------------
why not call the real thing directly ?


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:122
+
+  // Start-maker
+  {
----------------
s/maker/marker/ (same below)


================
Comment at: llvm/unittests/tools/llvm-exegesis/BenchmarkRunnerTest.cpp:127
+
+    EXPECT_EQ(llvm::X86::MOV32ri, FirstInst.getOpcode())
+        << "Instruction dump: " << FirstInstDump;
----------------
Same remark as above: let's use matchers.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60851/new/

https://reviews.llvm.org/D60851