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

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

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/base/adapters.h"
 #include "src/base/bits.h"
 #include "src/compiler/instruction-selector-impl.h"
 #include "src/compiler/node-matchers.h"
 #include "src/compiler/node-properties.h"
 
@@ skipping 1822 matching lines @@
       default:
         VisitCompare(selector, opcode, g.UseRegister(right),
                      g.UseRegister(left), cont);
     }
   } else {
     VisitCompare(selector, opcode, g.UseRegister(left), g.UseRegister(right),
                  cont);
   }
 }
 
+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) {
+  // 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);
+  }
+}
+
 
-void VisitWord32Compare(InstructionSelector* selector, Node* node,
-                        FlagsContinuation* cont) {
-  VisitWordCompare(selector, node, kMips64Cmp, cont, false);
-}
-
-
 void VisitWord64Compare(InstructionSelector* selector, Node* node,
                         FlagsContinuation* cont) {
   VisitWordCompare(selector, node, kMips64Cmp, cont, false);
 }
 
 
 
 void EmitWordCompareZero(InstructionSelector* selector, Node* value,
                          FlagsContinuation* cont) {
   Mips64OperandGenerator g(selector);
@@ skipping 471 matching lines @@
   } else {
     DCHECK(kArchVariant == kMips64r2);
     return MachineOperatorBuilder::AlignmentRequirements::
         NoUnalignedAccessSupport();
   }
 }
 
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8