[PATCH] D12798: [mips][micromips] Initial support for micrmomips DSP instructions and addu.qb implementation

[Daniel Sanders via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits.

I noticed you don't actually use DSPR3 yet but I expect will change in the next few patches.


================
Comment at: lib/Target/Mips/MicroMipsDSPInstrFormats.td:1-2
@@ +1,3 @@
+//===-- MicroMipsDSPInstrFormats.td - Micromips DSP Instruction Formats -*-
+//tablegen -*-===//
+//
----------------
Unfortunately, this has to be a single line and we have to fit into 80 cols as well. The '-*- tablegen -*-' is used by editors to configure for the specified language.

The 'Micromips DSP' part is probably the best bit to remove.

================
Comment at: lib/Target/Mips/MicroMipsDSPInstrFormats.td:13
@@ +12,3 @@
+    : MipsInst<(outs), (ins), "", [], NoItinerary, FrmOther>, PredicateControl {
+  let EncodingPredicates = [HasDSP];
+  string BaseOpcode = opstr;
----------------
HasDSP is not an encoding predicate, it belongs in InsnPredicates.

================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:935-936
@@ -934,3 +934,4 @@
 
-  if ((Ty != MVT::v2i16) && ((Ty != MVT::v4i8) || !Subtarget.hasDSPR2()))
+  if ((Ty != MVT::v2i16) && ((Ty != MVT::v4i8) || (!Subtarget.hasDSPR2() &&
+                                                   !Subtarget.hasDSPR3())))
     return SDValue();
----------------
!hasDSPR3() is redundant here. The predicates are cumulative so you have have DSPR3 without also having DSPR2.

================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:948-949
@@ -946,3 +947,4 @@
 
-  if (((Ty != MVT::v2i16) || !Subtarget.hasDSPR2()) && (Ty != MVT::v4i8))
+  if (((Ty != MVT::v2i16) || (!Subtarget.hasDSPR2() &&
+                              !Subtarget.hasDSPR3())) && (Ty != MVT::v4i8))
     return SDValue();
----------------
!hasDSPR3() is redundant here. The predicates are cumulative so you have have DSPR3 without also having DSPR2.


http://reviews.llvm.org/D12798