[PATCH] D68649: [Mips][llvm-exegesis] Add a Mips target

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

Thanks for the contribution. I only have cosmetic comments.



================
Comment at: unittests/tools/llvm-exegesis/Mips/TargetTest.cpp:27
+
+using llvm::APInt;
+using llvm::MCInst;
----------------
You don't need these.  I guess this is a copy-paste from the X86 tests: these are remains from a time when we were not in namespace llvm. (Fixed in rL374143).


================
Comment at: unittests/tools/llvm-exegesis/Mips/TargetTest.cpp:56
+TEST_F(MipsTargetTest, SetRegToConstant) {
+  const auto Insts = setRegTo(llvm::Mips::T0, llvm::APInt());
+  EXPECT_THAT(Insts, Not(IsEmpty()));
----------------
ditto: remove `llvm::`


================
Comment at: unittests/tools/llvm-exegesis/Mips/TargetTest.cpp:57
+  const auto Insts = setRegTo(llvm::Mips::T0, llvm::APInt());
+  EXPECT_THAT(Insts, Not(IsEmpty()));
+}
----------------
Don't you want to test the actual instruction ?




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

https://reviews.llvm.org/D68649