[PATCH] D11406: [mips][microMIPS] Implement LWM16, SB16, SH16, SW16, SWSP and SWM16 instructions

[Daniel Sanders via llvm-commits]

dsanders requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:983-987
@@ -982,4 +982,7 @@
     int Size = RegList.List->size();
-    if (Size < 2 || Size > 5 || *RegList.List->begin() != Mips::S0 ||
-        RegList.List->back() != Mips::RA)
+    if (!(Size >= 2 && Size <= 5 &&
+         ((*RegList.List->begin() == Mips::S0 &&
+           RegList.List->back() == Mips::RA) ||
+          (*RegList.List->begin() == Mips::S0_64 &&
+           RegList.List->back() == Mips::RA_64))))
       return false;
----------------
Can this be written more clearly? It's rather difficult to follow.

Also, why not use both front() and back() rather than mixing begin() and back()?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2579
@@ -2570,2 +2578,3 @@
     // It can be implemented as SWM16 or LWM16 instruction.
-    NewOpcode = Opcode == Mips::SWM_MM ? Mips::SWM16_MM : Mips::LWM16_MM;
+    if (inMicroMipsMode() && (hasMips32r6() || hasMips64r6()))
+      NewOpcode = Opcode == Mips::SWM_MM ? Mips::SWM16_MMR6 : Mips::LWM16_MMR6;
----------------
' || hasMips64r6()' is redundant since predicates are cumulative.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3978-3979
@@ -3965,3 +3977,4 @@
       // should be inserted first.
-      if (RegNo == Mips::RA) {
+      if ((isGP64bit() && RegNo == Mips::RA_64) ||
+          (!isGP64bit() && RegNo == Mips::RA)) {
         Regs.push_back(RegNo);
----------------
Do the registers mentioned here contain data or pointers? The reason I ask is that if it's a pointer then isGP64bit() isn't the right predicate (it should be ArePtrs64bit())

If the answer is 'either', then sticking with isGP64bit() is wrong but it's less wrong than ArePtrs64bit() and there's no right answer without knowing the data type.

Likewise for the other registers below.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:4004-4011
@@ -3986,4 +4003,10 @@
         return MatchOperand_ParseFail;
-      } else if (((RegNo < Mips::S0) || (RegNo > Mips::S7)) &&
-                 (RegNo != Mips::FP) && (RegNo != Mips::RA)) {
+      } else if (((isGP64bit() && ((RegNo < Mips::S0_64) ||
+                                   (RegNo > Mips::S7_64))) ||
+                 (!isGP64bit() && ((RegNo < Mips::S0) ||
+                                   (RegNo > Mips::S7)))) &&
+                 ((isGP64bit() && (RegNo != Mips::FP_64) &&
+                  (RegNo != Mips::RA_64)) ||
+                 (!isGP64bit() && (RegNo != Mips::FP) &&
+                  (RegNo != Mips::RA)))) {
         Error(E, "invalid register operand");
----------------
Can we write this more clearly? It's getting rather big and looks like it could be simpler.

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1873-1874
@@ +1872,4 @@
+  unsigned RegLst;
+  switch(Inst.getOpcode())
+  {
+  default:
----------------
Brace belongs on same line as the switch

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:45
@@ -44,1 +44,3 @@
 
+class LWM_FM_MM16R6<bits<4> funct> {
+  bits<2> rt;
----------------
Should begin with POOL16C

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:547
@@ +546,3 @@
+class LWM16_MMR6_DESC
+    : MicroMipsInst16<(outs reglist16:$rt), (ins mem_mm_4sp:$addr),
+                      !strconcat("lwm16", "\t$rt, $addr"), [],
----------------
This isn't for your patch but it would be good for 'mem_mm_4sp' to indicate the offset is unsigned.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:767
@@ +766,3 @@
+
+def : MipsPat<(store GPRMM16:$src, addrimm4lsl2:$addr),
+              (SW16_MMR6 GPRMM16:$src, addrimm4lsl2:$addr)>, ISA_MICROMIPS32R6;
----------------
Also not for your patch but it would be good for addrimm4lsl2 to indicate that it's unsigned too.

================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:530
@@ -530,1 +529,3 @@
+                  !strconcat(opstr, "\t$rt, $addr"), [], Itin, FrmI>,
+  PredicateControl {
   let DecoderMethod = "DecodeMemMMReglistImm4Lsl2";
----------------
This belongs in the format class. Likewise below


http://reviews.llvm.org/D11406