Re-enable using push instructions for syncing the virtual frame.

This change fixes the problem with the original version of this approach
(r3032) that may lead to a corrupted stack if we would invoke spilling during
syncing a large SMI constant (unsafe SMIs) in the virtual frame.

The new code for storing unsafe SMI constants does not use an extra temporary
register. This prevents the compiler from ever having to spill during a
virutal frame sync operation.

For storing a large SMI constant we previously generated:

  mov ecx, (large_smi & 0x0000ffff)
  xor ecx, (large_smi & 0xffff0000)
  push ecx

we now generate:

  push (large_smi & 0x0000ffff)
  or   [esp], (large_smi & 0xffff0000)

Not using a temporary register avoids spilling within an nvocation
of VirtualFrame::SyncRange.

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

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

diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc
index 04fec43..ab2aa63 100644 (file)
--- a/src/ia32/codegen-ia32.cc
+++ b/src/ia32/codegen-ia32.cc
@@ -3939,12 +3939,28 @@ void CodeGenerator::VisitLiteral(Literal* node) {
 }
 
 
-void CodeGenerator::LoadUnsafeSmi(Register target, Handle<Object> value) {
+void CodeGenerator::PushUnsafeSmi(Handle<Object> value) {
+  ASSERT(value->IsSmi());
+  int bits = reinterpret_cast<int>(*value);
+  __ push(Immediate(bits & 0x0000FFFF));
+  __ or_(Operand(esp, 0), Immediate(bits & 0xFFFF0000));
+}
+
+
+void CodeGenerator::StoreUnsafeSmiToLocal(int offset, Handle<Object> value) {
+  ASSERT(value->IsSmi());
+  int bits = reinterpret_cast<int>(*value);
+  __ mov(Operand(ebp, offset), Immediate(bits & 0x0000FFFF));
+  __ or_(Operand(ebp, offset), Immediate(bits & 0xFFFF0000));
+}
+
+
+void CodeGenerator::MoveUnsafeSmi(Register target, Handle<Object> value) {
   ASSERT(target.is_valid());
   ASSERT(value->IsSmi());
   int bits = reinterpret_cast<int>(*value);
   __ Set(target, Immediate(bits & 0x0000FFFF));
-  __ xor_(target, bits & 0xFFFF0000);
+  __ or_(target, bits & 0xFFFF0000);
 }
 
 

diff --git a/src/ia32/codegen-ia32.h b/src/ia32/codegen-ia32.h
index 8cd61c5..0e69a63 100644 (file)
--- a/src/ia32/codegen-ia32.h
+++ b/src/ia32/codegen-ia32.h
@@ -468,9 +468,11 @@ class CodeGenerator: public AstVisitor {
   // than 16 bits.
   static const int kMaxSmiInlinedBits = 16;
   bool IsUnsafeSmi(Handle<Object> value);
-  // Load an integer constant x into a register target using
+  // Load an integer constant x into a register target or into the stack using
   // at most 16 bits of user-controlled data per assembly operation.
-  void LoadUnsafeSmi(Register target, Handle<Object> value);
+  void MoveUnsafeSmi(Register target, Handle<Object> value);
+  void StoreUnsafeSmiToLocal(int offset, Handle<Object> value);
+  void PushUnsafeSmi(Handle<Object> value);
 
   void CallWithArguments(ZoneList<Expression*>* arguments, int position);
 

diff --git a/src/ia32/register-allocator-ia32.cc b/src/ia32/register-allocator-ia32.cc
index 2914960..0bad87d 100644 (file)
--- a/src/ia32/register-allocator-ia32.cc
+++ b/src/ia32/register-allocator-ia32.cc
@@ -42,7 +42,7 @@ void Result::ToRegister() {
     Result fresh = CodeGeneratorScope::Current()->allocator()->Allocate();
     ASSERT(fresh.is_valid());
     if (CodeGeneratorScope::Current()->IsUnsafeSmi(handle())) {
-      CodeGeneratorScope::Current()->LoadUnsafeSmi(fresh.reg(), handle());
+      CodeGeneratorScope::Current()->MoveUnsafeSmi(fresh.reg(), handle());
     } else {
       CodeGeneratorScope::Current()->masm()->Set(fresh.reg(),
                                                  Immediate(handle()));
@@ -64,7 +64,7 @@ void Result::ToRegister(Register target) {
     } else {
       ASSERT(is_constant());
       if (CodeGeneratorScope::Current()->IsUnsafeSmi(handle())) {
-        CodeGeneratorScope::Current()->LoadUnsafeSmi(fresh.reg(), handle());
+        CodeGeneratorScope::Current()->MoveUnsafeSmi(fresh.reg(), handle());
       } else {
         CodeGeneratorScope::Current()->masm()->Set(fresh.reg(),
                                                    Immediate(handle()));

diff --git a/src/ia32/virtual-frame-ia32.cc b/src/ia32/virtual-frame-ia32.cc
index f971ec4..e770cdd 100644 (file)
--- a/src/ia32/virtual-frame-ia32.cc
+++ b/src/ia32/virtual-frame-ia32.cc
@@ -75,10 +75,7 @@ void VirtualFrame::SyncElementBelowStackPointer(int index) {
 
     case FrameElement::CONSTANT:
       if (cgen()->IsUnsafeSmi(element.handle())) {
-        Result temp = cgen()->allocator()->Allocate();
-        ASSERT(temp.is_valid());
-        cgen()->LoadUnsafeSmi(temp.reg(), element.handle());
-        __ mov(Operand(ebp, fp_relative(index)), temp.reg());
+        cgen()->StoreUnsafeSmiToLocal(fp_relative(index), element.handle());
       } else {
         __ Set(Operand(ebp, fp_relative(index)),
                Immediate(element.handle()));
@@ -127,10 +124,7 @@ void VirtualFrame::SyncElementByPushing(int index) {
 
     case FrameElement::CONSTANT:
       if (cgen()->IsUnsafeSmi(element.handle())) {
-        Result temp = cgen()->allocator()->Allocate();
-        ASSERT(temp.is_valid());
-        cgen()->LoadUnsafeSmi(temp.reg(), element.handle());
-        __ push(temp.reg());
+       cgen()->PushUnsafeSmi(element.handle());
       } else {
         __ push(Immediate(element.handle()));
       }
@@ -161,15 +155,16 @@ void VirtualFrame::SyncRange(int begin, int end) {
   // on the stack.
   int start = Min(begin, stack_pointer_ + 1);
 
-  // If positive we have to adjust the stack pointer.
-  int delta = end - stack_pointer_;
-  if (delta > 0) {
-    stack_pointer_ = end;
-    __ sub(Operand(esp), Immediate(delta * kPointerSize));
-  }
-
+  // Emit normal push instructions for elements above stack pointer
+  // and use mov instructions if we are below stack pointer.
   for (int i = start; i <= end; i++) {
-    if (!elements_[i].is_synced()) SyncElementBelowStackPointer(i);
+    if (!elements_[i].is_synced()) {
+      if (i <= stack_pointer_) {
+        SyncElementBelowStackPointer(i);
+      } else {
+        SyncElementByPushing(i);
+      }
+    }
   }
 }
 
@@ -198,7 +193,7 @@ void VirtualFrame::MakeMergable() {
         // Emit a move.
         if (element.is_constant()) {
           if (cgen()->IsUnsafeSmi(element.handle())) {
-            cgen()->LoadUnsafeSmi(fresh.reg(), element.handle());
+            cgen()->MoveUnsafeSmi(fresh.reg(), element.handle());
           } else {
             __ Set(fresh.reg(), Immediate(element.handle()));
           }
@@ -299,7 +294,7 @@ void VirtualFrame::MergeMoveRegistersToMemory(VirtualFrame* expected) {
           if (!source.is_synced()) {
             if (cgen()->IsUnsafeSmi(source.handle())) {
               esi_caches = i;
-              cgen()->LoadUnsafeSmi(esi, source.handle());
+              cgen()->MoveUnsafeSmi(esi, source.handle());
               __ mov(Operand(ebp, fp_relative(i)), esi);
             } else {
               __ Set(Operand(ebp, fp_relative(i)), Immediate(source.handle()));
@@ -407,7 +402,7 @@ void VirtualFrame::MergeMoveMemoryToRegisters(VirtualFrame* expected) {
 
         case FrameElement::CONSTANT:
           if (cgen()->IsUnsafeSmi(source.handle())) {
-            cgen()->LoadUnsafeSmi(target_reg, source.handle());
+            cgen()->MoveUnsafeSmi(target_reg, source.handle());
           } else {
            __ Set(target_reg, Immediate(source.handle()));
           }