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

[Hrvoje Varga via llvm-commits]

hvarga marked 7 inline comments as done.

================
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
----------------
dsanders wrote:
> Naming nit: I'd like to keep the immediate size in the name if possible. Could we go with ConstantUImmRange5_Range2_64AsmOperandClass
I agree with the name change. But you probably mean to name it "ConstantUImm5_Range2_64AsmOperandClass" (note the removal of "Range" part).

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:542
@@ +541,3 @@
+def uimm_range_2_64 : Operand<i32> {
+  let PrintMethod = "printUnsignedImm";
+  let EncoderMethod = "getSizeInsEncoding";
----------------
dsanders wrote:
> 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.
Good to know, thanks for the heads up.

================
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"),
----------------
dsanders wrote:
> 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
Actually, there is a deviation in case of DINSM instruction. Class DINSM_MM64R6_DESC also inherits from InsBase class but uses uimm_range_2_64 as an argument instead of uimm5_inssize_plus1. So I will leave this class InsBase as is in this patch.

================
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
----------------
dsanders wrote:
> 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.
You are right. Reverted back.

================
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
----------------
dsanders wrote:
> We've fixed this one, but could you add a fixme about the 32<=pos + size <= 64 constraint on dinsm
Added.

================
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:
----------------
dsanders wrote:
> Indentation on the '# encoding'
Fixed.


http://reviews.llvm.org/D16181