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

src/ia32/codegen-ia32.cc

 // Copyright 2006-2009 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1773 matching lines @@
         LikelySmiBinaryOperation(op, operand, &constant_operand,
                                  overwrite_mode);
       }
       break;
     }
   }
   ASSERT(!operand->is_valid());
 }
 
 
-void CodeGenerator::Comparison(Condition cc,
+void CodeGenerator::Comparison(AstNode* node,
+                               Condition cc,
                                bool strict,
                                ControlDestination* dest) {
   // Strict only makes sense for equality comparisons.
   ASSERT(!strict || cc == equal);
 
   Result left_side;
   Result right_side;
   // Implement '>' and '<=' by reversal to obtain ECMA-262 conversion order.
   if (cc == greater || cc == less_equal) {
     cc = ReverseCondition(cc);
@@ skipping 26 matching lines @@
           break;
         case equal:
           dest->Goto(left_value == right_value);
           break;
         case greater_equal:
           dest->Goto(left_value >= right_value);
           break;
         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) {
         Result temp = left_side;
         left_side = right_side;
         right_side = temp;
         cc = ReverseCondition(cc);
         // This may reintroduce greater or less_equal as the value of cc.
         // CompareStub and the inline code both support all values of cc.
       }
       // 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
       // a jump target and branching to duplicate the virtual frame at
       // 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);
       result.ToRegister();
       __ cmp(result.reg(), 0);
       result.Unuse();
       dest->true_target()->Branch(cc);
       dest->false_target()->Jump();
 
       is_smi.Bind();
       left_side = Result(left_reg);
       right_side = Result(right_val);
       // Test smi equality and comparison by signed int comparison.
       if (IsUnsafeSmi(right_side.handle())) {
         right_side.ToRegister();
         __ cmp(left_side.reg(), Operand(right_side.reg()));
       } else {
         __ cmp(Operand(left_side.reg()), Immediate(right_side.handle()));
       }
       left_side.Unuse();
       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
     // the right side is the constant 'null'.
     // If so, we optimize the code by inlining a null check instead of
     // calling the (very) general runtime routine for checking equality.
     Result operand = left_side_constant_null ? right_side : left_side;
     right_side.Unuse();
     left_side.Unuse();
     operand.ToRegister();
@@ skipping 16 matching lines @@
       ASSERT(temp.is_valid());
       __ mov(temp.reg(),
              FieldOperand(operand.reg(), HeapObject::kMapOffset));
       __ movzx_b(temp.reg(),
                  FieldOperand(temp.reg(), Map::kBitFieldOffset));
       __ test(temp.reg(), Immediate(1 << Map::kIsUndetectable));
       temp.Unuse();
       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()) ||
         (right_side.is_constant() && !right_side.handle()->IsSmi());
     left_side.ToRegister();
     right_side.ToRegister();
 
     if (known_non_smi) {
       // When non-smi, call out to the compare stub.
       CompareStub stub(cc, strict);
@@ skipping 646 matching lines @@
 
     // Duplicate the switch value.
     frame_->Dup();
 
     // Compile the label expression.
     Load(clause->label());
 
     // 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.
     if (dest.true_was_fall_through()) {
       clause->body_target()->Unuse();
       clause->body_target()->Jump();
     }
   }
 
   // If there was control flow to a next test from the last one
@@ skipping 3392 matching lines @@
       __ test(answer.reg(), Operand(answer.reg()));
       answer.Unuse();
       destination()->Split(zero);
       return;
     }
     default:
       UNREACHABLE();
   }
   Load(left);
   Load(right);
-  Comparison(cc, strict, destination());
+  Comparison(node, cc, strict, destination());
 }
 
 
 #ifdef DEBUG
 bool CodeGenerator::HasValidEntryRegisters() {
   return (allocator()->count(eax) == (frame()->is_used(eax) ? 1 : 0))
       && (allocator()->count(ebx) == (frame()->is_used(ebx) ? 1 : 0))
       && (allocator()->count(ecx) == (frame()->is_used(ecx) ? 1 : 0))
       && (allocator()->count(edx) == (frame()->is_used(edx) ? 1 : 0))
       && (allocator()->count(edi) == (frame()->is_used(edi) ? 1 : 0));
@@ skipping 778 matching lines @@
 
 void GenericBinaryOpStub::GenerateCall(
     MacroAssembler* masm,
     Register left,
     Smi* right) {
   if (!ArgsInRegistersSupported()) {
     // Pass arguments on the stack.
     __ 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)) {
       __ mov(right_arg, Immediate(right));
     } else if (left.is(right_arg) && IsOperationCommutative()) {
       __ mov(left_arg, Immediate(right));
       SetArgsReversed();
     } else {
       __ mov(left_arg, left);
       __ mov(right_arg, Immediate(right));
@@ skipping 1862 matching lines @@
     __ add(Operand(dest), Immediate(2));
   }
   __ sub(Operand(count), Immediate(1));
   __ j(not_zero, &loop);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal