[PATCH] [mips] Support SELECT nodes for targets that don't have conditional-move instructions.

[Daniel Sanders]

Great! With this, we will be able to generate code for the mips/mipsel ports of Debian.

There are quite a few comments below but most are minor things. The main one is about whether we really need the SelectFP_[TF] nodes.

================
Comment at: lib/Target/Mips/MipsCondMov.td:269
@@ +268,3 @@
+// we have to match SELECT nodes with pseudo instructions.
+let usesCustomInserter = 1, Predicates = [HasNotCondMov] in {
+  class Select_Pseudo<RegisterOperand RC> :
----------------
Don't override the Predicates field, it has some surprising results since it's very easy to accidentally remove predicates. Instead override InsnPredicates, preferably using an ISA_* or INSN_* adjective. See the PredicateControl class for more information.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:536-537
@@ +535,4 @@
+  unsigned OpNode = (hasCondMov ?
+      (invert ? MipsISD::CMovFP_F : MipsISD::CMovFP_T) :
+      (invert ? MipsISD::SelectFP_F : MipsISD::SelectFP_T));
+
----------------
Given that CMovFP_[TF] and SelectFP_[TF] have the same inputs and outputs, do we really need new nodes? Couldn't we use CMovFP_[TF] and use predicates to pick between the CMov instructions (added in Mips4 and Mips32) and the new pseudo's?

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:189
@@ -188,2 +188,3 @@
                       AssemblerPredicate<"FeatureCnMips">;
+def HasNotCondMov :   Predicate<"!Subtarget->hasCondMov()">;
 def RelocStatic :     Predicate<"TM.getRelocationModel() == Reloc::Static">,
----------------
Nit: Based on the names of the other predicates this should either be NoCondMov or NotCondMov but, as noted above, it would be better to define and use an INSN_* or ISA_* adjective (e.g. ISA_NOT_4_32).

================
Comment at: lib/Target/Mips/MipsSubtarget.h:199-202
@@ -198,2 +198,6 @@
 
+  bool hasCondMov() const {
+    return MipsArchVersion >= Mips32 && MipsArchVersion != Mips3;
+  };
+
   bool isLittle() const { return IsLittle; }
----------------
This won't be needed once you do the above comments.

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:39
@@ +38,3 @@
+  ; SEL:    seleqz  $[[T1:[0-9]+]], $6, $[[T0]]
+  ; SEL:    selnez  $[[T0]], $5, $[[T0]]
+  ; SEL:    or      $2, $[[T0]], $[[T1]]
----------------
movn from the CMOV case has tied operands but this is not the case for seleqz/selnez. As a result, the first [[T0]] on this line isn't guaranteed to be the same register as the second [[T0]]. You should define another variable.

Likewise for the other tests below.

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:100-111
@@ +99,14 @@
+
+  ; M2:     andi    $[[T0:[0-9]+]], $4, 1
+  ; M2:     bnez    $[[T0]], $[[BB0:BB[0-9_]+]]
+  ; M2:     nop
+  ; M2:     lw      $[[T1:[0-9]+]], {{[0-9]+}}($sp)
+  ; M2:     $[[BB0]]:
+  ; M2:     bnez    $[[T0]], $[[BB1:BB[0-9_]+]]
+  ; M2:     nop
+  ; M2:     lw      $[[T2:[0-9]+]], {{[0-9]+}}($sp)
+  ; M2:     $[[BB1]]:
+  ; M2:     move    $2, $[[T1]]
+  ; M2:     jr      $ra
+  ; M2:     move    $3, $[[T2]]
+
----------------
Please add a FIXME commenting on the redundant control flow.

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:114
@@ +113,3 @@
+  ; CMOV-32:    andi    $[[T0:[0-9]+]], $4, 1
+  ; CMOV-32:    lw      $2, {{[0-9]+}}($sp)
+  ; CMOV-32:    movn    $2, $6, $[[T0]]
----------------
Please check the offsets as well.

Likewise in the tests below.

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:134
@@ +133,3 @@
+  ; SEL-64:     andi    $[[T0:[0-9]+]], $4, 1
+  ; SEL-64:     sll     $[[T0]], $[[T0]], 0
+  ; SEL-64:     seleqz  $[[T1:[0-9]+]], $6, $[[T0]]
----------------
This instruction is redundant. Could you add a FIXME comment?

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:142-173
@@ +141,34 @@
+
+define float @tst_select_i1_float(i1 signext %s, float %x, float %y) {
+entry:
+  ; ALL-LABEL: tst_select_i1_float:
+
+  ; M2:         andi    $[[T0:[0-9]+]], $4, 1
+  ; M2:         bnez    $[[T0]], $[[BB0:BB[0-9_]+]]
+  ; M2:         nop
+  ; M2:         jr      $ra
+  ; M2:         mtc1    $6, $f0
+  ; M2:         $[[BB0]]:
+  ; M2:         jr      $ra
+  ; M2:         mtc1    $5, $f0
+
+  ; CMOV-32:    mtc1    $6, $f0
+  ; CMOV-32:    mtc1    $5, $f1
+  ; CMOV-32:    andi    $[[T0:[0-9]+]], $4, 1
+  ; CMOV-32:    movn.s  $f0, $f1, $[[T0]]
+
+  ; SEL-32:     mtc1    $5, $[[F0:f[0-9]+]]
+  ; SEL-32:     mtc1    $6, $[[F1:f[0-9]+]]
+  ; SEL-32:     mtc1    $4, $f0
+  ; SEL-32:     sel.s   $f0, $[[F1]], $[[F0]]
+
+  ; CMOV-64:    andi    $[[T0:[0-9]+]], $4, 1
+  ; CMOV-64:    movn.s  $f14, $f13, $[[T0]]
+  ; CMOV-64:    mov.s   $f0, $f14
+
+  ; SEL-64:     mtc1    $4, $f0
+  ; SEL-64:     sel.s   $f0, $f14, $f13
+  %r = select i1 %s, float %x, float %y
+  ret float %r
+}
+
----------------
I should mention that for MIPS-II, this is testing selection between two GPR's. This is because of a quirk of the O32 ABI that prevents the use of FPR's if there is a non-floating point argument as the first argument. MIPS32 is testing selection between FPR's since it's copying $5 and $6 to $f0, and $f1 first.

We should duplicate this test with the arguments re-ordered (%x, %y, %s) so that we test the FPR case for 32-bit ISA's.

Likewise for the double test below.

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:327
@@ +326,3 @@
+
+
+define float @tst_select_fcmp_oeq_float(float %x, float %y) {
----------------
Nit: Too many blank lines

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:395
@@ +394,3 @@
+
+
+define double @tst_select_fcmp_olt_double(double %x, double %y) {
----------------
Nit: Too many blank lines

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:516
@@ +515,3 @@
+
+
+define double @tst_select_fcmp_oeq_double(double %x, double %y) {
----------------
Nit: Too many blank lines

http://reviews.llvm.org/D6212