[PATCH] D23564: [RISCV 7/10] Add RISCVInstPrinter and basic MC assembler tests

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

Getting 7 patches in before our first real test seems less than ideal.  That's a lot of code to exercise and not many tests doing it.  One possibility would be to move this patch a bit earlier in the sequence, and write some c++ code which prints manually constructed instructions.  Not sure that's actually worth doing, but I'm intentionally being pedantic as requested.  :)

(p.s. I don't work on the backend, so I'm learning as I go through the patches.  If something doesn't make sense, that's the probable reason.)


================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:10
@@ +9,3 @@
+//
+// This class prints an RISCV MCInst to a .s file.
+//
----------------
I believe you mentioned in another thread that the format is not well specified.  We should call that out here.  What is the goal for this implementation?  Compatibility with existing implementation?  Implementing some newly defined spec?  

================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:23
@@ +22,3 @@
+
+#define DEBUG_TYPE "asm-printer"
+
----------------
Should this be riscv-asm-printer?  Not sure what convention we use in various backends.

================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:44
@@ +43,3 @@
+  if (MO.isReg()) {
+    O << getRegisterName(MO.getReg());
+    return;
----------------
reuse printRegName?


https://reviews.llvm.org/D23564