Defer the prefix/postfix code generation.

src/arm/codegen-arm.cc

Index: src/arm/codegen-arm.cc
===================================================================
--- src/arm/codegen-arm.cc	(revision 5606)
+++ src/arm/codegen-arm.cc	(working copy)
@@ -6048,6 +6048,74 @@
 }
 
 
+class DeferredCountOperation: public DeferredCode {
+ public:
+  DeferredCountOperation(Register value,
+                         bool is_increment,
+                         bool is_postfix,
+                         int target_size)
+      : value_(value),
+        is_increment_(is_increment),
+        is_postfix_(is_postfix),
+        target_size_(target_size) {}
+
+  virtual void Generate() {
+    VirtualFrame copied_frame(*frame_state()->frame());
+    copied_frame.SpillAll();

[Erik Corry, 2010/11/05 22:04:36]

Actually I think you don't need this, though the builtin and runtime calls will
spill all registers regardless.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

Done.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

Done.

+
+    Label slow;
+    // Check for smi operand.
+    __ tst(value_, Operand(kSmiTagMask));
+    __ b(ne, &slow);
+
+    // Revert optimistic increment/decrement.
+    if (is_increment_) {
+      __ sub(value_, value_, Operand(Smi::FromInt(1)), LeaveCC, vs);
+    } else {
+      __ add(value_, value_, Operand(Smi::FromInt(1)), LeaveCC, vs);
+    }
+
+    // Slow case: Convert to number.  At this point the
+    // value to be incremented is in the value register..
+    __ bind(&slow);
+
+    // Convert the operand to a number.
+    __ push(value_);
+    copied_frame.RaiseHeight(1, 0);

[Erik Corry, 2010/11/05 22:04:36]

Instead of doing a "__ push" followed by RaiseHeight I think it would be cleaner
to just use __ EmitPush(), which combines both.  The InvokeBuiltin or
CallRuntime call will ensure that everything gets flushed to the stack.  This
also applies 2 places below.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

Done.

+
+    copied_frame.InvokeBuiltin(Builtins::TO_NUMBER, CALL_JS, 1);
+
+    if (is_postfix_) {
+      // Postfix: store to result (on the stack).
+      __ str(r0,  MemOperand(sp, target_size_ * kPointerSize));
+    }
+
+    // Compute the new value.
+    __ push(r0);
+    copied_frame.RaiseHeight(1, 0);
+    __ mov(r0, Operand(Smi::FromInt(1)));

[Erik Corry, 2010/11/05 22:04:36]

If you use EmitPush you will have to get a new register here by using
GetTOSRegister.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

Done.

+    __ push(r0);
+    copied_frame.RaiseHeight(1, 0);
+    if (is_increment_) {
+      copied_frame.CallRuntime(Runtime::kNumberAdd, 2);
+    } else {
+      copied_frame.CallRuntime(Runtime::kNumberSub, 2);
+    }
+
+    if (!value_.is(r0))
+      __ mov(value_, r0);

[Erik Corry, 2010/11/05 22:04:36]

We have __ Move(value_, r0) for this purpose.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

Done.

+
+    copied_frame.MergeTo(frame_state()->frame());
+  }
+
+ private:
+  Register value_;
+  bool is_increment_;
+  bool is_postfix_;
+  int target_size_;
+};
+
+
 void CodeGenerator::VisitCountOperation(CountOperation* node) {
 #ifdef DEBUG
   int original_height = frame_->height();
@@ -6107,9 +6175,6 @@
     // the target.  It also pushes the current value of the target.
     target.GetValue();
 
-    JumpTarget slow;
-    JumpTarget exit;
-
     Register value = frame_->PopToRegister();
 
     // Postfix: Store the old value as the result.
@@ -6123,8 +6188,14 @@
 
     // Check for smi operand.
     __ tst(value, Operand(kSmiTagMask));
-    slow.Branch(ne);
 
+    DeferredCode* deferred =
+        new DeferredCountOperation(value,
+                                   is_increment,
+                                   is_postfix,
+                                   target.size());
+    deferred->Branch(ne);

[Erik Corry, 2010/11/05 22:04:36]

It looks like you still have a bug here.  The tst instruction will not affect
the overflow flag, so whether or not the increment or decrement at the start of
the Generate() method gets performed will be essentially random, depending on
what the state of the overflow flag was before the tst instruction.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

I agree that tst won't affect the overflow tag, so at the beginning of
Genrate(), I do the checking for smi operand again. It it is smi operand,
optimistic increment/decrement isn't reverted. The increment/decrement inversion
in Generate() doesn't depend on the overflow flag anymore, so I take the "vs"
from sub/add in Generate. 

+
     // Perform optimistic increment/decrement.
     if (is_increment) {
       __ add(value, value, Operand(Smi::FromInt(1)), SetCC);
@@ -6132,46 +6203,15 @@
       __ sub(value, value, Operand(Smi::FromInt(1)), SetCC);
     }
 
-    // If the increment/decrement didn't overflow, we're done.
-    exit.Branch(vc);
+    // If increment/decrement overflows, go to deferred code.
+    deferred->Branch(vs);
 
-    // Revert optimistic increment/decrement.
-    if (is_increment) {
-      __ sub(value, value, Operand(Smi::FromInt(1)));
-    } else {
-      __ add(value, value, Operand(Smi::FromInt(1)));
-    }
-
     // Slow case: Convert to number.  At this point the

[Erik Corry, 2010/11/05 22:04:36]

This comment should be moved to the deferred code, where it applies.

[zhangk, 2010/11/09 23:07:53]

On 2010/11/05 22:04:36, Erik Corry wrote:

Done.

     // value to be incremented is in the value register..
-    slow.Bind();
+    deferred->BindExit();
 
-    // Convert the operand to a number.
-    frame_->EmitPush(value);
-
-    {
-      VirtualFrame::SpilledScope spilled(frame_);
-      frame_->InvokeBuiltin(Builtins::TO_NUMBER, CALL_JS, 1);
-
-      if (is_postfix) {
-        // Postfix: store to result (on the stack).
-        __ str(r0, frame_->ElementAt(target.size()));
-      }
-
-      // Compute the new value.
-      frame_->EmitPush(r0);
-      frame_->EmitPush(Operand(Smi::FromInt(1)));
-      if (is_increment) {
-        frame_->CallRuntime(Runtime::kNumberAdd, 2);
-      } else {
-        frame_->CallRuntime(Runtime::kNumberSub, 2);
-      }
-    }
-
-    __ Move(value, r0);
     // Store the new value in the target if not const.
     // At this point the answer is in the value register.
-    exit.Bind();
     frame_->EmitPush(value);
     // Set the target with the result, leaving the result on
     // top of the stack.  Removes the target from the stack if