[PATCH] D18352: [mips][microMIPS] Implement BC1EQZC, BC1NEZC, BC2EQZC and BC2NEZC instructions

[Simon Dardis via llvm-commits]

sdardis added a subscriber: sdardis.
sdardis requested changes to this revision.
sdardis added a reviewer: sdardis.
sdardis added a comment.
This revision now requires changes to proceed.

Small things mostly, comments inlined.

For the invalid/valid tests, can you sort the instructions you've added into alphabetical order?


================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:874
@@ +873,3 @@
+
+class POOL32I_BRANCH_COP1_FM_MMR6<string instr_asm, bits<5> funct>
+    : MMR6Arch<instr_asm>, MipsR6Inst {
----------------
Rename this to POOL32I_BRANCH_COP_1_2_FM_MMR6. The class below is a copy-paste which is then not necessary and so can be deleted.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:881
@@ +880,3 @@
+
+  let Inst{31-26} = 0b010000;
+  let Inst{25-21} = funct;
----------------
Just to be explicit, the encoding here is correct. The documentation isn't 100%.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:887-898
@@ +886,13 @@
+
+class POOL32I_BRANCH_COP2_FM_MMR6<string instr_asm, bits<5> funct>
+    : MMR6Arch<instr_asm>, MipsR6Inst {
+  bits<5> ct;
+  bits<16> offset;
+
+  bits<32> Inst;
+
+  let Inst{31-26} = 0b010000;
+  let Inst{25-21} = funct;
+  let Inst{20-16} = ct;
+  let Inst{15-0}  = offset;
+}
----------------
See my above comment.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:173-176
@@ -172,2 +172,6 @@
 class TLBINVF_MMR6_ENC : POOL32A_TLBINV_FM_MMR6<"tlbinvf", 0x14d>;
+class BC1EQZC_MMR6_ENC : POOL32I_BRANCH_COP1_FM_MMR6<"bc1eqzc", 0b01000>;
+class BC1NEZC_MMR6_ENC : POOL32I_BRANCH_COP1_FM_MMR6<"bc1nezc", 0b01001>;
+class BC2EQZC_MMR6_ENC : POOL32I_BRANCH_COP2_FM_MMR6<"bc2eqzc", 0b01010>;
+class BC2NEZC_MMR6_ENC : POOL32I_BRANCH_COP2_FM_MMR6<"bc2nezc", 0b01011>;
 
----------------
See my comments on changing the class name to POOL32I_BRANCH_COP_1_2_FM_MMR6.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:943
@@ -938,1 +942,3 @@
 
+class BRANCH_COP1_MMR6_DESC_BASE<string opstr> : BRANCH_DESC_BASE, HARDFLOAT {
+  dag InOperandList = (ins FGR64Opnd:$ft, brtarget_mm:$offset);
----------------
Formatting, break the line after "BRANCH_DESC_BASE," and indent "HARDFLOAT {" appropriately.

================
Comment at: test/MC/Disassembler/Mips/micromips32r6/valid.txt:261-268
@@ -260,1 +260,9 @@
 0x00 0x00 0x53 0x7c # CHECK: tlbinvf
+0x41 0x00 0x00 0x02 # CHECK: bc1eqzc $f0, 4
+0x41 0x1f 0x00 0x02 # CHECK: bc1eqzc $f31, 4
+0x41 0x20 0x00 0x02 # CHECK: bc1nezc $f0, 4
+0x41 0x3f 0x00 0x02 # CHECK: bc1nezc $f31, 4
+0x41 0x40 0x00 0x04 # CHECK: bc2eqzc $0, 8
+0x41 0x5f 0x00 0x04 # CHECK: bc2eqzc $31, 8
+0x41 0x60 0x00 0x04 # CHECK: bc2nezc $0, 8
+0x41 0x7f 0x00 0x04 # CHECK: bc2nezc $31, 8
----------------
You don't need to check each instruction twice if there is no real difference between the operands.

Just once is sufficient provided you do not use zero values.

This applies to the rest of the */valid.s tests.

================
Comment at: test/MC/Mips/micromips32r6/invalid.s:111-126
@@ -110,1 +110,17 @@
   swm16 $16, $17, $ra, 64($sp) # CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand for instruction
+  bc1eqzc $f0, -65535          # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc1eqzc $f31, -65535         # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc1nezc $f0, -65537          # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc1nezc $f31, -65537         # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc1eqzc $f0, 65535           # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc1eqzc $f31, 65535          # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc1nezc $f0, 65536           # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc1nezc $f31, 65536          # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc2eqzc $0, -65535           # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc2eqzc $31, -65535          # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc2nezc $0, -65537           # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc2nezc $31, -65537          # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc2eqzc $0, 65535            # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc2eqzc $31, 65535           # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch to misaligned address
+  bc2nezc $0, 65536            # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
+  bc2nezc $31, 65536           # CHECK: :[[@LINE]]:{{[0-9]+}}: error: branch target out of range
----------------
For these invalid tests, checking once for each type of error for each instruction is sufficient.


http://reviews.llvm.org/D18352