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

[Alex Bradbury via llvm-commits]

asb marked 2 inline comments as done.
asb added a comment.

In https://reviews.llvm.org/D23564#521832, @reames wrote:

> 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.  :)


I'd say that backend work doesn't start until patch 3, which adds the dummy backend. At that stage there's nothing to test anyway, so really it's patch 4+5+6 that add code that can't yet be tested, then we add the test here. Other than merging this patch into the previous one, I'm not sure what I can do to get the tests started earlier. I'd appreciate suggestions though.


================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:10
@@ +9,3 @@
+//
+// This class prints an RISCV MCInst to a .s file.
+//
----------------
reames wrote:
> 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?  
Basic assembly printing is fine. I think it's more that the accepted pseudo-instructions are under-documented, relocations aren't documented etc. These don't pose an issue for basic printing.

================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:23
@@ +22,3 @@
+
+#define DEBUG_TYPE "asm-printer"
+
----------------
reames wrote:
> Should this be riscv-asm-printer?  Not sure what convention we use in various backends.
I've verified that every other in-tree backend uses `DEBUG_TYPE "asm-printer"`. Was worth checking though, thanks


https://reviews.llvm.org/D23564