[PATCH] D60851: [llvm-exegesis] Insert IACA markers
Clement Courbet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 25 00:49:18 PDT 2019
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
More information about the llvm-commits
mailing list