MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

src/compiler/mips/code-generator-mips.cc

Index: src/compiler/mips/code-generator-mips.cc
diff --git a/src/compiler/mips/code-generator-mips.cc b/src/compiler/mips/code-generator-mips.cc
index a581ca5c3ae02a6eaa946e3d9258b2aff81a1739..6dcadbb791a787992e34e0df919ab781d97827fa 100644
--- a/src/compiler/mips/code-generator-mips.cc
+++ b/src/compiler/mips/code-generator-mips.cc
@@ -721,6 +721,62 @@ void CodeGenerator::AssembleArchInstruction(Instruction* instr) {
       ASSEMBLE_ROUND_DOUBLE_TO_DOUBLE(ceil_l_d, Ceil);
       break;
     }
+    case kMipsFloat64Max: {
+      // (b < a) ? a : b
+      if (IsMipsArchVariant(kMips32r6)) {
+        __ cmp(OLT, L, kScratchDoubleReg, i.InputDoubleRegister(0),

[paul.l..., 2015/11/02 22:35:11]

You're putting the result of cmp in scratch, but due to semantics of sel_d() it
needs to be in output register.

[dusan.milosavljevic, 2015/11/03 01:51:05]

Done.

+               i.InputDoubleRegister(1));
+        __ sel_d(i.OutputSingleRegister(), i.InputDoubleRegister(1),
+                 kScratchDoubleReg);

[paul.l..., 2015/11/02 22:35:11]

s/b: sel_d(i.OutputDoubleRegister(),
i.InputDoubleRegister(0),i.InputDoubleRegister(1))

from manual: FPR[fd] 􏰀= FPR[fd].bit0 ? FPR[ft] : FPR[fs]

I don't see how this is working, unless we have insufficient test coverage.

[dusan.milosavljevic, 2015/11/03 01:51:05]

Done.

Different instruction from other sel. instruction, i assumed they are similar,
but this one is not.

+      } else {
+        __ c(OLT, D, i.InputDoubleRegister(0), i.InputDoubleRegister(1));
+        // Left operand is result, passthrough if false.
+        __ movt_d(i.OutputSingleRegister(), i.InputDoubleRegister(1));

[paul.l..., 2015/11/02 22:35:12]

All the Float64 output regs are show as OutputSingleRegister(), but should be
OutputDoubleRegister().

I do realize they are aliases, but we should use them consistently.

[dusan.milosavljevic, 2015/11/03 01:51:05]

Done.

+      }
+      break;
+    }
+    case kMipsFloat64Min: {
+      // (a < b) ? a : b
+      if (IsMipsArchVariant(kMips32r6)) {
+        __ cmp(OLT, L, kScratchDoubleReg, i.InputDoubleRegister(0),

[paul.l..., 2015/11/02 22:35:11]

nit: we have asm aliases for cmp_d() and cmp_s(), you should use those here, and
below. Using the raw cmp() with L & W types is correct per the encodings, but
its a cognitive dissonance, since every other usage of L & W are for ints, and D
& S are the common usage for floats. (above, here, and below)

[dusan.milosavljevic, 2015/11/03 01:51:05]

Done.

+               i.InputDoubleRegister(1));
+        __ sel_d(i.OutputSingleRegister(), i.InputDoubleRegister(1),
+                 kScratchDoubleReg);
+      } else {
+        __ c(OLT, D, i.InputDoubleRegister(1), i.InputDoubleRegister(0));
+        // Right operand is result, passthrough if false.
+        __ movt_d(i.OutputSingleRegister(), i.InputDoubleRegister(1));
+      }
+      break;
+    }
+    case kMipsFloat32Max: {
+      // (b < a) ? a : b
+      if (IsMipsArchVariant(kMips32r6)) {
+        __ cmp(OLT, W, kScratchDoubleReg, i.InputDoubleRegister(0),
+               i.InputDoubleRegister(1));
+        __ sel_s(i.OutputSingleRegister(), i.InputDoubleRegister(1),
+                 kScratchDoubleReg);
+      } else {
+        __ c(OLT, S, i.InputDoubleRegister(0), i.InputDoubleRegister(1));
+        // Left operand is result, passthrough if false.
+        __ movt_s(i.OutputSingleRegister(), i.InputDoubleRegister(1));
+      }
+      break;
+    }
+    case kMipsFloat32Min: {
+      // (a < b) ? a : b
+      if (IsMipsArchVariant(kMips32r6)) {
+        __ cmp(OLT, W, kScratchDoubleReg, i.InputDoubleRegister(0),
+               i.InputDoubleRegister(1));
+        __ sel_d(i.OutputSingleRegister(), i.InputDoubleRegister(1),
+                 kScratchDoubleReg);
+      } else {
+        __ c(OLT, S, i.InputDoubleRegister(1), i.InputDoubleRegister(0));
+        // Right operand is result, passthrough if false.
+        __ movt_s(i.OutputSingleRegister(), i.InputDoubleRegister(1));
+      }
+      break;
+    }
     case kMipsCvtSD: {
       __ cvt_s_d(i.OutputSingleRegister(), i.InputDoubleRegister(0));
       break;