Rewrite of VisitCountOperation that should speed it up

Review URL: http://codereview.chromium.org/56151

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1677 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/builtins.h

@@ -105,8 +105,6 @@ namespace v8 { namespace internal {
   V(MUL, 1)                    \
   V(DIV, 1)                    \
   V(MOD, 1)                    \
-  V(INC, 0)                    \
-  V(DEC, 0)                    \
   V(BIT_OR, 1)                 \
   V(BIT_AND, 1)                \
   V(BIT_XOR, 1)                \

src/codegen-arm.cc

@@ -708,29 +708,6 @@ class GenericBinaryOpStub : public CodeStub {
 };
 
 
-class InvokeBuiltinStub : public CodeStub {
- public:
-  enum Kind { Inc, Dec, ToNumber };
-  InvokeBuiltinStub(Kind kind, int argc) : kind_(kind), argc_(argc) { }
-
- private:
-  Kind kind_;
-  int argc_;
-
-  Major MajorKey() { return InvokeBuiltin; }
-  int MinorKey() { return (argc_ << 3) | static_cast<int>(kind_); }
-  void Generate(MacroAssembler* masm);
-
-#ifdef DEBUG
-  void Print() {
-    PrintF("InvokeBuiltinStub (kind %d, argc, %d)\n",
-           static_cast<int>(kind_),
-           argc_);
-  }
-#endif
-};
-
-
 void CodeGenerator::GenericBinaryOperation(Token::Value op) {
   VirtualFrame::SpilledScope spilled_scope(this);
   // sp[0] : y
@@ -3696,22 +3673,27 @@ void CodeGenerator::VisitCountOperation(CountOperation* node) {
 
     // Slow case: Convert to number.
     slow.Bind();
-
-    // Postfix: Convert the operand to a number and store it as the result.
+    {
+      // Convert the operand to a number.
+      frame_->EmitPush(r0);
+      Result arg_count = allocator_->Allocate(r0);
+      ASSERT(arg_count.is_valid());
+      __ mov(arg_count.reg(), Operand(0));
+      frame_->InvokeBuiltin(Builtins::TO_NUMBER, CALL_JS, &arg_count, 1);
+    }
     if (is_postfix) {
-      InvokeBuiltinStub stub(InvokeBuiltinStub::ToNumber, 2);
-      frame_->CallStub(&stub, 0);
-      // Store to result (on the stack).
+      // Postfix: store to result (on the stack).
       __ str(r0, frame_->ElementAt(target.size()));
     }
 
-    // Compute the new value by calling the right JavaScript native.
+    // Compute the new value.
+    __ mov(r1, Operand(Smi::FromInt(1)));
+    frame_->EmitPush(r0);
+    frame_->EmitPush(r1);
     if (is_increment) {
-      InvokeBuiltinStub stub(InvokeBuiltinStub::Inc, 1);
-      frame_->CallStub(&stub, 0);
+      frame_->CallRuntime(Runtime::kNumberAdd, 2);
     } else {
-      InvokeBuiltinStub stub(InvokeBuiltinStub::Dec, 1);
-      frame_->CallStub(&stub, 0);
+      frame_->CallRuntime(Runtime::kNumberSub, 2);
     }
 
     // Store the new value in the target if not const.
@@ -4718,19 +4700,6 @@ void UnarySubStub::Generate(MacroAssembler* masm) {
 }
 
 
-void InvokeBuiltinStub::Generate(MacroAssembler* masm) {
-  __ push(r0);
-  __ mov(r0, Operand(0));  // set number of arguments
-  switch (kind_) {
-    case ToNumber: __ InvokeBuiltin(Builtins::TO_NUMBER, JUMP_JS); break;
-    case Inc:      __ InvokeBuiltin(Builtins::INC, JUMP_JS);       break;
-    case Dec:      __ InvokeBuiltin(Builtins::DEC, JUMP_JS);       break;
-    default: UNREACHABLE();
-  }
-  __ StubReturn(argc_);
-}
-
-
 void CEntryStub::GenerateThrowTOS(MacroAssembler* masm) {
   // r0 holds exception
   ASSERT(StackHandlerConstants::kSize == 6 * kPointerSize);  // adjust this code

src/codegen-ia32.cc

@@ -4724,11 +4724,11 @@ class DeferredCountOperation: public DeferredCode {
   DeferredCountOperation(CodeGenerator* generator,
                          bool is_postfix,
                          bool is_increment,
-                         int result_offset)
+                         int target_size)
       : DeferredCode(generator),
         is_postfix_(is_postfix),
         is_increment_(is_increment),
-        result_offset_(result_offset) {
+        target_size_(target_size) {
     set_comment("[ DeferredCountOperation");
   }
 
@@ -4737,75 +4737,38 @@ class DeferredCountOperation: public DeferredCode {
  private:
   bool is_postfix_;
   bool is_increment_;
-  int result_offset_;
-};
-
-
-class RevertToNumberStub: public CodeStub {
- public:
-  explicit RevertToNumberStub(bool is_increment)
-     :  is_increment_(is_increment) { }
-
- private:
-  bool is_increment_;
-
-  Major MajorKey() { return RevertToNumber; }
-  int MinorKey() { return is_increment_ ? 1 : 0; }
-  void Generate(MacroAssembler* masm);
-
-#ifdef DEBUG
-  void Print() {
-    PrintF("RevertToNumberStub (is_increment %s)\n",
-           is_increment_ ? "true" : "false");
-  }
-#endif
-};
-
-
-class CounterOpStub: public CodeStub {
- public:
-  CounterOpStub(int result_offset, bool is_postfix, bool is_increment)
-     :  result_offset_(result_offset),
-        is_postfix_(is_postfix),
-        is_increment_(is_increment) { }
-
- private:
-  int result_offset_;
-  bool is_postfix_;
-  bool is_increment_;
-
-  Major MajorKey() { return CounterOp; }
-  int MinorKey() {
-    return ((result_offset_ << 2) |
-            (is_postfix_ ? 2 : 0) |
-            (is_increment_ ? 1 : 0));
-  }
-  void Generate(MacroAssembler* masm);
-
-#ifdef DEBUG
-  void Print() {
-    PrintF("CounterOpStub (result_offset %d), (is_postfix %s),"
-           " (is_increment %s)\n",
-           result_offset_,
-           is_postfix_ ? "true" : "false",
-           is_increment_ ? "true" : "false");
-  }
-#endif
+  int target_size_;
 };
 
 
 void DeferredCountOperation::Generate() {
   CodeGenerator* cgen = generator();
-
   Result value(cgen);
   enter()->Bind(&value);
-  if (is_postfix_) {
-    RevertToNumberStub to_number_stub(is_increment_);
-    value = generator()->frame()->CallStub(&to_number_stub, &value);
+  VirtualFrame* frame = cgen->frame();
+  // Undo the optimistic smi operation.
+  value.ToRegister();
+  frame->Spill(value.reg());
+  if (is_increment_) {
+    __ sub(Operand(value.reg()), Immediate(Smi::FromInt(1)));
+  } else {
+    __ add(Operand(value.reg()), Immediate(Smi::FromInt(1)));
+  }
+  frame->Push(&value);
+  value = frame->InvokeBuiltin(Builtins::TO_NUMBER, CALL_FUNCTION, 1);
+  frame->Push(&value);
+  if (is_postfix_) {  // Fix up copy of old value with ToNumber(value).
+    // This is only safe because VisitCountOperation makes this frame slot
+    // beneath the reference a register, which is spilled at the above call.
+    // We cannot safely write to constants or copies below the water line.
+    frame->StoreToElementAt(target_size_ + 1);
+  }
+  frame->Push(Smi::FromInt(1));
+  if (is_increment_) {
+    value = frame->CallRuntime(Runtime::kNumberAdd, 2);
+  } else {
+    value = frame->CallRuntime(Runtime::kNumberSub, 2);
   }
-
-  CounterOpStub stub(result_offset_, is_postfix_, is_increment_);
-  value = generator()->frame()->CallStub(&stub, &value);
   exit_.Jump(&value);
 }
 
@@ -4819,7 +4782,8 @@ void CodeGenerator::VisitCountOperation(CountOperation* node) {
   Variable* var = node->expression()->AsVariableProxy()->AsVariable();
   bool is_const = (var != NULL && var->mode() == Variable::CONST);
 
-  // Postfix: Make room for the result.
+  // Postfix operators need a stack slot under the reference to hold
+  // the old value while the new one is being stored.
   if (is_postfix) {
     frame_->Push(Smi::FromInt(0));
   }
@@ -4836,16 +4800,21 @@ void CodeGenerator::VisitCountOperation(CountOperation* node) {
     target.TakeValue(NOT_INSIDE_TYPEOF);
 
     DeferredCountOperation* deferred =
-        new DeferredCountOperation(this, is_postfix, is_increment,
-                                   target.size() * kPointerSize);
+        new DeferredCountOperation(this, is_postfix,
+                                   is_increment, target.size());
 
     Result value = frame_->Pop();
     value.ToRegister();
-    ASSERT(value.is_valid());
 
     // Postfix: Store the old value as the result.
     if (is_postfix) {
-      Result old_value = value;
+      // Explicitly back the slot for the old value with a new register.
+      // This improves performance in some cases.
+      Result old_value = allocator_->Allocate();
+      ASSERT(old_value.is_valid());
+      __ mov(old_value.reg(), value.reg());
+      // SetElement must not create a constant element or a copy in this slot,
+      // since we will write to it, below the waterline, in deferred code.
       frame_->SetElementAt(target.size(), &old_value);
     }
 
@@ -4885,7 +4854,7 @@ void CodeGenerator::VisitCountOperation(CountOperation* node) {
       tmp.Unuse();
       __ test(value.reg(), Immediate(kSmiTagMask));
       deferred->enter()->Branch(not_zero, &value, not_taken);
-    } else {
+    } else {  // Otherwise we test separately for overflow and smi check.
       deferred->enter()->Branch(overflow, &value, not_taken);
       __ test(value.reg(), Immediate(kSmiTagMask));
       deferred->enter()->Branch(not_zero, &value, not_taken);
@@ -6777,49 +6746,6 @@ void CallFunctionStub::Generate(MacroAssembler* masm) {
 }
 
 
-void RevertToNumberStub::Generate(MacroAssembler* masm) {
-  // Revert optimistic increment/decrement.
-  if (is_increment_) {
-    __ sub(Operand(eax), Immediate(Smi::FromInt(1)));
-  } else {
-    __ add(Operand(eax), Immediate(Smi::FromInt(1)));
-  }
-
-  __ pop(ecx);
-  __ push(eax);
-  __ push(ecx);
-  __ InvokeBuiltin(Builtins::TO_NUMBER, JUMP_FUNCTION);
-  // Code never returns due to JUMP_FUNCTION.
-}
-
-
-void CounterOpStub::Generate(MacroAssembler* masm) {
-  // Store to the result on the stack (skip return address) before
-  // performing the count operation.
-  if (is_postfix_) {
-    __ mov(Operand(esp, result_offset_ + kPointerSize), eax);
-  }
-
-  // Revert optimistic increment/decrement but only for prefix
-  // counts. For postfix counts it has already been reverted before
-  // the conversion to numbers.
-  if (!is_postfix_) {
-    if (is_increment_) {
-      __ sub(Operand(eax), Immediate(Smi::FromInt(1)));
-    } else {
-      __ add(Operand(eax), Immediate(Smi::FromInt(1)));
-    }
-  }
-
-  // Compute the new value by calling the right JavaScript native.
-  __ pop(ecx);
-  __ push(eax);
-  __ push(ecx);
-  Builtins::JavaScript builtin = is_increment_ ? Builtins::INC : Builtins::DEC;
-  __ InvokeBuiltin(builtin, JUMP_FUNCTION);
-  // Code never returns due to JUMP_FUNCTION.
-}
-
 
 void CEntryStub::GenerateThrowTOS(MacroAssembler* masm) {
   ASSERT(StackHandlerConstants::kSize == 6 * kPointerSize);  // adjust this code

src/runtime.js

@@ -197,18 +197,6 @@ function MOD(x) {
 }
 
 
-// ECMA-262, section 11.4.4, page 47.
-function INC() {
-  return %NumberAdd(%ToNumber(this), 1);
-}
-
-
-// ECMA-262, section 11.4.5, page 48.
-function DEC() {
-  return %NumberSub(%ToNumber(this), 1);
-}
-
-
 
 /* -------------------------------------------
    - - -   B i t   o p e r a t i o n s   - - -

src/virtual-frame-ia32.h

@@ -172,6 +172,10 @@ class VirtualFrame : public Malloced {
     PushFrameSlotAt(elements_.length() - index - 1);
   }
 
+  void StoreToElementAt(int index) {
+    StoreToFrameSlotAt(elements_.length() - index - 1);
+  }
+
   // A frame-allocated local as an assembly operand.
   Operand LocalAt(int index) const {
     ASSERT(0 <= index);

test/mjsunit/compare-constants.js → test/mjsunit/constant-folding.js

@@ -25,6 +25,57 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+// Test operations that involve one or more constants.
+// The code generator now handles compile-time constants specially.
+// Test the code generated when operands are known at compile time
+
+// Test count operations involving constants
+function test_count() {
+  var x = "foo";
+  var y = "3";
+
+  x += x++;  // ++ and -- apply ToNumber to their operand, even for postfix.
+  assertEquals(x, "fooNaN", "fooNaN test");
+  x = "luft";
+  x += ++x;
+  assertEquals(x, "luftNaN", "luftNaN test");
+
+  assertTrue(y++ === 3, "y++ === 3, where y = \"3\"");
+  y = 3;
+  assertEquals(y++, 3, "y++ == 3, where y = 3");
+  y = "7.1";
+  assertTrue(y++ === 7.1, "y++ === 7.1, where y = \"7.1\"");
+  var z = y = x = "9";
+  assertEquals( z++ + (++y) + x++, 28, "z++ + (++y) + x++ == 28");
+  z = y = x = 13;
+  assertEquals( z++ + (++y) + x++, 40, "z++ + (++y) + x++ == 40");
+  z = y = x = -5.5;
+  assertEquals( z++ + (++y) + x++, -15.5, "z++ + (++y) + x++ == -15.5");
+
+  assertEquals(y, -4.5);
+  z = y;
+  z++;
+  assertEquals(y, -4.5);
+  z = y;
+  y++;
+  assertEquals(z, -4.5);
+
+  y = 20;
+  z = y;
+  z++;
+  assertEquals(y, 20);
+  z = y;
+  y++;
+  assertEquals(z, 20);
+
+  const w = 30;
+  assertEquals(w++, 30);
+  assertEquals(++w, 31);
+  assertEquals(++w, 31);
+}
+
+test_count();
+
 // Test comparison operations that involve one or two constant smis.
 
 function test() {
@@ -118,4 +169,3 @@ function test() {
 }
 
 test();
-