From 32cd13ebf10589b1110842cf92fec213c56bccdb Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Fri, 4 Feb 2011 12:06:41 +0000 Subject: [PATCH] Remove the redundant load on every context lookup. There was an unnecessary load on every statically-resolved context lookup. Remove it. This revealed a hidden bug in const initializers inside 'with'. They claim to be statically resolved (having slot type CONTEXT) but they occur in a spot where the runtime context chain and the static scope chain do not agree. This is fixed by special casing const initializers in the backend. Review URL: http://codereview.chromium.org/6384020 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6635 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/ia32/full-codegen-ia32.cc | 84 +++++++++++++++++++++++++--------------- src/ia32/lithium-codegen-ia32.cc | 8 +--- src/ia32/lithium-ia32.cc | 4 +- src/ia32/macro-assembler-ia32.cc | 20 +++++++--- src/prettyprinter.cc | 25 ++---------- 5 files changed, 77 insertions(+), 64 deletions(-) diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 3ef8159..e6f7416 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -610,7 +610,7 @@ void FullCodeGenerator::Move(Slot* dst, __ mov(location, src); // Emit the write barrier code if the location is in the heap. if (dst->type() == Slot::CONTEXT) { - int offset = FixedArray::kHeaderSize + dst->index() * kPointerSize; + int offset = Context::SlotOffset(dst->index()); __ RecordWrite(scratch1, offset, src, scratch2); } } @@ -666,10 +666,11 @@ void FullCodeGenerator::EmitDeclaration(Variable* variable, // We bypass the general EmitSlotSearch because we know more about // this specific context. - // The variable in the decl always resides in the current context. + // The variable in the decl always resides in the current function + // context. ASSERT_EQ(0, scope()->ContextChainLength(variable->scope())); if (FLAG_debug_code) { - // Check if we have the correct context pointer. + // Check that we're not inside a 'with'. __ mov(ebx, ContextOperand(esi, Context::FCONTEXT_INDEX)); __ cmp(ebx, Operand(esi)); __ Check(equal, "Unexpected declaration in current context."); @@ -1124,8 +1125,11 @@ MemOperand FullCodeGenerator::ContextSlotOperandCheckExtensions( // Check that last extension is NULL. __ cmp(ContextOperand(context, Context::EXTENSION_INDEX), Immediate(0)); __ j(not_equal, slow); - __ mov(temp, ContextOperand(context, Context::FCONTEXT_INDEX)); - return ContextOperand(temp, slot->index()); + + // This function is used only for loads, not stores, so it's safe to + // return an esi-based operand (the write barrier cannot be allowed to + // destroy the esi register). + return ContextOperand(context, slot->index()); } @@ -2000,57 +2004,75 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, Handle ic(Builtins::builtin(Builtins::StoreIC_Initialize)); EmitCallIC(ic, RelocInfo::CODE_TARGET); - } else if (var->mode() != Variable::CONST || op == Token::INIT_CONST) { - // Perform the assignment for non-const variables and for initialization - // of const variables. Const assignments are simply skipped. - Label done; + } else if (op == Token::INIT_CONST) { + // Like var declarations, const declarations are hoisted to function + // scope. However, unlike var initializers, const initializers are able + // to drill a hole to that function context, even from inside a 'with' + // context. We thus bypass the normal static scope lookup. + Slot* slot = var->AsSlot(); + Label skip; + switch (slot->type()) { + case Slot::PARAMETER: + // No const parameters. + UNREACHABLE(); + break; + case Slot::LOCAL: + __ mov(edx, Operand(ebp, SlotOffset(slot))); + __ cmp(edx, Factory::the_hole_value()); + __ j(not_equal, &skip); + __ mov(Operand(ebp, SlotOffset(slot)), eax); + break; + case Slot::CONTEXT: { + __ mov(ecx, ContextOperand(esi, Context::FCONTEXT_INDEX)); + __ mov(edx, ContextOperand(ecx, slot->index())); + __ cmp(edx, Factory::the_hole_value()); + __ j(not_equal, &skip); + __ mov(ContextOperand(ecx, slot->index()), eax); + int offset = Context::SlotOffset(slot->index()); + __ mov(edx, eax); // Preserve the stored value in eax. + __ RecordWrite(ecx, offset, edx, ebx); + break; + } + case Slot::LOOKUP: + __ push(eax); + __ push(esi); + __ push(Immediate(var->name())); + __ CallRuntime(Runtime::kInitializeConstContextSlot, 3); + break; + } + __ bind(&skip); + + } else if (var->mode() != Variable::CONST) { + // Perform the assignment for non-const variables. Const assignments + // are simply skipped. Slot* slot = var->AsSlot(); switch (slot->type()) { case Slot::PARAMETER: case Slot::LOCAL: - if (op == Token::INIT_CONST) { - // Detect const reinitialization by checking for the hole value. - __ mov(edx, Operand(ebp, SlotOffset(slot))); - __ cmp(edx, Factory::the_hole_value()); - __ j(not_equal, &done); - } // Perform the assignment. __ mov(Operand(ebp, SlotOffset(slot)), eax); break; case Slot::CONTEXT: { MemOperand target = EmitSlotSearch(slot, ecx); - if (op == Token::INIT_CONST) { - // Detect const reinitialization by checking for the hole value. - __ mov(edx, target); - __ cmp(edx, Factory::the_hole_value()); - __ j(not_equal, &done); - } // Perform the assignment and issue the write barrier. __ mov(target, eax); // The value of the assignment is in eax. RecordWrite clobbers its // register arguments. __ mov(edx, eax); - int offset = FixedArray::kHeaderSize + slot->index() * kPointerSize; + int offset = Context::SlotOffset(slot->index()); __ RecordWrite(ecx, offset, edx, ebx); break; } case Slot::LOOKUP: - // Call the runtime for the assignment. The runtime will ignore - // const reinitialization. + // Call the runtime for the assignment. __ push(eax); // Value. __ push(esi); // Context. __ push(Immediate(var->name())); - if (op == Token::INIT_CONST) { - // The runtime will ignore const redeclaration. - __ CallRuntime(Runtime::kInitializeConstContextSlot, 3); - } else { - __ CallRuntime(Runtime::kStoreContextSlot, 3); - } + __ CallRuntime(Runtime::kStoreContextSlot, 3); break; } - __ bind(&done); } } diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 851a111..6dc3c3c 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -1914,19 +1914,15 @@ void LCodeGen::DoStoreGlobal(LStoreGlobal* instr) { void LCodeGen::DoLoadContextSlot(LLoadContextSlot* instr) { - Register context = ToRegister(instr->InputAt(0)); + Register context = ToRegister(instr->context()); Register result = ToRegister(instr->result()); - __ mov(result, - Operand(context, Context::SlotOffset(Context::FCONTEXT_INDEX))); - __ mov(result, ContextOperand(result, instr->slot_index())); + __ mov(result, ContextOperand(context, instr->slot_index())); } void LCodeGen::DoStoreContextSlot(LStoreContextSlot* instr) { Register context = ToRegister(instr->context()); Register value = ToRegister(instr->value()); - __ mov(context, - Operand(context, Context::SlotOffset(Context::FCONTEXT_INDEX))); __ mov(ContextOperand(context, instr->slot_index()), value); if (instr->needs_write_barrier()) { Register temp = ToRegister(instr->TempAt(0)); diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index 1d5cefc..092601d 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -1685,13 +1685,15 @@ LInstruction* LChunkBuilder::DoLoadContextSlot(HLoadContextSlot* instr) { LInstruction* LChunkBuilder::DoStoreContextSlot(HStoreContextSlot* instr) { - LOperand* context = UseTempRegister(instr->context()); + LOperand* context; LOperand* value; LOperand* temp; if (instr->NeedsWriteBarrier()) { + context = UseTempRegister(instr->context()); value = UseTempRegister(instr->value()); temp = TempRegister(); } else { + context = UseRegister(instr->context()); value = UseRegister(instr->value()); temp = NULL; } diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc index a4c9b11..ee4e3d9 100644 --- a/src/ia32/macro-assembler-ia32.cc +++ b/src/ia32/macro-assembler-ia32.cc @@ -1523,11 +1523,21 @@ void MacroAssembler::LoadContext(Register dst, int context_chain_length) { mov(dst, Operand(dst, Context::SlotOffset(Context::CLOSURE_INDEX))); mov(dst, FieldOperand(dst, JSFunction::kContextOffset)); } - // The context may be an intermediate context, not a function context. - mov(dst, Operand(dst, Context::SlotOffset(Context::FCONTEXT_INDEX))); - } else { // Slot is in the current function context. - // The context may be an intermediate context, not a function context. - mov(dst, Operand(esi, Context::SlotOffset(Context::FCONTEXT_INDEX))); + } else { + // Slot is in the current function context. Move it into the + // destination register in case we store into it (the write barrier + // cannot be allowed to destroy the context in esi). + mov(dst, esi); + } + + // We should not have found a 'with' context by walking the context chain + // (i.e., the static scope chain and runtime context chain do not agree). + // A variable occurring in such a scope should have slot type LOOKUP and + // not CONTEXT. + if (FLAG_debug_code) { + cmp(dst, Operand(dst, Context::SlotOffset(Context::FCONTEXT_INDEX))); + Check(equal, "Yo dawg, I heard you liked function contexts " + "so I put function contexts in all your contexts"); } } diff --git a/src/prettyprinter.cc b/src/prettyprinter.cc index 211f3f6..dda7abb 100644 --- a/src/prettyprinter.cc +++ b/src/prettyprinter.cc @@ -297,13 +297,13 @@ void PrettyPrinter::VisitSlot(Slot* node) { Print("parameter[%d]", node->index()); break; case Slot::LOCAL: - Print("frame[%d]", node->index()); + Print("local[%d]", node->index()); break; case Slot::CONTEXT: - Print(".context[%d]", node->index()); + Print("context[%d]", node->index()); break; case Slot::LOOKUP: - Print(".context["); + Print("lookup["); PrintLiteral(node->var()->name(), false); Print("]"); break; @@ -999,24 +999,7 @@ void AstPrinter::VisitCatchExtensionObject(CatchExtensionObject* node) { void AstPrinter::VisitSlot(Slot* node) { PrintIndented("SLOT "); - switch (node->type()) { - case Slot::PARAMETER: - Print("parameter[%d]", node->index()); - break; - case Slot::LOCAL: - Print("frame[%d]", node->index()); - break; - case Slot::CONTEXT: - Print(".context[%d]", node->index()); - break; - case Slot::LOOKUP: - Print(".context["); - PrintLiteral(node->var()->name(), false); - Print("]"); - break; - default: - UNREACHABLE(); - } + PrettyPrinter::VisitSlot(node); Print("\n"); } -- 2.7.4