-Inlined double variant of compare iff one of the sides is a constant smi and...

src/ia32/codegen-ia32.cc

Index: src/ia32/codegen-ia32.cc
===================================================================
--- src/ia32/codegen-ia32.cc	(revision 3485)
+++ src/ia32/codegen-ia32.cc	(working copy)
@@ -1791,7 +1791,8 @@
 }
 
 
-void CodeGenerator::Comparison(Condition cc,
+void CodeGenerator::Comparison(AstNode* node,
+                               Condition cc,
                                bool strict,
                                ControlDestination* dest) {
   // Strict only makes sense for equality comparisons.
@@ -1838,7 +1839,8 @@
         default:
           UNREACHABLE();
       }
-    } else {  // Only one side is a constant Smi.
+    } else {
+      // Only one side is a constant Smi.
       // If left side is a constant Smi, reverse the operands.
       // Since one side is a constant Smi, conversion order does not matter.
       if (left_side_constant_smi) {
@@ -1852,6 +1854,8 @@
       // Implement comparison against a constant Smi, inlining the case
       // where both sides are Smis.
       left_side.ToRegister();
+      Register left_reg = left_side.reg();
+      Handle<Object> right_val = right_side.handle();
 
       // Here we split control flow to the stub call and inlined cases
       // before finally splitting it to the control destination.  We use
@@ -1859,11 +1863,49 @@
       // the first split.  We manually handle the off-frame references
       // by reconstituting them on the non-fall-through path.
       JumpTarget is_smi;
-      Register left_reg = left_side.reg();
-      Handle<Object> right_val = right_side.handle();
       __ test(left_side.reg(), Immediate(kSmiTagMask));
       is_smi.Branch(zero, taken);
 
+      bool is_for_loop_compare = node->AsCompareOperation()

[iposva, 2009/12/17 21:03:50]

Shouldn't you do a " != NULL" here?

[bakster, 2009/12/18 06:38:47]

Done.

+          && node->AsCompareOperation()->is_for_loop_condition();
+      if (!is_for_loop_compare
+          && CpuFeatures::IsSupported(SSE2)
+          && right_val->IsSmi()) {
+        // The right side value is either a smi or heap number.

[iposva, 2009/12/17 21:03:50]

Comment does not match the check. Right side is a smi.

[bakster, 2009/12/18 06:38:47]

Done.

+        CpuFeatures::Scope use_sse2(SSE2);
+        JumpTarget not_number;
+        __ cmp(FieldOperand(left_reg, HeapObject::kMapOffset),
+               Immediate(Factory::heap_number_map()));
+        not_number.Branch(not_equal, &left_side);
+        __ movdbl(xmm1,
+                  FieldOperand(left_reg, HeapNumber::kValueOffset));
+        int value = Smi::cast(*right_val)->value();
+        if (value == 0) {
+          __ xorpd(xmm0, xmm0);
+        } else {
+          Result temp = allocator()->Allocate();
+          __ mov (temp.reg(), Immediate(value));
+          __ cvtsi2sd(xmm0, Operand(temp.reg()));

[iposva, 2009/12/17 21:03:50]

Why do you keep converting the value to double from int on every check? You
could just create the double immediate and load it directly.

[bakster, 2009/12/18 06:38:47]

I'm not sure it makes a difference. I'll try the optimization after the commit.

On 2009/12/17 21:03:50, iposva wrote:

+          temp.Unuse();
+        }
+        __ comisd(xmm1, xmm0);
+        // Jump to builtin for NaN.
+        not_number.Branch(parity_even, &left_side);
+        left_side.Unuse();
+        Condition double_cc = cc;
+        switch (cc) {
+          case less:          double_cc = below;       break;
+          case equal:         double_cc = equal;       break;
+          case less_equal:    double_cc = below_equal; break;
+          case greater:       double_cc = above;       break;
+          case greater_equal: double_cc = above_equal; break;
+          default: UNREACHABLE();
+        }
+        dest->true_target()->Branch(double_cc);
+        dest->false_target()->Jump();
+        not_number.Bind(&left_side);
+      }
+
       // Setup and call the compare stub.
       CompareStub stub(cc, strict);
       Result result = frame_->CallStub(&stub, &left_side, &right_side);
@@ -1887,6 +1929,7 @@
       right_side.Unuse();
       dest->Split(cc);
     }
+
   } else if (cc == equal &&
              (left_side_constant_null || right_side_constant_null)) {
     // To make null checks efficient, we check if either the left side or
@@ -1923,7 +1966,8 @@
       operand.Unuse();
       dest->Split(not_zero);
     }
-  } else {  // Neither side is a constant Smi or null.
+  } else {
+    // Neither side is a constant Smi or null.
     // If either side is a non-smi constant, skip the smi check.
     bool known_non_smi =
         (left_side.is_constant() && !left_side.handle()->IsSmi()) ||
@@ -2590,7 +2634,7 @@
     // Compare and branch to the body if true or the next test if
     // false.  Prefer the next test as a fall through.
     ControlDestination dest(clause->body_target(), &next_test, false);
-    Comparison(equal, true, &dest);
+    Comparison(node, equal, true, &dest);
 
     // If the comparison fell through to the true target, jump to the
     // actual body.
@@ -6003,7 +6047,7 @@
   }
   Load(left);
   Load(right);
-  Comparison(cc, strict, destination());
+  Comparison(node, cc, strict, destination());
 }
 
 
@@ -6802,7 +6846,7 @@
     __ push(left);
     __ push(Immediate(right));
   } else {
-    // The calling convention with registers is left in edx and right in eax.
+     // The calling convention with registers is left in edx and right in eax.

[iposva, 2009/12/17 21:03:50]

Unmotivated space?

[bakster, 2009/12/18 06:38:47]

Done.

     Register left_arg = edx;
     Register right_arg = eax;
     if (left.is(left_arg)) {