[PATCH] D11799: [mips][microMIPS] Implement LLE, LUI, LW and LWE 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 nits


================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:288-290
@@ +287,5 @@
+static DecodeStatus DecodeMemMMImm9(MCInst &Inst,
+                                     unsigned Insn,
+                                     uint64_t Address,
+                                     const void *Decoder);
+
----------------
Indentation

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1347-1349
@@ +1346,5 @@
+static DecodeStatus DecodeMemMMImm9(MCInst &Inst,
+                                     unsigned Insn,
+                                     uint64_t Address,
+                                     const void *Decoder) {
+  int Offset = SignExtend32<9>(Insn & 0x1ff);
----------------
Indentation

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:201-238
@@ -200,2 +200,40 @@
 
+class LOAD_WORD_EVA_FM_MMR6<bits<3> funct> {
+  bits<5> rt;
+  bits<21> addr;
+
+  bits<32> Inst;
+
+  let Inst{31-26} = 0b011000;
+  let Inst{25-21} = rt;
+  let Inst{20-16} = addr{20-16};
+  let Inst{15-12} = 0b0110;
+  let Inst{11-9} = funct;
+  let Inst{8-0}  = addr{8-0};
+}
+
+class LOAD_WORD_FM_MMR6 {
+  bits<5> rt;
+  bits<21> addr;
+
+  bits<32> Inst;
+
+  let Inst{31-26} = 0b111111;
+  let Inst{25-21} = rt;
+  let Inst{20-16} = addr{20-16};
+  let Inst{15-0}  = addr{15-0};
+}
+
+class LOAD_UPPER_IMM_FM_MMR6 {
+  bits<5> rt;
+  bits<16> imm16;
+
+  bits<32> Inst;
+
+  let Inst{31-26} = 0b000100;
+  let Inst{25-21} = rt;
+  let Inst{20-16} = 0;
+  let Inst{15-0}  = imm16;
+}
+
 class CMP_BRANCH_1R_RT_OFF16_FM_MMR6<bits<6> funct> : MipsR6Inst {
----------------
These don't follow the naming convention.

As mentioned in D11801, please break out the parts of addr into variables.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:295
@@ +294,3 @@
+
+class LOAD_WORD<string instr_asm, DAGOperand RO,
+                SDPatternOperator OpNode = null_frag,
----------------
Naming convention

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:295-321
@@ +294,29 @@
+
+class LOAD_WORD<string instr_asm, DAGOperand RO,
+                SDPatternOperator OpNode = null_frag,
+                InstrItinClass Itin = NoItinerary,
+                ComplexPattern Addr = addr> :
+                    MMR6Arch<instr_asm>, MipsR6Inst {
+  dag OutOperandList = (outs RO:$rt);
+  dag InOperandList = (ins mem:$addr);
+  string AsmString = !strconcat(instr_asm, "\t$rt, $addr");
+  let DecoderMethod = "DecodeMemMMImm16";
+  let canFoldAsLoad = 1;
+  let mayLoad = 1;
+  list<dag> Pattern = [(set RO:$rt, (OpNode Addr:$addr))];
+}
+class LW_MMR6_DESC : LOAD_WORD<"lw", GPR32Opnd, load, II_LW, addrDefault>;
+
+class LOAD_UPPER_IMM<string instr_asm, RegisterOperand RO, Operand Imm> :
+                        IsAsCheapAsAMove, MMR6Arch<instr_asm>, MipsR6Inst {
+  dag OutOperandList = (outs RO:$rt);
+  dag InOperandList = (ins Imm:$imm16);
+  string AsmString = !strconcat(instr_asm, "\t$rt, $imm16");
+  list<dag> Pattern = [];
+  bit hasSideEffects = 0;
+  bit isReMaterializable = 1;
+  InstrItinClass Itinerary = II_LUI;
+  Format Form = FrmI;
+}
+class LUI_MMR6_DESC : LOAD_UPPER_IMM<"lui", GPR32Opnd, uimm16>;
+
----------------
dsanders wrote:
> Naming convention
I don't see a need for separate LOAD_WORD and LOAD_UPPER_IMM classes. Why not just have LW_MMR6_DESC and LUI_MMR6_DESC?

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:299
@@ +298,3 @@
+                ComplexPattern Addr = addr> :
+                    MMR6Arch<instr_asm>, MipsR6Inst {
+  dag OutOperandList = (outs RO:$rt);
----------------
Indentation

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:310
@@ +309,3 @@
+
+class LOAD_UPPER_IMM<string instr_asm, RegisterOperand RO, Operand Imm> :
+                        IsAsCheapAsAMove, MMR6Arch<instr_asm>, MipsR6Inst {
----------------
Naming convention

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:311
@@ +310,3 @@
+class LOAD_UPPER_IMM<string instr_asm, RegisterOperand RO, Operand Imm> :
+                        IsAsCheapAsAMove, MMR6Arch<instr_asm>, MipsR6Inst {
+  dag OutOperandList = (outs RO:$rt);
----------------
Indentation

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1221-1222
@@ -1220,4 +1220,4 @@
 let AdditionalPredicates = [NotInMicroMips] in {
-def LW  : Load<"lw", GPR32Opnd, load, II_LW, addrDefault>, MMRel,
+def LW  : StdMMR6Rel, Load<"lw", GPR32Opnd, load, II_LW, addrDefault>, MMRel,
           LW_FM<0x23>;
 }
----------------
This is from an earlier patch but please correct the indentation while you're changing it.


http://reviews.llvm.org/D11799