[PATCH] [mips] Refine octeon cins/cins32/exts/exts32 instruction.

[Daniel Sanders]

I've got a couple comments.


================
Comment at: lib/Target/Mips/Mips64InstrInfo.td:379-388
@@ -377,8 +378,12 @@
 // Extract a signed bit field /+32
-def EXTS  : ExtsCins<"exts">, EXTS_FM<0x3a>;
-def EXTS32: ExtsCins<"exts32">, EXTS_FM<0x3b>;
+def EXTS  : ExtsCins<"exts", GPR64Opnd, MipsExtS>, EXTS_FM<0x3a>;
+def EXTS32: ExtsCins<"exts32", GPR64Opnd, MipsExtS32>, EXTS_FM<0x3b>;
+let isCodeGenOnly = 1 in
+  def EXTS_i32 : ExtsCins<"exts", GPR32Opnd, MipsExtS>, EXTS_FM<0x3a>;
 
 // Clear and insert a bit field /+32
-def CINS  : ExtsCins<"cins">, EXTS_FM<0x32>;
-def CINS32: ExtsCins<"cins32">, EXTS_FM<0x33>;
+def CINS  : ExtsCins<"cins", GPR64Opnd, MipsCIns>, EXTS_FM<0x32>;
+def CINS32: ExtsCins<"cins32", GPR64Opnd, MipsCIns32>, EXTS_FM<0x33>;
+let isCodeGenOnly = 1 in
+  def CINS_i32 : ExtsCins<"cins", GPR32Opnd, MipsCIns>, EXTS_FM<0x32>;
 
----------------
The two isCodeGenOnly's are to resolve the decoder conflict aren't they?

If you set 'DecoderNamespace = "Mips64"'  on EXTS, EXTS32, CINS, and CINS32 then you'll resolve the decoder conflict and the disassembly will still disassemble all six cases.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:816-817
@@ +815,4 @@
+    Pos -= 32;
+  }
+  else
+    Opc = MipsISD::CIns;
----------------
Else should be on the same line as the '}'

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:857-858
@@ +856,4 @@
+  // The left shift amount must not exceed the right shift amount.
+  if (Right >= ValTy.getSizeInBits() || Left >= ValTy.getSizeInBits()
+      || Left > Right)
+    return SDValue();
----------------
Personally I prefer to line-wrap before the '||' like this but LLVM code normally line-wraps after the '||' and it's best to be consistent.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:872-873
@@ +871,4 @@
+    Pos -= 32;
+  }
+  else
+    Opc = MipsISD::ExtS;
----------------
Else should be on the same line as the '}'

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:130-133
@@ +129,6 @@
+// Octeon nodes for cins/cins32/exts/exts32
+def MipsCIns   :  SDNode<"MipsISD::CIns", SDT_Ext>;
+def MipsCIns32 :  SDNode<"MipsISD::CIns32", SDT_Ext>;
+def MipsExtS   :  SDNode<"MipsISD::ExtS", SDT_Ext>;
+def MipsExtS32 :  SDNode<"MipsISD::ExtS32", SDT_Ext>;
+
----------------
I'd prefer to have two nodes (MipsCIns, and MipsExtS) and handle this CIns32/ExtS32 cases the same way you did BBIT0/BBIT032 (i.e. with a special ImmLeaf/PatLeaf and SDNodeXForm).

http://reviews.llvm.org/D7048

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/