[PATCH] D11633: [mips][microMIPS] Implement LB, LBE, LBU and LBUE instructions

[Daniel Sanders via llvm-commits]

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

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:249-258
@@ -248,2 +248,12 @@
 
+static DecodeStatus DecodeLoadByte9(MCInst &Inst,
+                              unsigned Insn,
+                              uint64_t Address,
+                              const void *Decoder);
+
+static DecodeStatus DecodeLoadByte15(MCInst &Inst,
+                              unsigned Insn,
+                              uint64_t Address,
+                              const void *Decoder);
+
 static DecodeStatus DecodeCacheOp(MCInst &Inst,
----------------
Indentation

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

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1177-1180
@@ +1176,6 @@
+
+  if(Inst.getOpcode() == Mips::LBE_MMR6 ||
+    Inst.getOpcode() == Mips::LBUE_MMR6){
+    Inst.addOperand(MCOperand::createReg(Reg));
+  }
+
----------------
I don't think this block is correct. LBE_MMR6 and LBUE_MMR6 have one output (Reg) and two inputs (Base and Offset). This code adds one output (Reg) and three inputs (Reg, Base, and Offset). There's also no need for the condition since only LBE_MMR6+LBUE_MMR6 will call this decoder.

If it is correct, then there's some formatting issues here (redundant braces, and space between ')' and '{').

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1190-1192
@@ +1189,5 @@
+static DecodeStatus DecodeLoadByte15(MCInst &Inst,
+                              unsigned Insn,
+                              uint64_t Address,
+                              const void *Decoder) {
+  int Offset = SignExtend32<16>(Insn & 0xffff);
----------------
Indentation

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1200-1202
@@ +1199,5 @@
+
+  if(Inst.getOpcode() == Mips::LB_MMR6 || Inst.getOpcode() == Mips::LBU_MMR6){
+    Inst.addOperand(MCOperand::createReg(Reg));
+  }
+
----------------
I don't think this block is correct. LB_MMR6 and LBU_MMR6 have one output (Reg) and two inputs (Base and Offset). This code adds one output (Reg) and three inputs (Reg, Base, and Offset). There's also no need for the condition since only LB_MMR6+LBU_MMR6 will call this decoder.

If it is correct, then there's some formatting issues here (redundant braces, and space between ')' and '{').

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:136
@@ -135,1 +135,3 @@
 
+class LB_LBU_FM_MMR6<bits<6> op> : MipsR6Inst {
+  bits<21> addr;
----------------
Naming convention: This is a bit trickier than usual since LB is in LB32 and LBU is in LBU32 so we don't have a single major opcode to use as a prefix. I think we should have:
  class LB32_FM_MMR6 : MipsR6Inst {
    ...
    let Inst{31-26} = 0b000111
    ...
  }
  class LBU32_FM_MMR6 : LB32_FM_MMR6 {
    let Inst{31-26} = 0b000101
  }

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:148
@@ +147,3 @@
+
+class LBE_LBUE_FM_MMR6<bits<3> funct> : MipsR6Inst {
+  bits<21> addr;
----------------
Naming convention: LBE/LBUE are in POOL32C so this should be POOL32C_LB_LBU_FM_MMR6


================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:287
@@ +286,3 @@
+class LB_LBU_MMR6_DESC_BASE<string instr_asm, Operand MemOpnd,
+                           RegisterOperand GPROpnd> : MMR6Arch<instr_asm> {
+  dag OutOperandList = (outs GPROpnd:$dst);
----------------
Indentation

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:289
@@ +288,3 @@
+  dag OutOperandList = (outs GPROpnd:$dst);
+  dag InOperandList = (ins GPROpnd:$rt, MemOpnd:$addr);
+  string AsmString = !strconcat(instr_asm, "\t$rt, $addr");
----------------
Why three inputs? (mem_mm_16 has two operands inside it) These instructions don't read the result register.

I think you need to remove $rt from here and update AsmString, Constraints, and the decoder accordingly.

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:299
@@ +298,3 @@
+class LBE_LBUE_MMR6_DESC_BASE<string instr_asm, Operand MemOpnd,
+                           RegisterOperand GPROpnd> : LB_LBU_MMR6_DESC_BASE<instr_asm, MemOpnd, GPROpnd> {
+  let DecoderMethod = "DecodeLoadByte9";
----------------
Indentation and 80 cols

================
Comment at: test/MC/Mips/micromips32r6/valid.s:205
@@ +204,2 @@
+  lbue $4, 8($5)           # CHECK: lbue $4, 8($5)      # encoding: [0x60,0x85,0x60,0x08]
+
----------------
Blank line at EOF


http://reviews.llvm.org/D11633