[PATCH] D17133: Sparc back-end: Addition of missing registers and associated instructions.

[James Y Knight via llvm-commits]

jyknight added a comment.

Overall looks good, just a couple minor tweaks.

There should be some MC/Disassembler tests added as well. (One might hope that the same tests would be shared for assembly and disassembly, but that's not actually the case, unfortunately.)


================
Comment at: lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:153
@@ -152,1 +152,3 @@
 
+  static const MCPhysReg CoProRegs[32] = {
+    Sparc::C0,  Sparc::C1,  Sparc::C2,  Sparc::C3,
----------------
"CoPro" seems an unusual shortening of Coprocessor. "Coproc" seems better.

================
Comment at: test/MC/Sparc/sparc-copro.s:3
@@ +2,3 @@
+
+        ! CHECK: ld [%i1], %c4    ! encoding: [0xc9,0x86,0x40,0x00]
+        ld [%i1], %c4
----------------
Seems an odd selection of tests: %c4/%i1 %c4/%i7 %c19/%i1 all test the same instruction definition, so I don't really see the point of including all of them. I'd include, instead a reg+reg and a reg+imm for each of these new instructions.

================
Comment at: test/MC/Sparc/sparc-copro.s:58
@@ +57,3 @@
+
+        ! CHECK: std %fq, [%o4]   ! encoding: [0xc1,0x33,0x00,0x00]
+        std %fq, [%o4]
----------------
Last 3 tests misplaced? %fq doesn't go with coprocessor tests, and %y already had tests.


http://reviews.llvm.org/D17133