[PATCH] D13130: [mips][microMIPS] Implement ADDQ.PH, ADDQ_S.W, ADDQH.PH, ADDQH.W, ADDSC, ADDU.PH, ADDU_S.QB, ADDWC and ADDUH.QB instructions

[Daniel Sanders via llvm-commits]

dsanders added inline comments.

================
Comment at: lib/Target/Mips/MicroMipsDSPInstrFormats.td:13-14
@@ -12,3 +12,4 @@
     : MipsInst<(outs), (ins), "", [], NoItinerary, FrmOther>, PredicateControl {
   let EncodingPredicates = [HasDSP];
+  let InsnPredicates = [InMicroMips];
   string BaseOpcode = opstr;
----------------
It's very important that you put these in the correct category. PredicateControl only works if each sub-list is a mutually exclusive group.

HasDSP is an InsnPredicate. InMicroMips is an AdditionalPredicate.

================
Comment at: lib/Target/Mips/MicroMipsDSPInstrFormats.td:20
@@ -18,3 +19,3 @@
 
-class POOL32A_3R_FMT<bits<11> op> : MMDSPInst {
+class POOL32A_3R_FMT<string instr_asm, bits<11> op> : MMDSPInst<instr_asm> {
   bits<5> rd;
----------------
Please rename 'instr_asm' to 'opstr' to match MMCDSPInst. instr_asm is likely to be confused for assembly mnemonics which is not how it's used (the assembly syntax is defined by the *_DESC class).

================
Comment at: lib/Target/Mips/MicroMipsDSPInstrFormats.td:32-33
@@ +31,4 @@
+
+class POOL32A_3RB0_FMT<string instr_asm, bits<10> op>
+    : MMDSPInst<instr_asm> {
+  bits<5> rd;
----------------
Likewise

================
Comment at: lib/Target/Mips/MipsDSPInstrFormats.td:31-33
@@ -30,1 +30,5 @@
 
+class ISA_DSPR2 {
+  list<Predicate> EncodingPredicates = [HasDSPR2];
+}
+
----------------
HasDSPR2 is an InsnPredicate not an EncodingPredicate.

================
Comment at: test/MC/Disassembler/Mips/micromips_dsp.txt:1
@@ -1,2 +1,2 @@
 # RUN: llvm-mc --disassemble %s -triple=mips-unknown-linux -mcpu=mips32r6 -mattr=micromips  -mattr=+dsp | FileCheck %s
 
----------------
(about filename):
I've been trying to organize test/MC/Disassembler/Mips the same way as test/MC/Mips but other work has been taking priority so I haven't renamed all the files yet.

Could you name this file test/MC/Disassembler/Mips/micromips-dsp/valid.txt?

================
Comment at: test/MC/Disassembler/Mips/micromips_dsp.txt:5
@@ -4,1 +4,3 @@
 
+0x00 0xa4 0x1c 0xcd # CHECK: addu_s.qb $3, $4, $5
+
----------------
Nit: The blank lines in this file are unnecessary. If any double-spaced files have crept back in could you fix them? Thanks

================
Comment at: test/MC/Disassembler/Mips/micromips_dspr2.txt:1
@@ +1,2 @@
+# RUN: llvm-mc --disassemble %s -triple=mips-unknown-linux -mcpu=mips32r6 -mattr=micromips  -mattr=+dspr2 | FileCheck %s
+
----------------
(about filename):
I've been trying to organize test/MC/Disassembler/Mips the same way as test/MC/Mips but other work has been taking priority so I haven't renamed all the files yet.

Could you name this file test/MC/Disassembler/Mips/micromips-dspr2/valid.txt?

================
Comment at: test/MC/Disassembler/Mips/micromips_dspr2.txt:2
@@ +1,3 @@
+# RUN: llvm-mc --disassemble %s -triple=mips-unknown-linux -mcpu=mips32r6 -mattr=micromips  -mattr=+dspr2 | FileCheck %s
+
+0x00 0xa4 0x18 0x4d # CHECK: addqh.ph $3, $4, $5
----------------
Likewise

================
Comment at: test/MC/Mips/micromips32r6/micromips-dsp-instructions.s:1
@@ -1,2 +1,2 @@
 # RUN: llvm-mc %s -triple=mips-unknown-linux -show-encoding -mcpu=mips32r6 -mattr=micromips -mattr=+dsp | FileCheck %s
 
----------------
(about filename):
I didn't spot this on the previous patch, but this is in the wrong directory too. At test/MC/Mips we have a directory for each ISA and each ASE. DSP is an ASE so this should be in test/MC/Mips/micromips-dsp/valid.txt

I'll correct the repo.


http://reviews.llvm.org/D13130