[mips] Pick the right variant of DINS upfront and enable target instruction verification

This patch complements D16810 "[mips] Make isel select the correct DEXT variant
up front.". Now ISel picks the right variant of DINS, so now there is no need
to replace DINS with the appropriate variant during
MipsMCCodeEmitter::encodeInstruction().

This patch also enables target specific instruction verification for ins, dins,
dinsm, dinsu, ext, dext, dextm, dextu. These instructions have constraints that
are checked when generating MipsISD::Ins and MipsISD::Ext nodes, but these
constraints are not checked during instruction selection. Adding machine
verification should catch outstanding cases.

Finally, correct a bug that instruction verification uncovered, where the
position operand of a DINSU generated during lowering was being silently
and accidently corrected to the correct value.

Reviewers: slthakur

Differential Revision: https://reviews.llvm.org/D34809

llvm-svn: 313254

8 files changed, 119 insertions, 44 deletions

diff --git a/llvm/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp b/llvm/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
index 0330824fd61..12f7638594d 100644
--- a/llvm/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
+++ b/llvm/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
@@ -101,33 +101,6 @@ static void LowerLargeShift(MCInst& Inst) {
   }
 }
 
-// Pick a DINS instruction variant based on the pos and size operands
-static void LowerDins(MCInst& InstIn) {
-  assert(InstIn.getNumOperands() == 5 &&
-         "Invalid no. of machine operands for DINS!");
-
-  assert(InstIn.getOperand(2).isImm());
-  int64_t pos = InstIn.getOperand(2).getImm();
-  assert(InstIn.getOperand(3).isImm());
-  int64_t size = InstIn.getOperand(3).getImm();
-
-  assert((pos + size) <= 64 &&
-         "DINS cannot have position plus size over 64");
-  if (pos < 32) {
-    if ((pos + size) > 0 && (pos + size) <= 32)
-      return; // DINS, do nothing
-    else if ((pos + size) > 32) {
-      //DINSM
-      InstIn.getOperand(3).setImm(size - 32);
-      InstIn.setOpcode(Mips::DINSM);
-    }
-  } else if ((pos + size) > 32 && (pos + size) <= 64) {
-    // DINSU
-    InstIn.getOperand(2).setImm(pos - 32);
-    InstIn.setOpcode(Mips::DINSU);
-  }
-}
-
 // Fix a bad compact branch encoding for beqc/bnec.
 void MipsMCCodeEmitter::LowerCompactBranch(MCInst& Inst) const {
   // Encoding may be illegal !(rs < rt), but this situation is
@@ -211,10 +184,6 @@ encodeInstruction(const MCInst &MI, raw_ostream &OS,
   case Mips::DROTR_MM64R6:
     LowerLargeShift(TmpInst);
     break;
-    // Double extract instruction is chosen by pos and size operands
-  case Mips::DINS:
-    LowerDins(TmpInst);
-    break;
   // Compact branches, enforce encoding restrictions.
   case Mips::BEQC:
   case Mips::BNEC:
diff --git a/llvm/lib/Target/Mips/MicroMips64r6InstrInfo.td b/llvm/lib/Target/Mips/MicroMips64r6InstrInfo.td
index 38b09d105dd..a37b31643ac 100644
--- a/llvm/lib/Target/Mips/MicroMips64r6InstrInfo.td
+++ b/llvm/lib/Target/Mips/MicroMips64r6InstrInfo.td
@@ -164,10 +164,12 @@ class DCLZ_MM64R6_DESC {
 }
 
 class DINSU_MM64R6_DESC : InsBase<"dinsu", GPR64Opnd, uimm5_plus32,
-                                  uimm5_inssize_plus1, MipsIns>;
-class DINSM_MM64R6_DESC : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64>;
+                                  uimm5_inssize_plus1, immZExt5Plus32,
+                                  immZExt5Plus1, MipsIns>;
+class DINSM_MM64R6_DESC : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64,
+                                  immZExt5, immZExtRange2To64, MipsIns>;
 class DINS_MM64R6_DESC : InsBase<"dins", GPR64Opnd, uimm5, uimm5_inssize_plus1,
-                                 MipsIns>;
+                                 immZExt5, immZExt5Plus1,  MipsIns>;
 class DMTC0_MM64R6_DESC : MTC0_MMR6_DESC_BASE<"dmtc0", COP0Opnd, GPR64Opnd,
                                               II_DMTC0>;
 class DMTC1_MM64R6_DESC : MTC1_MMR6_DESC_BASE<"dmtc1", FGR64Opnd, GPR64Opnd,
diff --git a/llvm/lib/Target/Mips/MicroMipsInstrInfo.td b/llvm/lib/Target/Mips/MicroMipsInstrInfo.td
index ee554bc7f69..75a0ca30c11 100644
--- a/llvm/lib/Target/Mips/MicroMipsInstrInfo.td
+++ b/llvm/lib/Target/Mips/MicroMipsInstrInfo.td
@@ -884,7 +884,8 @@ let DecoderNamespace = "MicroMips", Predicates = [InMicroMips] in {
   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,
-                              MipsIns>, EXT_FM_MM<0x0c>;
+                              immZExt5, immZExt5Plus1, MipsIns>,
+               EXT_FM_MM<0x0c>;
 
   /// Jump Instructions
   let DecoderMethod = "DecodeJumpTargetMM" in {
diff --git a/llvm/lib/Target/Mips/Mips64InstrInfo.td b/llvm/lib/Target/Mips/Mips64InstrInfo.td
index 5082e4bab62..a5c3096739c 100644
--- a/llvm/lib/Target/Mips/Mips64InstrInfo.td
+++ b/llvm/lib/Target/Mips/Mips64InstrInfo.td
@@ -327,11 +327,13 @@ let AdditionalPredicates = [NotInMicroMips] in {
   def DEXTU : ExtBase<"dextu", GPR64Opnd, uimm5_plus32, uimm5_plus1,
                       immZExt5Plus32, immZExt5Plus1, MipsExt>, EXT_FM<2>,
                       ISA_MIPS64R2;
-  def DINS : InsBase<"dins", GPR64Opnd, uimm6, uimm5_inssize_plus1, MipsIns>,
-             EXT_FM<7>, ISA_MIPS64R2;
-  def DINSU : InsBase<"dinsu", GPR64Opnd, uimm5_plus32, uimm5_inssize_plus1>,
+  def DINS : InsBase<"dins", GPR64Opnd, uimm6, uimm5_inssize_plus1, immZExt5,
+                      immZExt5Plus1, MipsIns>, EXT_FM<7>, ISA_MIPS64R2;
+  def DINSU : InsBase<"dinsu", GPR64Opnd, uimm5_plus32, uimm5_inssize_plus1,
+                      immZExt5Plus32, immZExt5Plus1, MipsIns>,
               EXT_FM<6>, ISA_MIPS64R2;
-  def DINSM : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64>,
+  def DINSM : InsBase<"dinsm", GPR64Opnd, uimm5, uimm_range_2_64,
+                      immZExt5, immZExtRange2To64, MipsIns>,
               EXT_FM<5>, ISA_MIPS64R2;
 }
 
diff --git a/llvm/lib/Target/Mips/MipsInstrInfo.cpp b/llvm/lib/Target/Mips/MipsInstrInfo.cpp
index 92b1e3440f8..82dc442a53d 100644
--- a/llvm/lib/Target/Mips/MipsInstrInfo.cpp
+++ b/llvm/lib/Target/Mips/MipsInstrInfo.cpp
@@ -531,3 +531,90 @@ bool MipsInstrInfo::findCommutedOpIndices(MachineInstr &MI, unsigned &SrcOpIdx1,
   }
   return TargetInstrInfo::findCommutedOpIndices(MI, SrcOpIdx1, SrcOpIdx2);
 }
+
+// ins, ext, dext*, dins have the following constraints:
+// 0 <= pos      <  X
+// 0 <  size     <= X
+// 0 <  pos+size <= x
+//
+// dinsm and dinsm have the following contraints:
+// 0 <= pos      <  X
+// 0 <= size     <= X
+// 0 <  pos+size <= x
+
+static bool verifyInsExtInstruction(const MachineInstr &MI, StringRef &ErrInfo,
+                                    const int64_t PosLow, const int64_t PosHigh,
+                                    const int64_t SizeLow,
+                                    const int64_t SizeHigh,
+                                    const int64_t BothLow,
+                                    const int64_t BothHigh) {
+  MachineOperand MOPos = MI.getOperand(2);
+  if (!MOPos.isImm()) {
+    ErrInfo = "Position is not an immediate!";
+    return false;
+  }
+  int64_t Pos = MOPos.getImm();
+  if (!((PosLow <= Pos) && (Pos < PosHigh))) {
+    ErrInfo = "Position operand is out of range!";
+    return false;
+  }
+
+  MachineOperand MOSize = MI.getOperand(3);
+  if (!MOSize.isImm()) {
+    ErrInfo = "Size operand is not an immediate!";
+    return false;
+  }
+  int64_t Size = MOSize.getImm();
+  if (!((SizeLow < Size) && (Size <= SizeHigh))) {
+    ErrInfo = "Size operand is out of range!";
+    return false;
+  }
+
+  if (!((BothLow < (Pos + Size)) && ((Pos + Size) <= BothHigh))) {
+    ErrInfo = "Position + Size is out of range!";
+    return false;
+  }
+
+  return true;
+}
+
+//  Perform target specific instruction verification.
+bool MipsInstrInfo::verifyInstruction(const MachineInstr &MI,
+                                      StringRef &ErrInfo) const {
+  // Verify that ins and ext instructions are well formed.
+  switch (MI.getOpcode()) {
+    case Mips::EXT:
+    case Mips::EXT_MM:
+    case Mips::INS:
+    case Mips::INS_MM:
+    case Mips::DINS:
+    case Mips::DINS_MM64R6:
+      return verifyInsExtInstruction(MI, ErrInfo, 0, 32, 0, 32, 0, 32);
+    case Mips::DINSM:
+    case Mips::DINSM_MM64R6:
+      // The ISA spec has a subtle difference here in that it says:
+      //  2 <= size <= 64 for 'dinsm', so we change the bounds so that it
+      // is in line with the rest of instructions.
+      return verifyInsExtInstruction(MI, ErrInfo, 0, 32, 1, 64, 32, 64);
+    case Mips::DINSU:
+    case Mips::DINSU_MM64R6:
+      // The ISA spec has a subtle difference here in that it says:
+      //  2 <= size <= 64 for 'dinsm', so we change the bounds so that it
+      // is in line with the rest of instructions.
+      return verifyInsExtInstruction(MI, ErrInfo, 32, 64, 1, 32, 32, 64);
+    case Mips::DEXT:
+    case Mips::DEXT_MM64R6:
+      return verifyInsExtInstruction(MI, ErrInfo, 0, 32, 0, 32, 0, 63);
+    case Mips::DEXTM:
+    case Mips::DEXTM_MM64R6:
+      return verifyInsExtInstruction(MI, ErrInfo, 0, 32, 32, 64, 32, 64);
+    case Mips::DEXTU:
+    case Mips::DEXTU_MM64R6:
+      return verifyInsExtInstruction(MI, ErrInfo, 32, 64, 0, 32, 32, 64);
+    default:
+      return true;
+  }
+
+  return true;
+}
+
diff --git a/llvm/lib/Target/Mips/MipsInstrInfo.h b/llvm/lib/Target/Mips/MipsInstrInfo.h
index d04bde9afb5..0830b57cd05 100644
--- a/llvm/lib/Target/Mips/MipsInstrInfo.h
+++ b/llvm/lib/Target/Mips/MipsInstrInfo.h
@@ -148,6 +148,10 @@ public:
   bool findCommutedOpIndices(MachineInstr &MI, unsigned &SrcOpIdx1,
                              unsigned &SrcOpIdx2) const override;
 
+  /// Perform target specific instruction verification.
+  bool verifyInstruction(const MachineInstr &MI,
+                         StringRef &ErrInfo) const override;
+
 protected:
   bool isZeroImm(const MachineOperand &op) const;
 
diff --git a/llvm/lib/Target/Mips/MipsInstrInfo.td b/llvm/lib/Target/Mips/MipsInstrInfo.td
index 12a84b10586..22410864d70 100644
--- a/llvm/lib/Target/Mips/MipsInstrInfo.td
+++ b/llvm/lib/Target/Mips/MipsInstrInfo.td
@@ -1205,6 +1205,11 @@ def immSExt16Plus1 : PatLeaf<(imm), [{
   return isInt<17>(N->getSExtValue()) && isInt<16>(N->getSExtValue() + 1);
 }]>;
 
+def immZExtRange2To64 : PatLeaf<(imm), [{
+  return isUInt<7>(N->getZExtValue()) && (N->getZExtValue() >= 2) &&
+         (N->getZExtValue() <= 64);
+}]>;
+
 // Mips Address Mode! SDNode frameindex could possibily be a match
 // since load and store instructions from stack used it.
 def addr :
@@ -1676,10 +1681,11 @@ class ExtBase<string opstr, RegisterOperand RO, Operand PosOpnd,
          FrmR, opstr>, ISA_MIPS32R2;
 
 class InsBase<string opstr, RegisterOperand RO, Operand PosOpnd,
-              Operand SizeOpnd, SDPatternOperator Op = null_frag>:
+              Operand SizeOpnd, PatFrag PosImm, PatFrag SizeImm,
+              SDPatternOperator Op = null_frag>:
   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, imm:$pos, imm:$size, RO:$src))],
+         [(set RO:$rt, (Op RO:$rs, PosImm:$pos, SizeImm:$size, RO:$src))],
          II_INS, FrmR, opstr>, ISA_MIPS32R2 {
   let Constraints = "$src = $rt";
 }
@@ -2183,7 +2189,8 @@ let AdditionalPredicates = [NotInMicroMips] in {
                                        immZExt5, immZExt5Plus1, MipsExt>,
             EXT_FM<0>;
   def INS : MMRel, StdMMR6Rel, InsBase<"ins", GPR32Opnd, uimm5,
-                                       uimm5_inssize_plus1, MipsIns>,
+                                       uimm5_inssize_plus1, immZExt5,
+                                       immZExt5Plus1, MipsIns>,
             EXT_FM<4>;
 }
 /// Move Control Registers From/To CPU Registers
diff --git a/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp b/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
index 4be26dd25dc..283fcaa73a7 100644
--- a/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
+++ b/llvm/lib/Target/Mips/MipsSEISelDAGToDAG.cpp
@@ -1188,9 +1188,12 @@ bool MipsSEDAGToDAGISel::trySelect(SDNode *Node) {
         // The obvious "missing" case is when both are zero, but that case is
         // handled by the ldi case.
         if (ResNonZero) {
+          IntegerType *Int32Ty =
+              IntegerType::get(MF->getFunction()->getContext(), 32);
+          const ConstantInt *Const32 = ConstantInt::get(Int32Ty, 32);
           SDValue Ops[4] = {HiResNonZero ? SDValue(HiRes, 0) : Zero64Val,
-                            CurDAG->getTargetConstant(64, DL, MVT::i32),
-                            CurDAG->getTargetConstant(32, DL, MVT::i32),
+                            CurDAG->getConstant(*Const32, DL, MVT::i32),
+                            CurDAG->getConstant(*Const32, DL, MVT::i32),
                             SDValue(Res, 0)};
 
           Res = CurDAG->getMachineNode(Mips::DINSU, DL, MVT::i64, Ops);