[PATCH] D19906: [mips][microMIPS] Implement SLT, SLTI, SLTIU, SLTU microMIPS32r6 instructions

[Simon Dardis via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

Two major points: The multiclass patterns you've copied can instead be extended. LUi is also covered in a NotInMicroMips predicate. The rest is just formatting nits over the assembly. More details inline.

Thanks.


================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:1040-1069
@@ -1039,1 +1039,32 @@
 
+// brcond patterns
+multiclass BrcondPats_MM<RegisterClass RC, Instruction BEQOp, Instruction BNEOp,
+                      Instruction SLTOp, Instruction SLTuOp, Instruction SLTiOp,
+                      Instruction SLTiuOp, Register ZEROReg> {
+  def : MipsPat<(brcond (i32 (setne RC:$lhs, 0)), bb:$dst),
+                (BNEOp RC:$lhs, ZEROReg, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (seteq RC:$lhs, 0)), bb:$dst),
+                (BEQOp RC:$lhs, ZEROReg, bb:$dst)>;
+
+  def : MipsPat<(brcond (i32 (setge RC:$lhs, RC:$rhs)), bb:$dst),
+                (BEQ_MM (SLTOp RC:$lhs, RC:$rhs), ZERO, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (setuge RC:$lhs, RC:$rhs)), bb:$dst),
+                (BEQ_MM (SLTuOp RC:$lhs, RC:$rhs), ZERO, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (setge RC:$lhs, immSExt16:$rhs)), bb:$dst),
+                (BEQ_MM (SLTiOp RC:$lhs, immSExt16:$rhs), ZERO, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (setuge RC:$lhs, immSExt16:$rhs)), bb:$dst),
+                (BEQ_MM (SLTiuOp RC:$lhs, immSExt16:$rhs), ZERO, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (setgt RC:$lhs, immSExt16Plus1:$rhs)), bb:$dst),
+                (BEQ_MM (SLTiOp RC:$lhs, (Plus1 imm:$rhs)), ZERO, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (setugt RC:$lhs, immSExt16Plus1:$rhs)), bb:$dst),
+                (BEQ_MM (SLTiuOp RC:$lhs, (Plus1 imm:$rhs)), ZERO, bb:$dst)>;
+
+  def : MipsPat<(brcond (i32 (setle RC:$lhs, RC:$rhs)), bb:$dst),
+                (BEQ_MM (SLTOp RC:$rhs, RC:$lhs), ZERO, bb:$dst)>;
+  def : MipsPat<(brcond (i32 (setule RC:$lhs, RC:$rhs)), bb:$dst),
+                (BEQ_MM (SLTuOp RC:$rhs, RC:$lhs), ZERO, bb:$dst)>;
+
+  def : MipsPat<(brcond RC:$cond, bb:$dst),
+                (BNEOp RC:$cond, ZEROReg, bb:$dst)>;
+}
+
----------------
This multiclass definition is unnecessary. Instead, change BrcondPats in MipsInstrInfo.td so that the pattern is using BEQ instead use BEQOp. You can then instantiate the pattern for microMIPS.

================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:1071-1077
@@ +1070,9 @@
+
+multiclass SetgeImmPats_MM<RegisterClass RC, Instruction SLTiOp,
+                        Instruction SLTiuOp> {
+  def : MipsPat<(setge RC:$lhs, immSExt16:$rhs),
+                (XORi_MM (SLTiOp RC:$lhs, immSExt16:$rhs), 1)>;
+  def : MipsPat<(setuge RC:$lhs, immSExt16:$rhs),
+                (XORi_MM (SLTiuOp RC:$lhs, immSExt16:$rhs), 1)>;
+}
+
----------------
Similarly here. You can extend SetgeImmPats in MipsInstrInfo.td to take and XORiOp parameter.

================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:1082-1086
@@ +1081,7 @@
+
+  defm : SeteqPats<GPR32, SLTiu_MM, XOR_MM, SLTu_MM, ZERO>;
+  defm : SetlePats<GPR32, SLT_MM, SLTu_MM>;
+  defm : SetgtPats<GPR32, SLT_MM, SLTu_MM>;
+  defm : SetgePats<GPR32, SLT_MM, SLTu_MM>;
+  defm : SetgeImmPats_MM<GPR32, SLTi_MM, SLTiu_MM>;
+
----------------
With the changes you've made here, some of those patterns (SetlePats, SetgtPats) combine microMIPS and MIPS instructions.

For these multiclass instantiations, extend the definitions in MipsInstroInfo.td to take an XORiOp parameter.



================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:1123-1126
@@ -1073,1 +1122,6 @@
+                      (SLTi_MM GPR32Opnd:$rs, GPR32Opnd:$rt,
+                      simm32_relaxed:$imm), 0>;
+  def : MipsInstAlias<"sltu $rs, $rt, $imm",
+                      (SLTiu_MM GPR32Opnd:$rs, GPR32Opnd:$rt,
+                      simm32_relaxed:$imm), 0>;
   def : MipsInstAlias<"sll $rd, $rt, $rs",
----------------
Align the simm32_relaxed:$imm under GPR32Opnd like:

   def : MipsInstAlias<"sltu $rs, $rt, $imm",
                       (SLTiu_MM GPR32Opnd:$rs, GPR32Opnd:$rt,
                                 simm32_relaxed:$imm), 0>;

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1694
@@ +1693,3 @@
+              SLTI_FM<0xb>;
+  def LUi   : MMRel, LoadUpper<"lui", GPR32Opnd, uimm16_relaxed>, LUI_FM;
+
----------------
Move LUi out of this NotInMicromips block.

================
Comment at: test/CodeGen/Mips/brconlt.ll:17
@@ -15,2 +16,3 @@
 ; 16:	slt	${{[0-9]+}}, ${{[0-9]+}}
+; MM32R6:   slt ${{[0-9]+}}, ${{[0-9]+}}
 ; 16:	btnez	$[[LABEL:[0-9A-Ba-b_]+]]
----------------
Formatting: line up the sltius in the immediate block so that they are all in the same column, one space after the longest check prefix. And in the other cases below.

================
Comment at: test/CodeGen/Mips/setcc-se.ll:9
@@ -7,1 +8,3 @@
+; MMR6: sltiu ${{[0-9]+}}, $4, 1
+; MMR6: SLTiu_MM
 
----------------
Can you instead match this with "<MCInst #{{[0-9]+}} SLTiu_MM" ? And the other cases below, as it a better match to the expected output.


http://reviews.llvm.org/D19906