MIPS64: Fix Word32Compare turbofan operator implementation when comparing signed with unsigned oper…

src/compiler/mips64/instruction-selector-mips64.cc

Index: src/compiler/mips64/instruction-selector-mips64.cc
diff --git a/src/compiler/mips64/instruction-selector-mips64.cc b/src/compiler/mips64/instruction-selector-mips64.cc
index 6e937e20d7ae9fdc4c48245b4bf3f8e760504a8a..d4491d7b08c3af79f0d67f97184633cef928b995 100644
--- a/src/compiler/mips64/instruction-selector-mips64.cc
+++ b/src/compiler/mips64/instruction-selector-mips64.cc
@@ -1840,10 +1840,60 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
   }
 }
 
+bool IsNodeUnsigned(Node* n) {
+  NodeMatcher m(n);
+
+  if (m.IsLoad()) {
+    LoadRepresentation load_rep = LoadRepresentationOf(n->op());
+    return load_rep.IsUnsigned();
+  } else if (m.IsUnalignedLoad()) {
+    UnalignedLoadRepresentation load_rep =
+        UnalignedLoadRepresentationOf(n->op());
+    return load_rep.IsUnsigned();
+  } else {

[ahaas, 2016/10/13 08:10:54]

I think you also have to deal with parameter and phi nodes here.

[ivica.bogosavljevic, 2016/10/14 08:16:40]

Hi Ahaas!

I looked at the code: the Phi and Select operators from common-operators.h don't
store their representation using MachineType (that contains Unsigned/Signed
information), instead they use MachineRepresentation (that contains only
Word32/Word64).

[titzer, 2016/10/15 10:45:33]

Is it possible to add some extra --debug-code checks to verify in debug mode
that when this optimization occurs, it is correct? That will help flush out
bugs.

+    return m.IsUint32Div() || m.IsUint32LessThan() ||
+           m.IsUint32LessThanOrEqual() || m.IsUint32Mod() ||
+           m.IsUint32MulHigh() || m.IsChangeFloat64ToUint32() ||
+           m.IsTruncateFloat64ToUint32() || m.IsTruncateFloat32ToUint32();
+  }
+}
+
+// Shared routine for multiple word compare operations.
+void VisitSimulateWord32Compare(InstructionSelector* selector, Node* node,
+                                InstructionCode opcode, FlagsContinuation* cont,
+                                bool commutative) {
+  Mips64OperandGenerator g(selector);
+  Node* left = node->InputAt(0);
+  Node* right = node->InputAt(1);
+  InstructionOperand leftOp = g.TempRegister();
+  InstructionOperand rightOp = g.TempRegister();
+
+  selector->Emit(kMips64Dshl, leftOp, g.UseRegister(left), g.TempImmediate(32));
+  selector->Emit(kMips64Dshl, rightOp, g.UseRegister(right),
+                 g.TempImmediate(32));
+
+  VisitCompare(selector, opcode, leftOp, rightOp, cont);
+}
 
 void VisitWord32Compare(InstructionSelector* selector, Node* node,
                         FlagsContinuation* cont) {
-  VisitWordCompare(selector, node, kMips64Cmp, cont, false);
+  // MIPS64 doesn't support Word32 compare instructions. Instead it relies
+  // that the values in registers are correctly sign-extended and uses
+  // Word64 comparison instead. This behavior is correct in most cases,
+  // but doesn't work when comparing signed with unsigned operands.
+  // We could simulate Word32 compare in all cases but this would create
+  // an unnecessary overhead since unsigned integers are rarely used
+  // in JavaScript.
+  // The solution proposed here tries to match a comparison of signed
+  // with unsigned operand, and perform Word32Compare simulation only
+  // in those cases. Unfortunately, the solution is not complete because
+  // it might skip cases where Word32 compare simulation is needed, so
+  // basically it is a hack.
+  if (IsNodeUnsigned(node->InputAt(0)) != IsNodeUnsigned(node->InputAt(1))) {
+    VisitSimulateWord32Compare(selector, node, kMips64Cmp, cont, false);
+  } else {
+    VisitWordCompare(selector, node, kMips64Cmp, cont, false);
+  }
 }