Should this have been a fix of issue 969 instead of 989? ... and if so, would it make sense to re-enable the test disabled in http://code.google.com/p/v8/source/detail?r=5980?
Cheers, Kasper On Mon, Dec 13, 2010 at 5:30 PM, <[email protected]> wrote: > Revision: 5990 > Author: [email protected] > Date: Mon Dec 13 08:29:47 2010 > Log: Deoptimize to the proper target after assignment side effects. > > This fixes V8 issue 989. > > Before, assignments used the AST ID of the assignment expression to > mark the side effect of the store, which became a target for > deoptimization bailout for code after the assignment. In effect > contexts this environment included the value of the assignment, which > was unexpected by the unoptimized code. > > Now we introduce a new assignment ID for AST node types that include > an assignment (Assignment, CountOperation, and ForInStatement) and use > it for the side effect of the store. > > Review URL: http://codereview.chromium.org/5682010 > http://code.google.com/p/v8/source/detail?r=5990 > > Added: > /branches/bleeding_edge/test/mjsunit/regress/regress-989.js > Modified: > /branches/bleeding_edge/src/ast-inl.h > /branches/bleeding_edge/src/ast.cc > /branches/bleeding_edge/src/ast.h > /branches/bleeding_edge/src/full-codegen.h > /branches/bleeding_edge/src/hydrogen.cc > /branches/bleeding_edge/src/ia32/full-codegen-ia32.cc > > ======================================= > --- /dev/null > +++ /branches/bleeding_edge/test/mjsunit/regress/regress-989.js Mon Dec 13 > 08:29:47 2010 > @@ -0,0 +1,127 @@ > +// 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 > +// with the distribution. > +// * Neither the name of Google Inc. nor the names of its > +// contributors may be used to endorse or promote products derived > +// from this software without specific prior written permission. > +// > +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > +// Regression test for bugs when deoptimizing after assignments in effect > +// contexts. > + > +// Bug 989 is that there was an extra value on the expression stack when > +// deoptimizing after an assignment in effect context (the value of the > +// assignment was lingering). This is hard to observe in the unoptimized > +// code. > +// > +// This test uses comma expressions to put assignments in effect contexts, > +// references to deleted global variables to force deoptimization, and > +// function calls to observe an extra value. > + > +function first(x, y) { return x; } > +var y = 0; > +var o = {}; > +o.x = 0; > +o[0] = 0; > + > +// Assignment to global variable. > +x0 = 0; > +function test0() { return first((y = 1, typeof x0), 2); } > +// Call the function once to compile it. > +assertEquals('number', test0()); > +// Delete to force deoptimization on the next call. > +delete x0; > +assertEquals('undefined', test0()); > + > +// Compound assignment to global variable. > +x1 = 0; > +function test1() { return first((y += 1, typeof x1), 2); } > +assertEquals('number', test1(), 'test1 before'); > +delete x1; > +assertEquals('undefined', test1(), 'test1 after'); > + > +// Pre and post-increment of global variable. > +x2 = 0; > +function test2() { return first((++y, typeof x2), 2); } > +assertEquals('number', test2(), 'test2 before'); > +delete x2; > +assertEquals('undefined', test2(), 'test2 after'); > + > +x3 = 0; > +function test3() { return first((y++, typeof x3), 2); } > +assertEquals('number', test3(), 'test3 before'); > +delete x3; > +assertEquals('undefined', test3(), 'test3 after'); > + > + > +// Assignment, compound assignment, and pre and post-increment of named > +// properties. > +x4 = 0; > +function test4() { return first((o.x = 1, typeof x4), 2); } > +assertEquals('number', test4()); > +delete x4; > +assertEquals('undefined', test4()); > + > +x5 = 0; > +function test5() { return first((o.x += 1, typeof x5), 2); } > +assertEquals('number', test5()); > +delete x5; > +assertEquals('undefined', test5()); > + > +x6 = 0; > +function test6() { return first((++o.x, typeof x6), 2); } > +assertEquals('number', test6()); > +delete x6; > +assertEquals('undefined', test6()); > + > +x7 = 0; > +function test7() { return first((o.x++, typeof x7), 2); } > +assertEquals('number', test7()); > +delete x7; > +assertEquals('undefined', test7()); > + > + > +// Assignment, compound assignment, and pre and post-increment of indexed > +// properties. > +x8 = 0; > +function test8(index) { return first((o[index] = 1, typeof x8), 2); } > +assertEquals('number', test8()); > +delete x8; > +assertEquals('undefined', test8()); > + > +x9 = 0; > +function test9(index) { return first((o[index] += 1, typeof x9), 2); } > +assertEquals('number', test9()); > +delete x9; > +assertEquals('undefined', test9()); > + > +x10 = 0; > +function test10(index) { return first((++o[index], typeof x10), 2); } > +assertEquals('number', test10()); > +delete x10; > +assertEquals('undefined', test10()); > + > +x11 = 0; > +function test11(index) { return first((o[index]++, typeof x11), 2); } > +assertEquals('number', test11()); > +delete x11; > +assertEquals('undefined', test11()); > ======================================= > --- /branches/bleeding_edge/src/ast-inl.h Tue Dec 7 03:31:57 2010 > +++ /branches/bleeding_edge/src/ast-inl.h Mon Dec 13 08:29:47 2010 > @@ -94,7 +94,8 @@ > > > ForInStatement::ForInStatement(ZoneStringList* labels) > - : IterationStatement(labels), each_(NULL), enumerable_(NULL) { > + : IterationStatement(labels), each_(NULL), enumerable_(NULL), > + assignment_id_(GetNextId()) { > } > > > ======================================= > --- /branches/bleeding_edge/src/ast.cc Tue Dec 7 03:31:57 2010 > +++ /branches/bleeding_edge/src/ast.cc Mon Dec 13 08:29:47 2010 > @@ -125,17 +125,18 @@ > target_(target), > value_(value), > pos_(pos), > - compound_bailout_id_(kNoNumber), > + binary_operation_(NULL), > + compound_load_id_(kNoNumber), > + assignment_id_(GetNextId()), > block_start_(false), > block_end_(false), > is_monomorphic_(false), > receiver_types_(NULL) { > ASSERT(Token::IsAssignmentOp(op)); > - binary_operation_ = is_compound() > - ? new BinaryOperation(binary_op(), target, value, pos + 1) > - : NULL; > if (is_compound()) { > - compound_bailout_id_ = GetNextId(); > + binary_operation_ = > + new BinaryOperation(binary_op(), target, value, pos + 1); > + compound_load_id_ = GetNextId(); > } > } > > ======================================= > --- /branches/bleeding_edge/src/ast.h Thu Dec 9 06:09:50 2010 > +++ /branches/bleeding_edge/src/ast.h Mon Dec 13 08:29:47 2010 > @@ -575,11 +575,13 @@ > Expression* enumerable() const { return enumerable_; } > > // Bailout support. > + int AssignmentId() const { return assignment_id_; } > virtual int ContinueId() const { return EntryId(); } > > private: > Expression* each_; > Expression* enumerable_; > + int assignment_id_; > }; > > > @@ -1426,7 +1428,9 @@ > class CountOperation: public Expression { > public: > CountOperation(bool is_prefix, IncrementOperation* increment, int pos) > - : is_prefix_(is_prefix), increment_(increment), pos_(pos) { } > + : is_prefix_(is_prefix), increment_(increment), pos_(pos), > + assignment_id_(GetNextId()) { > + } > > DECLARE_NODE_TYPE(CountOperation) > > @@ -1445,11 +1449,15 @@ > virtual void MarkAsStatement() { is_prefix_ = true; } > > virtual bool IsInlineable() const; > + > + // Bailout support. > + int AssignmentId() const { return assignment_id_; } > > private: > bool is_prefix_; > IncrementOperation* increment_; > int pos_; > + int assignment_id_; > }; > > > @@ -1579,7 +1587,8 @@ > } > > // Bailout support. > - int compound_bailout_id() const { return compound_bailout_id_; } > + int CompoundLoadId() const { return compound_load_id_; } > + int AssignmentId() const { return assignment_id_; } > > private: > Token::Value op_; > @@ -1587,7 +1596,8 @@ > Expression* value_; > int pos_; > BinaryOperation* binary_operation_; > - int compound_bailout_id_; > + int compound_load_id_; > + int assignment_id_; > > bool block_start_; > bool block_end_; > ======================================= > --- /branches/bleeding_edge/src/full-codegen.h Mon Dec 13 02:41:50 2010 > +++ /branches/bleeding_edge/src/full-codegen.h Mon Dec 13 08:29:47 2010 > @@ -481,7 +481,7 @@ > > // Assign to the given expression as if via '='. The right-hand-side value > // is expected in the accumulator. > - void EmitAssignment(Expression* expr); > + void EmitAssignment(Expression* expr, int bailout_ast_id); > > // Complete a variable assignment. The right-hand-side value is expected > // in the accumulator. > ======================================= > --- /branches/bleeding_edge/src/hydrogen.cc Mon Dec 13 06:37:19 2010 > +++ /branches/bleeding_edge/src/hydrogen.cc Mon Dec 13 08:29:47 2010 > @@ -3299,7 +3299,7 @@ > Push(value); > instr->set_position(expr->position()); > AddInstruction(instr); > - if (instr->HasSideEffects()) AddSimulate(expr->id()); > + if (instr->HasSideEffects()) AddSimulate(expr->AssignmentId()); > ast_context()->ReturnValue(Pop()); > } > > @@ -3344,7 +3344,10 @@ > VISIT_FOR_VALUE(operation); > > if (var->is_global()) { > - HandleGlobalVariableAssignment(var, Top(), expr->position(), > expr->id()); > + HandleGlobalVariableAssignment(var, > + Top(), > + expr->position(), > + expr->AssignmentId()); > } else { > Bind(var, Top()); > } > @@ -3367,7 +3370,7 @@ > load = BuildLoadNamedGeneric(obj, prop); > } > PushAndAdd(load); > - if (load->HasSideEffects()) AddSimulate(expr->compound_bailout_id()); > + if (load->HasSideEffects()) AddSimulate(expr->CompoundLoadId()); > > VISIT_FOR_VALUE(expr->value()); > HValue* right = Pop(); > @@ -3379,11 +3382,11 @@ > > HInstruction* store = BuildStoreNamed(obj, instr, prop); > AddInstruction(store); > - if (store->HasSideEffects()) AddSimulate(expr->id()); > - > // Drop the simulated receiver and value. Return the value. > Drop(2); > - ast_context()->ReturnValue(instr); > + Push(instr); > + if (store->HasSideEffects()) AddSimulate(expr->AssignmentId()); > + ast_context()->ReturnValue(Pop()); > > } else { > // Keyed property. > @@ -3399,7 +3402,7 @@ > ? BuildLoadKeyedFastElement(obj, key, prop) > : BuildLoadKeyedGeneric(obj, key); > PushAndAdd(load); > - if (load->HasSideEffects()) AddSimulate(expr->compound_bailout_id()); > + if (load->HasSideEffects()) AddSimulate(expr->CompoundLoadId()); > > VISIT_FOR_VALUE(expr->value()); > HValue* right = Pop(); > @@ -3413,11 +3416,11 @@ > ? BuildStoreKeyedFastElement(obj, key, instr, prop) > : BuildStoreKeyedGeneric(obj, key, instr); > AddInstruction(store); > - if (store->HasSideEffects()) AddSimulate(expr->id()); > - > // Drop the simulated receiver, key, and value. Return the value. > Drop(3); > - ast_context()->ReturnValue(instr); > + Push(instr); > + if (store->HasSideEffects()) AddSimulate(expr->AssignmentId()); > + ast_context()->ReturnValue(Pop()); > } > > } else { > @@ -3443,7 +3446,10 @@ > // Handle the assignment. > if (var->is_global()) { > VISIT_FOR_VALUE(expr->value()); > - HandleGlobalVariableAssignment(var, Top(), expr->position(), > expr->id()); > + HandleGlobalVariableAssignment(var, > + Top(), > + expr->position(), > + expr->AssignmentId()); > } else { > // We allow reference to the arguments object only in assignemtns > // to local variables to make sure that the arguments object does > @@ -4537,7 +4543,10 @@ > } > > if (var->is_global()) { > - HandleGlobalVariableAssignment(var, instr, expr->position(), > expr->id()); > + HandleGlobalVariableAssignment(var, > + instr, > + expr->position(), > + expr->AssignmentId()); > } else { > ASSERT(var->IsStackAllocated()); > Bind(var, instr); > @@ -4552,9 +4561,8 @@ > > // Match the full code generator stack by simulating an extra stack > // element for postfix operations in a value context. > - if (expr->is_postfix() && !ast_context()->IsEffect()) { > - Push(graph_->GetConstantUndefined()); > - } > + bool has_extra = expr->is_postfix() && !ast_context()->IsEffect(); > + if (has_extra) Push(graph_->GetConstantUndefined()); > > VISIT_FOR_VALUE(prop->obj()); > HValue* obj = Top(); > @@ -4570,40 +4578,35 @@ > PushAndAdd(load); > if (load->HasSideEffects()) AddSimulate(increment->id()); > > - HValue* value = Pop(); > - > + HValue* before = Pop(); > // There is no deoptimization to after the increment, so we don't need > // to simulate the expression stack after this instruction. > - HInstruction* instr = BuildIncrement(value, inc); > - AddInstruction(instr); > - > - HInstruction* store = BuildStoreNamed(obj, instr, prop); > + HInstruction* after = BuildIncrement(before, inc); > + AddInstruction(after); > + > + HInstruction* store = BuildStoreNamed(obj, after, prop); > AddInstruction(store); > > - // Drop simulated receiver and push the result. > - Drop(1); > - if (expr->is_prefix()) { > - Push(instr); > - } else { > - if (!ast_context()->IsEffect()) Drop(1); // Drop simulated zero. > - Push(value); > - } > - > - if (store->HasSideEffects()) AddSimulate(expr->id()); > - ast_context()->ReturnValue(Pop()); > + // Overwrite the receiver in the bailout environment with the result > + // of the operation, and the placeholder with the original value if > + // necessary. > + environment()->SetExpressionStackAt(0, after); > + if (has_extra) environment()->SetExpressionStackAt(1, before); > + if (store->HasSideEffects()) AddSimulate(expr->AssignmentId()); > + Drop(has_extra ? 2 : 1); > + > + ast_context()->ReturnValue(expr->is_postfix() ? before : after); > > } else { > // Keyed property. > > // Match the full code generator stack by simulate an extra stack > element > // for postfix operations in a value context. > - if (expr->is_postfix() && !ast_context()->IsEffect()) { > - Push(graph_->GetConstantUndefined()); > - } > + bool has_extra = expr->is_postfix() && !ast_context()->IsEffect(); > + if (has_extra) Push(graph_->GetConstantUndefined()); > > VISIT_FOR_VALUE(prop->obj()); > VISIT_FOR_VALUE(prop->key()); > - > HValue* obj = environment()->ExpressionStackAt(1); > HValue* key = environment()->ExpressionStackAt(0); > > @@ -4616,29 +4619,27 @@ > PushAndAdd(load); > if (load->HasSideEffects()) AddSimulate(increment->id()); > > - HValue* value = Pop(); > - > + HValue* before = Pop(); > // There is no deoptimization to after the increment, so we don't need > // to simulate the expression stack after this instruction. > - HInstruction* instr = BuildIncrement(value, inc); > - AddInstruction(instr); > + HInstruction* after = BuildIncrement(before, inc); > + AddInstruction(after); > > HInstruction* store = is_fast_elements > - ? BuildStoreKeyedFastElement(obj, key, instr, prop) > - : new HStoreKeyedGeneric(obj, key, instr); > + ? BuildStoreKeyedFastElement(obj, key, after, prop) > + : new HStoreKeyedGeneric(obj, key, after); > AddInstruction(store); > > - // Drop simulated receiver and key and push the result. > - Drop(2); > - if (expr->is_prefix()) { > - Push(instr); > - } else { > - if (!ast_context()->IsEffect()) Drop(1); // Drop simulated zero. > - Push(value); > - } > - > - if (store->HasSideEffects()) AddSimulate(expr->id()); > - ast_context()->ReturnValue(Pop()); > + // Drop the key from the bailout environment. Overwrite the receiver > + // with the result of the operation, and the placeholder with the > + // original value if necessary. > + Drop(1); > + environment()->SetExpressionStackAt(0, after); > + if (has_extra) environment()->SetExpressionStackAt(1, before); > + if (store->HasSideEffects()) AddSimulate(expr->AssignmentId()); > + Drop(has_extra ? 2 : 1); > + > + ast_context()->ReturnValue(expr->is_postfix() ? before : after); > } > > } else { > ======================================= > --- /branches/bleeding_edge/src/ia32/full-codegen-ia32.cc Mon Dec 13 > 02:41:50 2010 > +++ /branches/bleeding_edge/src/ia32/full-codegen-ia32.cc Mon Dec 13 > 08:29:47 2010 > @@ -911,7 +911,9 @@ > __ bind(&update_each); > __ mov(result_register(), ebx); > // Perform the assignment as if via '='. > - EmitAssignment(stmt->each()); > + { EffectContext context(this); > + EmitAssignment(stmt->each(), stmt->AssignmentId()); > + } > > // Generate code for the body of the loop. > Visit(stmt->body()); > @@ -1478,7 +1480,7 @@ > // For property compound assignments we need another deoptimization > // point after the property load. > if (property != NULL) { > - PrepareForBailoutForId(expr->compound_bailout_id(), TOS_REG); > + PrepareForBailoutForId(expr->CompoundLoadId(), TOS_REG); > } > > Token::Value op = expr->binary_op(); > @@ -1521,6 +1523,8 @@ > case VARIABLE: > EmitVariableAssignment(expr->target()->AsVariableProxy()->var(), > expr->op()); > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > + context()->Plug(eax); > break; > case NAMED_PROPERTY: > EmitNamedPropertyAssignment(expr); > @@ -1849,7 +1853,7 @@ > } > > > -void FullCodeGenerator::EmitAssignment(Expression* expr) { > +void FullCodeGenerator::EmitAssignment(Expression* expr, int > bailout_ast_id) { > // Invalid left-hand sides are rewritten to have a 'throw > // ReferenceError' on the left-hand side. > if (!expr->IsValidLeftHandSide()) { > @@ -1897,6 +1901,8 @@ > break; > } > } > + PrepareForBailoutForId(bailout_ast_id, TOS_REG); > + context()->Plug(eax); > } > > > @@ -1969,8 +1975,6 @@ > } > __ bind(&done); > } > - > - context()->Plug(eax); > } > > > @@ -2007,10 +2011,10 @@ > __ push(Operand(esp, kPointerSize)); // Receiver is under value. > __ CallRuntime(Runtime::kToFastProperties, 1); > __ pop(eax); > - context()->DropAndPlug(1, eax); > - } else { > - context()->Plug(eax); > - } > + __ Drop(1); > + } > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > + context()->Plug(eax); > } > > > @@ -2048,6 +2052,7 @@ > __ pop(eax); > } > > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > context()->Plug(eax); > } > > @@ -3749,6 +3754,8 @@ > { EffectContext context(this); > > EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(), > Token::ASSIGN); > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > + context.Plug(eax); > } > // For all contexts except EffectContext We have the result on > // top of the stack. > @@ -3759,6 +3766,8 @@ > // Perform the assignment as if via '='. > EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(), > Token::ASSIGN); > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > + context()->Plug(eax); > } > break; > case NAMED_PROPERTY: { > @@ -3766,6 +3775,7 @@ > __ pop(edx); > Handle<Code> ic(Builtins::builtin(Builtins::StoreIC_Initialize)); > EmitCallIC(ic, RelocInfo::CODE_TARGET); > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > if (expr->is_postfix()) { > if (!context()->IsEffect()) { > context()->PlugTOS(); > @@ -3780,6 +3790,7 @@ > __ pop(edx); > Handle<Code> ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize)); > EmitCallIC(ic, RelocInfo::CODE_TARGET); > + PrepareForBailoutForId(expr->AssignmentId(), TOS_REG); > if (expr->is_postfix()) { > // Result is on the stack > if (!context()->IsEffect()) { > > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
