[PATCH] D14429: [mips][microMIPS][DSP] Implement PACKRL.PH, PICK.PH, PICK.QB, SHILO, SHILOV and WRDSP instructions

[Daniel Sanders via llvm-commits]

dsanders added inline comments.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3358-3360
@@ -3357,1 +3357,5 @@
                  "expected 4-bit unsigned immediate");
+  case Match_UImm7_0_Report_UImm:
+    return Error(RefineErrorLoc(IDLoc, Operands, ErrorInfo),
+                 "unsigned immediate operand value out of range");
+  case Match_UImm10_0:
----------------
Is there a reason to not give the range? I suspect this ought to be a plain Match_UImm7_0 similar to the others and the operand in the *_DESC should be updated accordingly.

I'm thinking it might be because it hits the same wrong-error issue that I encounter in my WIP patch for range-checked uimm16. For now I'm using the correct range-checked immediates and putting the problem cases in invalid-wrong-error.s files. I'm thinking that we should extend the error reporting to report multiple reasons for a failed match. For example:
  filename.s:10:19: error: expected 7-bit unsigned immediate, or 10-bit unsigned immediate if '.set nomicromips' were used.
      wrdsp $2, -1
                ^~
This avoids the need to pick a single representative error message and is more helpful to the user.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3369
@@ -3358,1 +3368,3 @@
+    return Error(RefineErrorLoc(IDLoc, Operands, ErrorInfo),
+                 "expected 6-bit immediate");
   }
----------------
Could you add 'signed' to the message so the user doesn't expect 0 - 63.

================
Comment at: lib/Target/Mips/MipsDSPInstrInfo.td:20
@@ +19,3 @@
+  let ParserMethod = "parseImm";
+  let PredicateMethod = "isSImm<" # Bits # ">";
+  let SuperClasses = Supers;
----------------
Is it possible for SHILO's immediate to be a relocatable expression? If not, we should is isConstantSImm

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:404-407
@@ -393,3 +403,6 @@
+
+def ConstantUImm10AsmOperandClass
+    : ConstantUImmAsmOperandClass<10, []>;
 def ConstantUImm4AsmOperandClass
     : ConstantUImmAsmOperandClass<4, []>;
 def ConstantUImm3AsmOperandClass
----------------
You need to add ConstantUImm10AsmOperandClass to the list in the ConstantUImm4AsmOperandClass. This ensures instructions are matched in the correct order (uimm4 first). Otherwise, given the choice between uimm4 and uimm10 it might always pick uimm10 because that appears first in the matching table.

Similarly, MipsSImm6AsmOperandClass should be in this list too (and should be defined near here). I was planning to add the signed immediate just above the unsigned ones. I think MipsSImm6AsmOperandClass might need ConstantUImm10AsmOperandClass in its list too but I haven't tested this yet.

================
Comment at: test/MC/Mips/micromips-dsp/invalid.s:24
@@ +23,3 @@
+  shilo $ac1, -64          # CHECK: :[[@LINE]]:15: error: expected 6-bit immediate
+  wrdsp $5, 128            # CHECK: :[[@LINE]]:{{[0-9]+}}: error: instruction requires a CPU feature not currently enabled
+  wrdsp $5, -1             # CHECK: :[[@LINE]]:13: error: unsigned immediate operand value out of range
----------------
Missing column number. It will point at the mnemonic.


http://reviews.llvm.org/D14429