[mips] Match 'ins' and its' variants with C++ code

[sdardis]

Summary

Change the ISel matching of 'ins', 'dins[mu]' from tablegen code to
C++ code. This resolves an issue where ISel would select 'dins' instead
of 'dinsm' when the instructions size and position were individually in
range but their sum was out of range according to the ISA specification.

[atanasyan]

lib/Target/Mips/MipsSEISelDAGToDAG.cpp

933 ↗

(On Diff #119642)

In the code below if Pos is greater than zero and Size is 0 and Node->getValueType(0) is equal to MVT::i32, the function returns false. But if Node->getValueType(0) is equal to MVT::i64, the function returns true and replaces the node by the Mips::DINS opcode. Does it conform the specification?

935 ↗

(On Diff #119642)

• 0 <= Pos && Pos < 32 can be changed to Pos < 32 because the Pos is unsigned integer.
• I think 0 < Size <= 32 should be changed to the 0 < Size && Size <= 32

938 ↗

(On Diff #119642)

Else-after-return https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

[sdardis]

Address review comments.

I've reflowed the logic for handling dins* to match the ISA documentation, with the exception of an early return if the size is zero.

lib/Target/Mips/MipsSEISelDAGToDAG.cpp

933 ↗

(On Diff #119642)

If size is zero, then it doesn't match the specification for any of the cases. I've changed the logic accordingly.

935 ↗

(On Diff #119642)

Thanks for spotting, I've reflowed the logic around these checks.

[atanasyan]

lib/Target/Mips/MipsSEISelDAGToDAG.cpp

927 ↗

(On Diff #120036)

Now the code below is okay, but i think we can write it a bit more clear. I think the following variant is logically equal to the original one. But I do not force to use it.

uint64_t MVT = Node->getConstantOperandVal(0);
uint64_t Pos = Node->getConstantOperandVal(1);
uint64_t Size = Node->getConstantOperandVal(2);

// Size has to be >0 for 'ins', 'dins' and 'dinsu'.
if (!Size)
  return false;

if (Size + Pos > 64)
  return false;

unsigned Opcode = 0;
if (MVT == MVT::i32) {
  if (Pos + Size <= 32)
    Opcode = Mips::INS;
} else {
  if (Pos + Size <= 32)
    Opcode = Mips::DINS;
  else if (Pos < 32)
    Opcode = Mips::DINSM;
  else
    Opcode = Mips::DINSU;
}

if (Opcode) {
  SDValue Ops[4] = {
          MVT, CurDAG->getTargetConstant(Pos, DL, MVT::i32),
          CurDAG->getTargetConstant(Size, DL, MVT::i32), Node->getOperand(3)};

  ReplaceNode(Node, CurDAG->getMachineNode(Opcode, DL, MVT, Ops));
  return true;
}

return false;

[sdardis]

Address review comments.

[sdardis]

lib/Target/Mips/MipsSEISelDAGToDAG.cpp

927 ↗

(On Diff #120036)

I agree, I think this is clearer.

[atanasyan]

LGTM

llvm/trunk/lib/Target/Mips/MicroMips64r6InstrInfo.td

class DCLZ_MM64R6_DESC {
   list<dag> Pattern = [(set GPR64Opnd:$rt, (ctlz GPR64Opnd:$rs))];
   InstrItinClass Itinerary = II_DCLZ;
   Format Form = FrmR;
   string BaseOpcode = "dclz";
 }
 
 class DINSU_MM64R6_DESC : InsBase<"dinsu", GPR64Opnd, uimm5_plus32,
                                   uimm5_inssize_plus1, immZExt5Plus32,
-                                  immZExt5Plus1, MipsIns>;
+                                  immZExt5Plus1>;
 class DINSM_MM64R6_DESC : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64,
-                                  immZExt5, immZExtRange2To64, MipsIns>;
+                                  immZExt5, immZExtRange2To64>;
 class DINS_MM64R6_DESC : InsBase<"dins", GPR64Opnd, uimm5_report_uimm6,
-                                 uimm5_inssize_plus1, immZExt5, immZExt5Plus1,
-                                 MipsIns>;
+                                 uimm5_inssize_plus1, immZExt5, immZExt5Plus1>;
 class DMTC0_MM64R6_DESC : MTC0_MMR6_DESC_BASE<"dmtc0", COP0Opnd, GPR64Opnd,
                                               II_DMTC0>;
 class DMTC1_MM64R6_DESC : MTC1_MMR6_DESC_BASE<"dmtc1", FGR64Opnd, GPR64Opnd,
                                               II_DMTC1, bitconvert>;
 class DMTC2_MM64R6_DESC : MTC2_MMR6_DESC_BASE<"dmtc2", COP2Opnd, GPR64Opnd,
                                               II_DMTC2>;
 class DMFC0_MM64R6_DESC : MFC0_MMR6_DESC_BASE<"dmfc0", GPR64Opnd, COP0Opnd,
                                               II_DMFC0>;

llvm/trunk/lib/Target/Mips/MicroMipsInstrInfo.td

let DecoderNamespace = "MicroMips", Predicates = [InMicroMips] in {
 
   /// Word Swap Bytes Within Halfwords
   def WSBH_MM : MMRel, SubwordSwap<"wsbh", GPR32Opnd, II_WSBH>,
                 SEB_FM_MM<0x1ec>, ISA_MIPS32R2;
   // TODO: Add '0 < pos+size <= 32' constraint check to ext instruction
   def EXT_MM : MMRel, ExtBase<"ext", GPR32Opnd, uimm5, uimm5_plus1, immZExt5,
                               immZExt5Plus1, MipsExt>, EXT_FM_MM<0x2c>;
   def INS_MM : MMRel, InsBase<"ins", GPR32Opnd, uimm5, uimm5_inssize_plus1,
-                              immZExt5, immZExt5Plus1, MipsIns>,
+                              immZExt5, immZExt5Plus1>,
                EXT_FM_MM<0x0c>;
 
   /// Jump Instructions
   let DecoderMethod = "DecodeJumpTargetMM" in {
     def J_MM        : MMRel, JumpFJ<jmptarget_mm, "j", br, bb, "j">,
                       J_FM_MM<0x35>;
     def JAL_MM      : MMRel, JumpLink<"jal", calltarget_mm>, J_FM_MM<0x3d>;
     def JALX_MM     : MMRel, JumpLink<"jalx", calltarget>, J_FM_MM<0x3c>;

llvm/trunk/lib/Target/Mips/Mips64InstrInfo.td

let AdditionalPredicates = [NotInMicroMips] in {
   // The 'pos + size' constraints for code generation are enforced by the
   // code that lowers into MipsISD::Ins.
   // For assembly parsing, we alias dinsu and dinsm to dins, and match by
   // operand were possible then check the 'pos + size' in MipsAsmParser.
   // We override the generated decoder to enforce that dins always comes out
   // for dinsm and dinsu like binutils.
   let DecoderMethod = "DecodeDINS" in {
     def DINS  : InsBase<"dins", GPR64Opnd, uimm6, uimm5_inssize_plus1,
-                        immZExt5, immZExt5Plus1, MipsIns>, EXT_FM<7>,
+                        immZExt5, immZExt5Plus1>, EXT_FM<7>,
                 ISA_MIPS64R2;
     def DINSU : InsBase<"dinsu", GPR64Opnd, uimm5_plus32, uimm5_inssize_plus1,
-                        immZExt5Plus32, immZExt5Plus1, MipsIns>,
+                        immZExt5Plus32, immZExt5Plus1>,
                 EXT_FM<6>, ISA_MIPS64R2;
     def DINSM : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64,
-                        immZExt5, immZExtRange2To64, MipsIns>,
+                        immZExt5, immZExtRange2To64>,
                 EXT_FM<5>, ISA_MIPS64R2;
   }
 }
 
 let isCodeGenOnly = 1, AdditionalPredicates = [NotInMicroMips] in {
   def DEXT64_32 : InstSE<(outs GPR64Opnd:$rt),
                          (ins GPR32Opnd:$rs, uimm5_report_uimm6:$pos,
                               uimm5_plus1:$size),

llvm/trunk/lib/Target/Mips/MipsInstrInfo.td

 class ExtBase<string opstr, RegisterOperand RO, Operand PosOpnd,
               Operand SizeOpnd, PatFrag PosImm, PatFrag SizeImm,
               SDPatternOperator Op = null_frag> :
   InstSE<(outs RO:$rt), (ins RO:$rs, PosOpnd:$pos, SizeOpnd:$size),
          !strconcat(opstr, " $rt, $rs, $pos, $size"),
          [(set RO:$rt, (Op RO:$rs, PosImm:$pos, SizeImm:$size))], II_EXT,
          FrmR, opstr>, ISA_MIPS32R2;
 
+// 'ins' and its' 64 bit variants are matched by C++ code.
 class InsBase<string opstr, RegisterOperand RO, Operand PosOpnd,
-              Operand SizeOpnd, PatFrag PosImm, PatFrag SizeImm,
-              SDPatternOperator Op = null_frag>:
+              Operand SizeOpnd, PatFrag PosImm, PatFrag SizeImm>:
   InstSE<(outs RO:$rt), (ins RO:$rs, PosOpnd:$pos, SizeOpnd:$size, RO:$src),
          !strconcat(opstr, " $rt, $rs, $pos, $size"),
-         [(set RO:$rt, (Op RO:$rs, PosImm:$pos, SizeImm:$size, RO:$src))],
+         [(set RO:$rt, (null_frag RO:$rs, PosImm:$pos, SizeImm:$size,
+                                  RO:$src))],
          II_INS, FrmR, opstr>, ISA_MIPS32R2 {
   let Constraints = "$src = $rt";
 }
 
 // Atomic instructions with 2 source operands (ATOMIC_SWAP & ATOMIC_LOAD_*).
 class Atomic2Ops<PatFrag Op, RegisterClass DRC> :
   PseudoSE<(outs DRC:$dst), (ins PtrRC:$ptr, DRC:$incr),
            [(set DRC:$dst, (Op iPTR:$ptr, DRC:$incr))]>;
def PseudoUDIV : MultDivPseudo<UDIV, ACC64, GPR32Opnd, MipsDivRemU, II_DIVU,
                                  0, 1, 1>, ISA_MIPS1_NOT_32R6_64R6;
   def RDHWR : MMRel, ReadHardware<GPR32Opnd, HWRegsOpnd>, RDHWR_FM;
   // TODO: Add '0 < pos+size <= 32' constraint check to ext instruction
   def EXT : MMRel, StdMMR6Rel, ExtBase<"ext", GPR32Opnd, uimm5, uimm5_plus1,
                                        immZExt5, immZExt5Plus1, MipsExt>,
             EXT_FM<0>;
   def INS : MMRel, StdMMR6Rel, InsBase<"ins", GPR32Opnd, uimm5,
                                        uimm5_inssize_plus1, immZExt5,
-                                       immZExt5Plus1, MipsIns>,
+                                       immZExt5Plus1>,
             EXT_FM<4>;
 }
 /// Move Control Registers From/To CPU Registers
 let AdditionalPredicates = [NotInMicroMips] in {
   def MTC0 : MTC3OP<"mtc0", COP0Opnd, GPR32Opnd, II_MTC0>, MFC3OP_FM<0x10, 4>,
              ISA_MIPS32;
   def MFC0 : MFC3OP<"mfc0", GPR32Opnd, COP0Opnd, II_MFC0>, MFC3OP_FM<0x10, 0>,
              ISA_MIPS32;

llvm/trunk/lib/Target/Mips/MipsSEISelDAGToDAG.cpp

case Intrinsic::mips_ctcmsa: {
                                               getMSACtrlReg(RegIdx), Value);
       ReplaceNode(Node, ChainOut.getNode());
       return true;
     }
     }
     break;
   }
 
+  // Manually match MipsISD::Ins nodes to get the correct instruction. It has
+  // to be done in this fashion so that we respect the differences between
+  // dins and dinsm, as the difference is that the size operand has the range
+  // 0 < size <= 32 for dins while dinsm has the range 2 <= size <= 64 which
+  // means SelectionDAGISel would have to test all the operands at once to
+  // match the instruction.
+  case MipsISD::Ins: {
+
+    // Sanity checking for the node operands.
+    if (Node->getValueType(0) != MVT::i32 && Node->getValueType(0) != MVT::i64)
+      return false;
+
+    if (Node->getNumOperands() != 4)
+      return false;
+
+    if (Node->getOperand(1)->getOpcode() != ISD::Constant ||
+        Node->getOperand(2)->getOpcode() != ISD::Constant)
+      return false;
+
+    MVT ResTy = Node->getSimpleValueType(0);
+    uint64_t Pos = Node->getConstantOperandVal(1);
+    uint64_t Size = Node->getConstantOperandVal(2);
+
+    // Size has to be >0 for 'ins', 'dins' and 'dinsu'.
+    if (!Size)
+      return false;
+
+    if (Pos + Size > 64)
+      return false;
+
+    if (ResTy != MVT::i32 && ResTy != MVT::i64)
+      return false;
+
+    unsigned Opcode = 0;
+    if (ResTy == MVT::i32) {
+      if (Pos + Size <= 32)
+        Opcode = Mips::INS;
+    } else {
+      if (Pos + Size <= 32)
+        Opcode = Mips::DINS;
+      else if (Pos < 32 && 1 < Size)
+        Opcode = Mips::DINSM;
+      else
+        Opcode = Mips::DINSU;
+    }
+
+    if (Opcode) {
+      SDValue Ops[4] = {
+          Node->getOperand(0), CurDAG->getTargetConstant(Pos, DL, MVT::i32),
+          CurDAG->getTargetConstant(Size, DL, MVT::i32), Node->getOperand(3)};
+
+      ReplaceNode(Node, CurDAG->getMachineNode(Opcode, DL, ResTy, Ops));
+      return true;
+    }
+
+    return false;
+  }
+
   case MipsISD::ThreadPointer: {
     EVT PtrVT = getTargetLowering()->getPointerTy(CurDAG->getDataLayout());
     unsigned RdhwrOpc, DestReg;
 
     if (PtrVT == MVT::i32) {
       RdhwrOpc = Mips::RDHWR;
       DestReg = Mips::V1;
     } else {

llvm/trunk/test/CodeGen/Mips/dins.ll

-; RUN: llc -O2 -march=mips64 -mcpu=mips64r2 -target-abi=n64 < %s -o - | FileCheck %s -check-prefix=MIPS64R2
-; RUN: llc -O2 -march=mips -mcpu=mips32r2 < %s -o - | FileCheck %s -check-prefix=MIPS32R2
-; RUN: llc -O2 -march=mips -mattr=mips16 < %s -o - | FileCheck %s -check-prefix=MIPS16
-; RUN: llc -O2 -march=mips64 -mcpu=mips64r2 -target-abi=n32 < %s -o - | FileCheck %s -check-prefix=MIPS64R2N32
+; RUN: llc -O2 -verify-machineinstrs -march=mips64 -mcpu=mips64r2 \
+; RUN:   -target-abi=n64 < %s -o - | FileCheck %s -check-prefix=MIPS64R2
+; RUN: llc -O2 -verify-machineinstrs -march=mips -mcpu=mips32r2 < %s -o - \
+; RUN:   | FileCheck %s -check-prefix=MIPS32R2
+; RUN: llc -O2 -verify-machineinstrs -march=mips -mattr=mips16 < %s -o - \
+; RUN:   | FileCheck %s -check-prefix=MIPS16
+; RUN: llc -O2 -verify-machineinstrs -march=mips64 -mcpu=mips64r2 \
+; RUN:   -target-abi=n32 < %s -o - | FileCheck %s -check-prefix=MIPS64R2N32
 
 ; #include <stdint.h>
 ; #include <stdio.h>
 ; struct cvmx_buf_ptr {
 
 ;   struct {
 ;     unsigned long long addr :37;
 ;     unsigned long long addr1 :15;
entry:
   %bf.lshr18 = lshr i64 %bufptr.sroa.0.0.bufptr.sroa.0.0.bufptr.sroa.0.0.bf.load17, 27
   ret i64 %bf.lshr18
 }
 
 ; CHECK-LABEL: f123:
 ; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 123
 ; MIPS64R2: dinsm   $[[R0:[0-9]+]], $[[R1:[0-9]+]], 27, 37
 ; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 4
-; MIPS64R2: dins    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 28, 6
+; MIPS64R2: dinsm   $[[R0:[0-9]+]], $[[R1:[0-9]+]], 28, 6
 ; MIPS64R2: daddiu  $[[R0:[0-9]+]], $zero, 5
 ; MIPS64R2: dinsu   $[[R0:[0-9]+]], $[[R1:[0-9]+]], 50, 14
 ; MIPS64R2: dsrl    $[[R0:[0-9]+]], $[[R1:[0-9]+]], 50
 ; MIPS64R2: dinsu   $[[R0:[0-9]+]], $[[R1:[0-9]+]], 34, 16
 ; MIPS32R2: ins     $[[R0:[0-9]+]], $[[R1:[0-9]+]], 2, 16
 ; MIPS32R2-NOT: ins $[[R0:[0-9]+]], $[[R1:[0-9]+]], 18, 46
 ; MIPS16-NOT: ins{{[[:space:]].*}}