Defer the prefix/postfix code generation.

src/arm/codegen-arm.cc

 // Copyright 2010 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 6030 matching lines @@
       default:
         UNREACHABLE();
     }
   }
   ASSERT(!has_valid_frame() ||
          (has_cc() && frame_->height() == original_height) ||
          (!has_cc() && frame_->height() == original_height + 1));
 }
 
 
+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();
 #endif
   Comment cmnt(masm_, "[ CountOperation");
   VirtualFrame::RegisterAllocationScope scope(this);
 
   bool is_postfix = node->is_postfix();
   bool is_increment = node->op() == Token::INC;
 
@@ skipping 39 matching lines @@
       // than on entry).
       if (!placeholder_pushed) frame_->EmitPush(Operand(Smi::FromInt(0)));
       ASSERT_EQ(original_height + 1, frame_->height());
       return;
     }
 
     // This pushes 0, 1 or 2 words on the object to be used later when updating
     // 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.
     if (placeholder_pushed) {
       frame_->SetElementAt(value, target.size());
     } else if (is_postfix) {
       frame_->EmitPush(value);
       __ mov(VirtualFrame::scratch0(), value);
       value = VirtualFrame::scratch0();
     }
 
     // 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);
     } else {
       __ sub(value, value, Operand(Smi::FromInt(1)), SetCC);
     }
 
-    // If the increment/decrement didn't overflow, we're done.
-    exit.Branch(vc);
-
-    // Revert optimistic increment/decrement.
-    if (is_increment) {
-      __ sub(value, value, Operand(Smi::FromInt(1)));
-    } else {
-      __ add(value, value, Operand(Smi::FromInt(1)));
-    }
+    // If increment/decrement overflows, go to deferred code.
+    deferred->Branch(vs);
 
     // 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
     // it has a non-zero size.
     if (!is_const) target.SetValue(NOT_CONST_INIT, LIKELY_SMI);
   }
 
   // Postfix: Discard the new value and use the old.
   if (is_postfix) frame_->Pop();
   ASSERT_EQ(original_height + 1, frame_->height());
@@ skipping 1119 matching lines @@
                BinaryOpIC::GetName(runtime_operands_type_));
   return name_;
 }
 
 
 #undef __
 
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_ARM