From 0eca2b4fc15fbf0e98581992a21e9ff4433f31a2 Mon Sep 17 00:00:00 2001 From: "whesse@chromium.org" Date: Tue, 17 May 2011 11:41:59 +0000 Subject: [PATCH] Fix error in postfix ++ in Crankshaft. Add HForceRepresentation, to represent the implicit ToNumber applied to the input of a count operation. BUG=v8:1389 TEST= Review URL: http://codereview.chromium.org/7033008 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@7913 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/lithium-arm.cc | 8 +++ src/hydrogen-instructions.cc | 7 +++ src/hydrogen-instructions.h | 20 +++++++ src/hydrogen.cc | 104 +++++++++++++++++++---------------- src/hydrogen.h | 3 +- src/ia32/lithium-ia32.cc | 8 +++ src/x64/lithium-x64.cc | 8 +++ test/mjsunit/regress/regress-1389.js | 42 ++++++++++++++ 8 files changed, 150 insertions(+), 50 deletions(-) create mode 100644 test/mjsunit/regress/regress-1389.js diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index 752341b..e9e8351 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -1643,6 +1643,14 @@ LInstruction* LChunkBuilder::DoThrow(HThrow* instr) { } +LInstruction* LChunkBuilder::DoForceRepresentation(HForceRepresentation* bad) { + // All HForceRepresentation instructions should be eliminated in the + // representation change phase of Hydrogen. + UNREACHABLE(); + return NULL; +} + + LInstruction* LChunkBuilder::DoChange(HChange* instr) { Representation from = instr->from(); Representation to = instr->to(); diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 1880111..dc15532 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -1638,6 +1638,13 @@ HValue* HChange::EnsureAndPropagateNotMinusZero(BitVector* visited) { } +HValue* HForceRepresentation::EnsureAndPropagateNotMinusZero( + BitVector* visited) { + visited->Add(id()); + return value(); +} + + HValue* HMod::EnsureAndPropagateNotMinusZero(BitVector* visited) { visited->Add(id()); if (range() == NULL || range()->CanBeMinusZero()) { diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 56810ef..d515943 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -101,6 +101,7 @@ class LChunkBuilder; V(EnterInlined) \ V(ExternalArrayLength) \ V(FixedArrayLength) \ + V(ForceRepresentation) \ V(FunctionLiteral) \ V(GetCachedArrayIndex) \ V(GlobalObject) \ @@ -1009,6 +1010,25 @@ class HThrow: public HUnaryOperation { }; +class HForceRepresentation: public HTemplateInstruction<1> { + public: + HForceRepresentation(HValue* value, Representation required_representation) { + SetOperandAt(0, value); + set_representation(required_representation); + } + + HValue* value() { return OperandAt(0); } + + virtual HValue* EnsureAndPropagateNotMinusZero(BitVector* visited); + + virtual Representation RequiredInputRepresentation(int index) const { + return representation(); // Same as the output representation. + } + + DECLARE_CONCRETE_INSTRUCTION(ForceRepresentation) +}; + + class HChange: public HUnaryOperation { public: HChange(HValue* value, diff --git a/src/hydrogen.cc b/src/hydrogen.cc index 73d3e49..17d0131 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -1803,6 +1803,12 @@ void HGraph::InsertRepresentationChangesForValue(HValue* value) { ASSERT(value->IsConstant()); value->DeleteAndReplaceWith(NULL); } + + // The only purpose of a HForceRepresentation is to represent the value + // after the (possible) HChange instruction. We make it disappear. + if (value->IsForceRepresentation()) { + value->DeleteAndReplaceWith(HForceRepresentation::cast(value)->value()); + } } @@ -4699,20 +4705,35 @@ void HGraphBuilder::VisitNot(UnaryOperation* expr) { } -HInstruction* HGraphBuilder::BuildIncrement(HValue* value, - bool increment, +HInstruction* HGraphBuilder::BuildIncrement(bool returns_original_input, CountOperation* expr) { - HConstant* delta = increment - ? graph_->GetConstant1() - : graph_->GetConstantMinus1(); - HInstruction* instr = new(zone()) HAdd(value, delta); + // The input to the count operation is on top of the expression stack. TypeInfo info = oracle()->IncrementType(expr); Representation rep = ToRepresentation(info); if (rep.IsTagged()) { rep = Representation::Integer32(); } + + if (returns_original_input) { + // We need an explicit HValue representing ToNumber(input). The + // actual HChange instruction we need is (sometimes) added in a later + // phase, so it is not available now to be used as an input to HAdd and + // as the return value. + HInstruction* number_input = new(zone()) HForceRepresentation(Pop(), rep); + AddInstruction(number_input); + Push(number_input); + } + + // The addition has no side effects, so we do not need + // to simulate the expression stack after this instruction. + // Any later failures deopt to the load of the input or earlier. + HConstant* delta = (expr->op() == Token::INC) + ? graph_->GetConstant1() + : graph_->GetConstantMinus1(); + HInstruction* instr = new(zone()) HAdd(Top(), delta); TraceRepresentation(expr->op(), info, instr, rep); instr->AssumeRepresentation(rep); + AddInstruction(instr); return instr; } @@ -4725,18 +4746,25 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) { VariableProxy* proxy = target->AsVariableProxy(); Variable* var = proxy->AsVariable(); Property* prop = target->AsProperty(); - ASSERT(var == NULL || prop == NULL); - bool inc = expr->op() == Token::INC; + if (var == NULL && prop == NULL) { + return Bailout("invalid lhs in count operation"); + } + + // Match the full code generator stack by simulating an extra stack + // element for postfix operations in a non-effect context. The return + // value is ToNumber(input). + bool returns_original_input = + expr->is_postfix() && !ast_context()->IsEffect(); + HValue* input = NULL; // ToNumber(original_input). + HValue* after = NULL; // The result after incrementing or decrementing. if (var != NULL) { + // Argument of the count operation is a variable, not a property. + ASSERT(prop == NULL); CHECK_ALIVE(VisitForValue(target)); - // Match the full code generator stack by simulating an extra stack - // element for postfix operations in a non-effect context. - bool has_extra = expr->is_postfix() && !ast_context()->IsEffect(); - HValue* before = has_extra ? Top() : Pop(); - HInstruction* after = BuildIncrement(before, inc, expr); - AddInstruction(after); + after = BuildIncrement(returns_original_input, expr); + input = returns_original_input ? Top() : Pop(); Push(after); if (var->is_global()) { @@ -4756,19 +4784,15 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) { } else { return Bailout("lookup variable in count operation"); } - Drop(has_extra ? 2 : 1); - ast_context()->ReturnValue(expr->is_postfix() ? before : after); - } else if (prop != NULL) { + } else { + // Argument of the count operation is a property. + ASSERT(prop != NULL); prop->RecordTypeFeedback(oracle()); if (prop->key()->IsPropertyName()) { // Named property. - - // Match the full code generator stack by simulating an extra stack - // element for postfix operations in a non-effect context. - bool has_extra = expr->is_postfix() && !ast_context()->IsEffect(); - if (has_extra) Push(graph_->GetConstantUndefined()); + if (returns_original_input) Push(graph_->GetConstantUndefined()); CHECK_ALIVE(VisitForValue(prop->obj())); HValue* obj = Top(); @@ -4784,11 +4808,8 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) { PushAndAdd(load); if (load->HasSideEffects()) AddSimulate(expr->CountId()); - 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* after = BuildIncrement(before, inc, expr); - AddInstruction(after); + after = BuildIncrement(returns_original_input, expr); + input = Pop(); HInstruction* store = BuildStoreNamed(obj, after, prop); AddInstruction(store); @@ -4797,19 +4818,12 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) { // of the operation, and the placeholder with the original value if // necessary. environment()->SetExpressionStackAt(0, after); - if (has_extra) environment()->SetExpressionStackAt(1, before); + if (returns_original_input) environment()->SetExpressionStackAt(1, input); 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 non-effect context. - bool has_extra = expr->is_postfix() && !ast_context()->IsEffect(); - if (has_extra) Push(graph_->GetConstantUndefined()); + if (returns_original_input) Push(graph_->GetConstantUndefined()); CHECK_ALIVE(VisitForValue(prop->obj())); CHECK_ALIVE(VisitForValue(prop->key())); @@ -4820,11 +4834,8 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) { PushAndAdd(load); if (load->HasSideEffects()) AddSimulate(expr->CountId()); - 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* after = BuildIncrement(before, inc, expr); - AddInstruction(after); + after = BuildIncrement(returns_original_input, expr); + input = Pop(); expr->RecordTypeFeedback(oracle()); HInstruction* store = BuildStoreKeyed(obj, key, after, expr); @@ -4835,16 +4846,13 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) { // original value if necessary. Drop(1); environment()->SetExpressionStackAt(0, after); - if (has_extra) environment()->SetExpressionStackAt(1, before); + if (returns_original_input) environment()->SetExpressionStackAt(1, input); if (store->HasSideEffects()) AddSimulate(expr->AssignmentId()); - Drop(has_extra ? 2 : 1); - - ast_context()->ReturnValue(expr->is_postfix() ? before : after); } - - } else { - return Bailout("invalid lhs in count operation"); } + + Drop(returns_original_input ? 2 : 1); + ast_context()->ReturnValue(expr->is_postfix() ? input : after); } diff --git a/src/hydrogen.h b/src/hydrogen.h index 8be1637..41ae0be 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -888,8 +888,7 @@ class HGraphBuilder: public AstVisitor { HValue* left, HValue* right, TypeInfo info); - HInstruction* BuildIncrement(HValue* value, - bool increment, + HInstruction* BuildIncrement(bool returns_original_input, CountOperation* expr); HLoadNamedField* BuildLoadNamedField(HValue* object, Property* expr, diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index fed25a0..76eaec5 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -1677,6 +1677,14 @@ LInstruction* LChunkBuilder::DoThrow(HThrow* instr) { } +LInstruction* LChunkBuilder::DoForceRepresentation(HForceRepresentation* bad) { + // All HForceRepresentation instructions should be eliminated in the + // representation change phase of Hydrogen. + UNREACHABLE(); + return NULL; +} + + LInstruction* LChunkBuilder::DoChange(HChange* instr) { Representation from = instr->from(); Representation to = instr->to(); diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index 00fd1bb..bfffc9c 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -1648,6 +1648,14 @@ LInstruction* LChunkBuilder::DoThrow(HThrow* instr) { } +LInstruction* LChunkBuilder::DoForceRepresentation(HForceRepresentation* bad) { + // All HForceRepresentation instructions should be eliminated in the + // representation change phase of Hydrogen. + UNREACHABLE(); + return NULL; +} + + LInstruction* LChunkBuilder::DoChange(HChange* instr) { Representation from = instr->from(); Representation to = instr->to(); diff --git a/test/mjsunit/regress/regress-1389.js b/test/mjsunit/regress/regress-1389.js new file mode 100644 index 0000000..9b89bbf --- /dev/null +++ b/test/mjsunit/regress/regress-1389.js @@ -0,0 +1,42 @@ +// Copyright 2011 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. + +// Test optimized implementation of postfix ++ on undefined input. +// See http://code.google.com/p/v8/issues/detail?id=1389 + +for (var i=0; i<4; i++) { + (function () { + (function () { + (function () { + var x; + y = x++; + })(); + })(); + })(); +} + +assertEquals(NaN, y); -- 2.7.4