Yes and yes. On Mon, Dec 13, 2010 at 5:39 PM, Kasper Lund <[email protected]> wrote:
> 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 > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
