[PATCH] D11798: [mips][microMIPS] Implement BOVC, BNVC, EXT, INS and JALRC 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:608-610
@@ -597,1 +607,5 @@
 template <typename InsnType>
+static DecodeStatus DecodeAddiGroupBranchMMR6(MCInst &MI, InsnType insn,
+                                          uint64_t Address,
+                                          const void *Decoder) {
+  InsnType Rt = fieldFromInstruction(insn, 21, 5);
----------------
BOVC/BEQC/BEQZALC aren't in the ADDI opcode group in microMIPS. The documentation says they're in the POP35 group

Also, indentation.

================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:678-680
@@ -636,1 +677,5 @@
 template <typename InsnType>
+static DecodeStatus DecodeDaddiGroupBranchMMR6(MCInst &MI, InsnType insn,
+                                           uint64_t Address,
+                                           const void *Decoder) {
+  InsnType Rt = fieldFromInstruction(insn, 21, 5);
----------------
Similarly, BNVC/BNEC/BNEZALC are in the POP37 group rather than DADDI.

Also, indentation

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:245-246
@@ +244,4 @@
+getBranchTargetOpValueMMR6(const MCInst &MI, unsigned OpNo,
+                       SmallVectorImpl<MCFixup> &Fixups,
+                       const MCSubtargetInfo &STI) const {
+
----------------
Indentation

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:251
@@ +250,3 @@
+  // If the destination is an immediate, divide by 2.
+  if (MO.isImm()) return MO.getImm() >> 1;
+
----------------
The return statement should be on the next line

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.h:109-110
@@ +108,4 @@
+  unsigned getBranchTargetOpValueMMR6(const MCInst &MI, unsigned OpNo,
+                                  SmallVectorImpl<MCFixup> &Fixups,
+                                  const MCSubtargetInfo &STI) const;
+
----------------
Indentation

================
Comment at: lib/Target/Mips/MicroMips32r6InstrFormats.td:517
@@ -490,1 +516,3 @@
 
+class POOL32A_EXT_INS_FM_MMR6<bits<6> funct> {
+  bits<5> rt;
----------------
This should inherit from MipsR6Inst and MMR6Arch<instr_asm>

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:932
@@ +931,3 @@
+  string AsmString = !strconcat("ins", "\t$rt, $rs, $pos, $size");
+  list<dag> Pattern = [(set GPR32Opnd:$rt, (MipsIns GPR32Opnd:$rs, imm:$pos, imm:$size, GPR32Opnd:$src))];
+  InstrItinClass Itinerary = II_INS;
----------------
80 cols?

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:941
@@ +940,3 @@
+  dag OutOperandList = (outs GPR32Opnd:$rt);
+  dag InOperandList = (ins GPR32Opnd:$dst, GPR32Opnd:$rs);
+  string AsmString = !strconcat("jalrc", "\t$rt, $rs");
----------------
I believe $rs is the only input (like JALR). What value are you expecting to read from $rt/$dst?

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:950
@@ +949,3 @@
+
+class BOVC_BNVC_MMR6_DESC_BASE<string instr_asm, DAGOperand opnd,
+                               RegisterOperand GPROpnd>
----------------
DAGOperand -> Operand

================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:1240-1241
@@ -1173,1 +1239,4 @@
+      ISA_MICROMIPS32R6;
+def : MipsInstAlias<"jr $src", (JALRC_MMR6 ZERO, GPR32Opnd:$src), 1>,
+      ISA_MICROMIPS32R6;
 
----------------
This alias isn't correct for all contexts (particularly .set noreorder) and the correctness rules appear to be complex and context dependent.
Could you defer this alias to a later patch that handles all the compact branch aliases? 

================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:669-670
@@ -668,2 +668,4 @@
                  ISA_MICROMIPS32_NOT_MIPS32R6;
+  def JR_MM : MMRel, IndirectBranch<"jr", GPR32Opnd>, JR_FM_MM<0x3c>,
+              ISA_MICROMIPS32_NOT_MIPS32R6;
 }
----------------
Please move this back where it was. It used to be grouped with the other jumps.


http://reviews.llvm.org/D11798