[PATCH] D16181: [mips][microMIPS] Implement DINSU, DINSM, DINS instructions

[Daniel Sanders via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with some minor changes.


================
Comment at: lib/Target/Mips/MicroMips64r6InstrInfo.td:98
@@ +97,3 @@
+                                  uimm5_inssize_plus1, MipsIns>;
+class DINSM_MM64R6_DESC : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64>;
+class DINS_MM64R6_DESC : InsBase<"dins", GPR64Opnd, uimm5, uimm5_inssize_plus1,
----------------
There's a couple things I ought to mention here:
* I recently discovered that the selection between dins/dinsm/dinsu is wrong, they don't use the same criteria as dext/dextm/dextu. It turned out IAS gets it wrong but GAS has been quietly fixing the bug for us (because it falls into the dins macro which expands to dinsm). One of my range checked immediate patches will correct the decision and make it
* There is a constraint on the size operand of dinsm that we don't yet implement, in addition to 2<=Size<=64, there's also 32 < pos+size <= 64. I'm not aware of a way to check multiple operands in the assembly parser yet.

Neither of these issues require changes to this particular patch. I just mention them in case you encounter them.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:428-429
@@ -417,2 +427,4 @@
     : ConstantUImmAsmOperandClass<5, [ConstantUImm6AsmOperandClass], 1>;
+def ConstantUImmRange2_64AsmOperandClass
+    : ConstantUImmRangeAsmOperandClass<2, 64, [ConstantUImm6AsmOperandClass]>;
 def ConstantUImm5Plus32AsmOperandClass
----------------
Naming nit: I'd like to keep the immediate size in the name if possible. Could we go with ConstantUImmRange5_Range2_64AsmOperandClass

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:542
@@ +541,3 @@
+def uimm_range_2_64 : Operand<i32> {
+  let PrintMethod = "printUnsignedImm";
+  let EncoderMethod = "getSizeInsEncoding";
----------------
Just to mention it: My patch series deletes this function in favour of printUImm<BitSize>. Depending on what order we commit in you may need to change this.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1236-1237
@@ -1208,4 +1235,4 @@
 class InsBase<string opstr, RegisterOperand RO, Operand PosOpnd,
-              SDPatternOperator Op = null_frag>:
-  InstSE<(outs RO:$rt), (ins RO:$rs, PosOpnd:$pos, size_ins:$size, RO:$src),
+              Operand SizeOpnd, SDPatternOperator Op = null_frag>:
+  InstSE<(outs RO:$rt), (ins RO:$rs, PosOpnd:$pos, SizeOpnd:$size, RO:$src),
          !strconcat(opstr, " $rt, $rs, $pos, $size"),
----------------
I noticed that we always use uimm5_inssize_plus1 as this argument. If that's the case, can we remove the argument and use it directly

================
Comment at: test/MC/Mips/micromips64r6/invalid.s:21
@@ -20,3 +20,2 @@
   cache 32, 255($7)        # CHECK: :[[@LINE]]:9: error: expected 5-bit unsigned immediate
-  # FIXME: Check various 'pos + size' constraints on dext*
   dext $2, $3, -1, 1   # CHECK: :[[@LINE]]:16: error: expected 5-bit unsigned immediate
----------------
We haven't fixed this (and I'm not sure how we're going to). The valid range for size is determined by the value of pos.
Please restore the fixme.

================
Comment at: test/MC/Mips/micromips64r6/invalid.s:36
@@ -35,3 +34,1 @@
   dextu $2, $3, 32, 33 # CHECK: :[[@LINE]]:21: error: expected immediate in range 1 .. 32
-  # FIXME: Check size on dins*
-  dins $2, $3, -1, 1   # CHECK: :[[@LINE]]:16: error: expected 6-bit unsigned immediate
----------------
We've fixed this one, but could you add a fixme about the 32<=pos + size <= 64 constraint on dinsm

================
Comment at: test/MC/Mips/micromips64r6/valid.s:153-155
@@ -152,3 +152,5 @@
         di $15                   # CHECK: di $15                  # encoding: [0x00,0x0f,0x47,0x7c]
-
+        dinsu $4, $2, 32, 5      # CHECK: dinsu $4, $2, 32, 5 # encoding: [0x58,0x82,0x20,0x34]
+        dinsm $4, $2, 3, 5       # CHECK: dinsm $4, $2, 3, 5  # encoding: [0x58,0x82,0x38,0xc4]
+        dins $4, $2, 3, 5        # CHECK: dins $4, $2, 3, 5   # encoding: [0x58,0x82,0x38,0xcc]
 1:
----------------
Indentation on the '# encoding'

================
Comment at: test/MC/Mips/mips64r2/invalid.s:27-28
@@ -26,4 +26,4 @@
         # FIXME: Check size on dins*
-        dins $2, $3, -1, 1   # CHECK: :[[@LINE]]:22: error: expected 6-bit unsigned immediate
-        dins $2, $3, 64, 1   # CHECK: :[[@LINE]]:22: error: expected 6-bit unsigned immediate
+        dins $2, $3, -1, 1   # CHECK: :[[@LINE]]:22: error: expected 5-bit unsigned immediate
+        dins $2, $3, 64, 1   # CHECK: :[[@LINE]]:22: error: expected 5-bit unsigned immediate
         dinsm $2, $3, -1, 1  # CHECK: :[[@LINE]]:23: error: expected 5-bit unsigned immediate
----------------
dins should report 6-bit because there's a macro that expands to the appropriate dins* for each 6-bit immediate.


http://reviews.llvm.org/D16181