From 53bee811add90f5e146239edaada25c096a5d8e9 Mon Sep 17 00:00:00 2001 From: "fschneider@chromium.org" Date: Mon, 16 Nov 2009 23:11:19 +0000 Subject: [PATCH] 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 --- src/ia32/codegen-ia32.cc | 20 +++++++++++++++-- src/ia32/codegen-ia32.h | 6 ++++-- src/ia32/register-allocator-ia32.cc | 4 ++-- src/ia32/virtual-frame-ia32.cc | 33 ++++++++++++----------------- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc index 04fec43e55..ab2aa6379b 100644 --- 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 value) { +void CodeGenerator::PushUnsafeSmi(Handle value) { + ASSERT(value->IsSmi()); + int bits = reinterpret_cast(*value); + __ push(Immediate(bits & 0x0000FFFF)); + __ or_(Operand(esp, 0), Immediate(bits & 0xFFFF0000)); +} + + +void CodeGenerator::StoreUnsafeSmiToLocal(int offset, Handle value) { + ASSERT(value->IsSmi()); + int bits = reinterpret_cast(*value); + __ mov(Operand(ebp, offset), Immediate(bits & 0x0000FFFF)); + __ or_(Operand(ebp, offset), Immediate(bits & 0xFFFF0000)); +} + + +void CodeGenerator::MoveUnsafeSmi(Register target, Handle value) { ASSERT(target.is_valid()); ASSERT(value->IsSmi()); int bits = reinterpret_cast(*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 8cd61c5a97..0e69a63d89 100644 --- 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 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 value); + void MoveUnsafeSmi(Register target, Handle value); + void StoreUnsafeSmiToLocal(int offset, Handle value); + void PushUnsafeSmi(Handle value); void CallWithArguments(ZoneList* arguments, int position); diff --git a/src/ia32/register-allocator-ia32.cc b/src/ia32/register-allocator-ia32.cc index 2914960eac..0bad87d082 100644 --- 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 f971ec48e5..e770cddb15 100644 --- 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())); }