[PATCH] D10640: [mips][microMIPS] Implement LWP and SWP 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:3449-3454
@@ -3444,2 +3448,8 @@
   MipsOperand &Op = static_cast<MipsOperand &>(*Operands.back());
+
+  if (!Op.isGPRAsmReg()) {
+    Error(E, "invalid operand for instruction");
+    return MatchOperand_ParseFail;
+  }
+
   unsigned Reg = Op.getGPR32Reg();
----------------
dsanders wrote:
> While this looks like a sensible thing to do I'm afraid this isn't correct. The problems it causes will be very subtle and might not reveal themselves for a long time. It's not a parse failure to let non-GPR's through here, we just have to catch them during matching using predicates instead.
> 
> The root of the problem is that all parsing decisions are final since there is no backtracking. Making matching decisions during the parse can can cause erratic behaviour that depends on the order of the matcher table. About a year ago, this became apparent when the parser was selecting FPU register classes at parse time. As a result, the matcher was failing to match legitimate code because the parser had made the wrong decision, blocking the possibility of matching the correct instruction. This is also why numeric registers aren't resolved to a register class until the moment they are rendered into an instruction. If we decided earlier, then an incorrect decision would prevent the assembler matching valid instructions.
This update is closer and fixes the subtle bugs but is still making the key mistake described in my previous comment. You're trying to convert to a internal register number too early.

You need to remove RegIdxOp::ParsedRegNum and the related changes and leave the conversion to an internal register number to the render method (addRegPairOperand()). The reason this render method doesn't work at the moment is that it's trying to use an index as a register number. It's missing the call to getGPR32Reg() that other renderers like addGPR32AsmRegOperands() have.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:947
@@ +946,3 @@
+    return isMem() && isConstantMemOff() && isInt<Bits>(getConstantMemOff())
+      && getMemBase()->isGPRAsmReg();
+  }
----------------
LLVM convention is operators on the previous line

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1013
@@ +1012,3 @@
+  bool isRegPair() const {
+    return Kind == k_RegPair && RegIdx.ParsedRegNum <= 31;
+  }
----------------
You want to test RegIdxOp::Index rather than ParsedRegNum (0-31 contains NoRegister, DSPCCond, MSACSR, and AT_64, among others).

Also, I think it should be '30' instead of '31' since register pairs consist of Index and Index+1

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1331-1333
@@ -1330,4 +1330,5 @@
     Inst.addOperand(MCOperand::createReg(Reg));
-    if (Inst.getOpcode() == Mips::LWP_MM || Inst.getOpcode() == Mips::SWP_MM)
+    if (Inst.getOpcode() == Mips::LWP_MM || Inst.getOpcode() == Mips::SWP_MM
+      || Inst.getOpcode() == Mips::LWP_MMR6 || Inst.getOpcode() == Mips::SWP_MMR6)
       Inst.addOperand(MCOperand::createReg(Reg+1));
 
----------------
This hasn't been fixed.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:252-254
@@ -251,5 +251,5 @@
   let Inst{5-0}   = 0x07;
 }
 
 class BARRIER_MMR6_ENC<string instr_asm, bits<5> op> : MMR6Arch<instr_asm> {
   bits<32> Inst;
----------------
Done

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:259-270
@@ -256,1 +258,14 @@
 
+class LWP_MMR6_DESC : MMR6Arch<"lwp"> {
+  dag OutOperandList = (outs regpair:$rd);
+  dag InOperandList = (ins mem_simm12gpr:$addr);
+  string AsmString = !strconcat("lwp", "\t$rd, $addr");
+  list<dag> Pattern = [];
+  InstrItinClass Itin = NoItinerary;
+  ComplexPattern Addr = addr;
+  Format f = FrmI;
+  string BaseOpcode = "lwp";
+  string DecoderMethod = "DecodeMemMMImm12";
+  bit mayLoad = 1;
+}
+
----------------
Done

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:272-301
@@ -257,18 +271,32 @@
+
+class SWP_MMR6_DESC : MMR6Arch<"swp"> {
+  dag OutOperandList = (outs);
+  dag InOperandList = (ins regpair:$rd, mem_simm12gpr:$addr);
+  string AsmString = !strconcat("swp", "\t$rd, $addr");
+  list<dag> Pattern = [];
+  InstrItinClass Itin = NoItinerary;
+  ComplexPattern Addr = addr;
+  Format f = FrmI;
+  string BaseOpcode = "swp";
+  string DecoderMethod = "DecodeMemMMImm12";
+  bit mayStore = 1;
+}
+
 class SELEQNE_Z_MMR6_DESC_BASE<string instr_asm, RegisterOperand GPROpnd>
     : MMR6Arch<instr_asm> {
   dag OutOperandList = (outs GPROpnd:$rd);
   dag InOperandList = (ins GPROpnd:$rs, GPROpnd:$rt);
   string AsmString = !strconcat(instr_asm, "\t$rd, $rs, $rt");
   list<dag> Pattern = [];
 }
 
 class SELEQZ_MMR6_DESC : SELEQNE_Z_MMR6_DESC_BASE<"seleqz", GPR32Opnd>;
 class SELNEZ_MMR6_DESC : SELEQNE_Z_MMR6_DESC_BASE<"selnez", GPR32Opnd>;
 class SLL_MMR6_DESC : shift_rotate_imm<"sll", uimm5, GPR32Opnd, II_SLL>;
 class DIV_MMR6_DESC : ArithLogicR<"div", GPR32Opnd>;
 class DIVU_MMR6_DESC : ArithLogicR<"divu", GPR32Opnd>;
 class MOD_MMR6_DESC : ArithLogicR<"mod", GPR32Opnd>;
 class MODU_MMR6_DESC : ArithLogicR<"modu", GPR32Opnd>;
 class AND_MMR6_DESC : ArithLogicR<"and", GPR32Opnd>;
 class ANDI_MMR6_DESC : ArithLogicI<"andi", simm16, GPR32Opnd>;
 class NOR_MMR6_DESC : ArithLogicR<"nor", GPR32Opnd>;
----------------
Done


http://reviews.llvm.org/D10640