From e0c1ee1591f4d82d2184988603950f4991bc7d3f Mon Sep 17 00:00:00 2001 From: "jarin@chromium.org" Date: Wed, 13 Aug 2014 10:22:53 +0000 Subject: [PATCH] Revert "Refactor building of lazy bailouts in AstGraphBuilder." This reverts commit r23096. TBR=mstarzinger@chromium.org BUG= Review URL: https://codereview.chromium.org/470573002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23097 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/compiler/ast-graph-builder.cc | 118 ++++++++++++++++++++++---------------- src/compiler/ast-graph-builder.h | 41 ++++++------- src/compiler/graph-builder.cc | 2 - 3 files changed, 84 insertions(+), 77 deletions(-) diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc index 9359719..49a6715 100644 --- a/src/compiler/ast-graph-builder.cc +++ b/src/compiler/ast-graph-builder.cc @@ -238,8 +238,12 @@ Node* AstGraphBuilder::Environment::Checkpoint(BailoutId ast_id) { AstGraphBuilder::AstContext::AstContext(AstGraphBuilder* own, - Expression::Context kind) - : kind_(kind), owner_(own), outer_(own->ast_context()) { + Expression::Context kind, + BailoutId bailout_id) + : bailout_id_(bailout_id), + kind_(kind), + owner_(own), + outer_(own->ast_context()) { owner()->set_ast_context(this); // Push. #ifdef DEBUG original_height_ = environment()->stack_height(); @@ -267,6 +271,28 @@ AstGraphBuilder::AstTestContext::~AstTestContext() { } +void AstGraphBuilder::AstEffectContext::ProduceValueWithLazyBailout( + Node* value) { + ProduceValue(value); + owner()->BuildLazyBailout(value, bailout_id_); +} + + +void AstGraphBuilder::AstValueContext::ProduceValueWithLazyBailout( + Node* value) { + ProduceValue(value); + owner()->BuildLazyBailout(value, bailout_id_); +} + + +void AstGraphBuilder::AstTestContext::ProduceValueWithLazyBailout(Node* value) { + environment()->Push(value); + owner()->BuildLazyBailout(value, bailout_id_); + environment()->Pop(); + ProduceValue(value); +} + + void AstGraphBuilder::AstEffectContext::ProduceValue(Node* value) { // The value is ignored. } @@ -333,7 +359,7 @@ void AstGraphBuilder::VisitForValues(ZoneList* exprs) { void AstGraphBuilder::VisitForValue(Expression* expr) { - AstValueContext for_value(this); + AstValueContext for_value(this, expr->id()); if (!HasStackOverflow()) { expr->Accept(this); } @@ -341,7 +367,7 @@ void AstGraphBuilder::VisitForValue(Expression* expr) { void AstGraphBuilder::VisitForEffect(Expression* expr) { - AstEffectContext for_effect(this); + AstEffectContext for_effect(this, expr->id()); if (!HasStackOverflow()) { expr->Accept(this); } @@ -349,7 +375,7 @@ void AstGraphBuilder::VisitForEffect(Expression* expr) { void AstGraphBuilder::VisitForTest(Expression* expr) { - AstTestContext for_condition(this); + AstTestContext for_condition(this, expr->id()); if (!HasStackOverflow()) { expr->Accept(this); } @@ -687,7 +713,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { Node* exit_cond = NewNode(javascript()->LessThan(), index, cache_length); // TODO(jarin): provide real bailout id. - PrepareFrameState(exit_cond, BailoutId::None()); + BuildLazyBailout(exit_cond, BailoutId::None()); for_loop.BreakUnless(exit_cond); // TODO(dcarney): this runtime call should be a handful of // simplified instructions that @@ -727,7 +753,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { Node* res = ProcessArguments( javascript()->Call(3, NO_CALL_FUNCTION_FLAGS), 3); // TODO(jarin): provide real bailout id. - PrepareFrameState(res, BailoutId::None()); + BuildLazyBailout(res, BailoutId::None()); Node* property_missing = NewNode(javascript()->StrictEqual(), res, jsgraph()->ZeroConstant()); { @@ -739,7 +765,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); environment()->Poke(0, index_inc); // TODO(jarin): provide real bailout id. - PrepareFrameState(index_inc, BailoutId::None()); + BuildLazyBailout(index_inc, BailoutId::None()); for_loop.Continue(); is_property_missing.Else(); is_property_missing.End(); @@ -758,7 +784,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); environment()->Poke(0, index_inc); // TODO(jarin): provide real bailout id. - PrepareFrameState(index_inc, BailoutId::None()); + BuildLazyBailout(index_inc, BailoutId::None()); for_loop.EndBody(); for_loop.EndLoop(); environment()->Drop(5); @@ -906,7 +932,7 @@ void AstGraphBuilder::VisitObjectLiteral(ObjectLiteral* expr) { PrintableUnique name = MakeUnique(key->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), literal, value); - PrepareFrameState(store, key->id()); + BuildLazyBailout(store, key->id()); } else { VisitForEffect(property->value()); } @@ -998,7 +1024,7 @@ void AstGraphBuilder::VisitArrayLiteral(ArrayLiteral* expr) { Node* value = environment()->Pop(); Node* index = jsgraph()->Constant(i); Node* store = NewNode(javascript()->StoreProperty(), literal, index, value); - PrepareFrameState(store, expr->GetIdForElement(i)); + BuildLazyBailout(store, expr->GetIdForElement(i)); } environment()->Pop(); // Array literal index. @@ -1030,7 +1056,7 @@ void AstGraphBuilder::VisitForInAssignment(Expression* expr, Node* value) { MakeUnique(property->key()->AsLiteral()->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), object, value); // TODO(jarin) Fill in the correct bailout id. - PrepareFrameState(store, BailoutId::None()); + BuildLazyBailout(store, BailoutId::None()); break; } case KEYED_PROPERTY: { @@ -1042,7 +1068,7 @@ void AstGraphBuilder::VisitForInAssignment(Expression* expr, Node* value) { value = environment()->Pop(); Node* store = NewNode(javascript()->StoreProperty(), object, key, value); // TODO(jarin) Fill in the correct bailout id. - PrepareFrameState(store, BailoutId::None()); + BuildLazyBailout(store, BailoutId::None()); break; } } @@ -1086,14 +1112,14 @@ void AstGraphBuilder::VisitAssignment(Assignment* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); old_value = NewNode(javascript()->LoadNamed(name), object); - PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); + BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); break; } case KEYED_PROPERTY: { Node* key = environment()->Top(); Node* object = environment()->Peek(1); old_value = NewNode(javascript()->LoadProperty(), object, key); - PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); + BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); break; } } @@ -1103,7 +1129,7 @@ void AstGraphBuilder::VisitAssignment(Assignment* expr) { Node* left = environment()->Pop(); Node* value = BuildBinaryOp(left, right, expr->binary_op()); environment()->Push(value); - PrepareFrameState(value, expr->binary_operation()->id()); + BuildLazyBailout(value, expr->binary_operation()->id()); } else { VisitForValue(expr->value()); } @@ -1122,14 +1148,14 @@ void AstGraphBuilder::VisitAssignment(Assignment* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), object, value); - PrepareFrameState(store, expr->AssignmentId()); + BuildLazyBailout(store, expr->AssignmentId()); break; } case KEYED_PROPERTY: { Node* key = environment()->Pop(); Node* object = environment()->Pop(); Node* store = NewNode(javascript()->StoreProperty(), object, key, value); - PrepareFrameState(store, expr->AssignmentId()); + BuildLazyBailout(store, expr->AssignmentId()); break; } } @@ -1172,8 +1198,7 @@ void AstGraphBuilder::VisitProperty(Property* expr) { Node* object = environment()->Pop(); value = NewNode(javascript()->LoadProperty(), object, key); } - PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); - ast_context()->ProduceValue(value); + ast_context()->ProduceValueWithLazyBailout(value); } @@ -1217,7 +1242,7 @@ void AstGraphBuilder::VisitCall(Call* expr) { Node* key = environment()->Pop(); callee_value = NewNode(javascript()->LoadProperty(), object, key); } - PrepareFrameState(callee_value, property->LoadId(), PUSH_OUTPUT); + BuildLazyBailoutWithPushedNode(callee_value, property->LoadId()); receiver_value = environment()->Pop(); // Note that a PROPERTY_CALL requires the receiver to be wrapped into an // object for sloppy callees. This could also be modeled explicitly here, @@ -1272,8 +1297,7 @@ void AstGraphBuilder::VisitCall(Call* expr) { // Create node to perform the function call. Operator* call = javascript()->Call(args->length() + 2, flags); Node* value = ProcessArguments(call, args->length() + 2); - PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); - ast_context()->ProduceValue(value); + ast_context()->ProduceValueWithLazyBailout(value); } @@ -1287,8 +1311,7 @@ void AstGraphBuilder::VisitCallNew(CallNew* expr) { // Create node to perform the construct call. Operator* call = javascript()->CallNew(args->length() + 1); Node* value = ProcessArguments(call, args->length() + 1); - PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); - ast_context()->ProduceValue(value); + ast_context()->ProduceValueWithLazyBailout(value); } @@ -1304,7 +1327,7 @@ void AstGraphBuilder::VisitCallJSRuntime(CallRuntime* expr) { environment()->Push(callee_value); // TODO(jarin): Find/create a bailout id to deoptimize to (crankshaft // refuses to optimize functions with jsruntime calls). - PrepareFrameState(callee_value, BailoutId::None()); + BuildLazyBailout(callee_value, BailoutId::None()); environment()->Push(receiver_value); // Evaluate all arguments to the JS runtime call. @@ -1314,8 +1337,7 @@ void AstGraphBuilder::VisitCallJSRuntime(CallRuntime* expr) { // Create node to perform the JS runtime call. Operator* call = javascript()->Call(args->length() + 2, flags); Node* value = ProcessArguments(call, args->length() + 2); - PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); - ast_context()->ProduceValue(value); + ast_context()->ProduceValueWithLazyBailout(value); } @@ -1337,8 +1359,7 @@ void AstGraphBuilder::VisitCallRuntime(CallRuntime* expr) { Runtime::FunctionId functionId = function->function_id; Operator* call = javascript()->Runtime(functionId, args->length()); Node* value = ProcessArguments(call, args->length()); - PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); - ast_context()->ProduceValue(value); + ast_context()->ProduceValueWithLazyBailout(value); } @@ -1385,7 +1406,7 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); old_value = NewNode(javascript()->LoadNamed(name), object); - PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); + BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); stack_depth = 1; break; } @@ -1395,7 +1416,7 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { Node* key = environment()->Top(); Node* object = environment()->Peek(1); old_value = NewNode(javascript()->LoadProperty(), object, key); - PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); + BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); stack_depth = 2; break; } @@ -1412,7 +1433,7 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { BuildBinaryOp(old_value, jsgraph()->OneConstant(), expr->binary_op()); // TODO(jarin) Insert proper bailout id here (will need to change // full code generator). - PrepareFrameState(value, BailoutId::None()); + BuildLazyBailout(value, BailoutId::None()); // Store the value. switch (assign_type) { @@ -1427,14 +1448,14 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), object, value); - PrepareFrameState(store, expr->AssignmentId()); + BuildLazyBailout(store, expr->AssignmentId()); break; } case KEYED_PROPERTY: { Node* key = environment()->Pop(); Node* object = environment()->Pop(); Node* store = NewNode(javascript()->StoreProperty(), object, key, value); - PrepareFrameState(store, expr->AssignmentId()); + BuildLazyBailout(store, expr->AssignmentId()); break; } } @@ -1459,8 +1480,7 @@ void AstGraphBuilder::VisitBinaryOperation(BinaryOperation* expr) { Node* right = environment()->Pop(); Node* left = environment()->Pop(); Node* value = BuildBinaryOp(left, right, expr->op()); - PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); - ast_context()->ProduceValue(value); + ast_context()->ProduceValueWithLazyBailout(value); } } } @@ -1510,7 +1530,7 @@ void AstGraphBuilder::VisitCompareOperation(CompareOperation* expr) { Node* value = NewNode(op, left, right); ast_context()->ProduceValue(value); - PrepareFrameState(value, expr->id()); + BuildLazyBailout(value, expr->id()); } @@ -1740,7 +1760,7 @@ Node* AstGraphBuilder::BuildVariableLoad(Variable* variable, PrintableUnique name = MakeUnique(variable->name()); Operator* op = javascript()->LoadNamed(name, contextual_mode); Node* node = NewNode(op, global); - PrepareFrameState(node, bailout_id, PUSH_OUTPUT); + BuildLazyBailoutWithPushedNode(node, bailout_id); return node; } case Variable::PARAMETER: @@ -1841,7 +1861,7 @@ Node* AstGraphBuilder::BuildVariableAssignment(Variable* variable, Node* value, PrintableUnique name = MakeUnique(variable->name()); Operator* op = javascript()->StoreNamed(name); Node* store = NewNode(op, global, value); - PrepareFrameState(store, bailout_id); + BuildLazyBailout(store, bailout_id); return store; } case Variable::PARAMETER: @@ -1993,16 +2013,11 @@ Node* AstGraphBuilder::BuildBinaryOp(Node* left, Node* right, Token::Value op) { } -void AstGraphBuilder::PrepareFrameState(Node* node, BailoutId ast_id, - OutputFrameStateCombine combine) { +void AstGraphBuilder::BuildLazyBailout(Node* node, BailoutId ast_id) { if (OperatorProperties::CanLazilyDeoptimize(node->op())) { // The deopting node should have an outgoing control dependency. DCHECK(environment()->GetControlDependency() == node); - if (combine == PUSH_OUTPUT) { - environment()->Push(node); - } - StructuredGraphBuilder::Environment* continuation_env = environment(); // Create environment for the deoptimization block, and build the block. StructuredGraphBuilder::Environment* deopt_env = @@ -2024,14 +2039,17 @@ void AstGraphBuilder::PrepareFrameState(Node* node, BailoutId ast_id, // Continue with the original environment. set_environment(continuation_env); - if (combine == PUSH_OUTPUT) { - environment()->Pop(); - } - NewNode(common()->Continuation()); } } + +void AstGraphBuilder::BuildLazyBailoutWithPushedNode(Node* node, + BailoutId ast_id) { + environment()->Push(node); + BuildLazyBailout(node, ast_id); + environment()->Pop(); +} } } } // namespace v8::internal::compiler diff --git a/src/compiler/ast-graph-builder.h b/src/compiler/ast-graph-builder.h index c9ed8d6..861bd5b 100644 --- a/src/compiler/ast-graph-builder.h +++ b/src/compiler/ast-graph-builder.h @@ -171,18 +171,8 @@ class AstGraphBuilder : public StructuredGraphBuilder, public AstVisitor { // Dispatched from VisitForInStatement. void VisitForInAssignment(Expression* expr, Node* value); - // Flag that describes how to combine the current environment with - // the output of a node to obtain a framestate for lazy bailout. - enum OutputFrameStateCombine { - PUSH_OUTPUT, // Push the output on the expression stack. - IGNORE_OUTPUT // Use the frame state as-is. - }; - - // Builds deoptimization for a given node. - void PrepareFrameState(Node* node, BailoutId ast_id, - OutputFrameStateCombine combine = IGNORE_OUTPUT); - - OutputFrameStateCombine StateCombineFromAstContext(); + void BuildLazyBailout(Node* node, BailoutId ast_id); + void BuildLazyBailoutWithPushedNode(Node* node, BailoutId ast_id); DEFINE_AST_VISITOR_SUBCLASS_MEMBERS(); DISALLOW_COPY_AND_ASSIGN(AstGraphBuilder); @@ -292,15 +282,10 @@ class AstGraphBuilder::AstContext BASE_EMBEDDED { bool IsValue() const { return kind_ == Expression::kValue; } bool IsTest() const { return kind_ == Expression::kTest; } - // Determines how to combine the frame state with the value - // that is about to be plugged into this AstContext. - AstGraphBuilder::OutputFrameStateCombine GetStateCombine() { - return IsEffect() ? IGNORE_OUTPUT : PUSH_OUTPUT; - } - // Plug a node into this expression context. Call this function in tail // position in the Visit functions for expressions. virtual void ProduceValue(Node* value) = 0; + virtual void ProduceValueWithLazyBailout(Node* value) = 0; // Unplugs a node from this expression context. Call this to retrieve the // result of another Visit function that already plugged the context. @@ -310,7 +295,8 @@ class AstGraphBuilder::AstContext BASE_EMBEDDED { void ReplaceValue() { ProduceValue(ConsumeValue()); } protected: - AstContext(AstGraphBuilder* owner, Expression::Context kind); + AstContext(AstGraphBuilder* owner, Expression::Context kind, + BailoutId bailout_id); virtual ~AstContext(); AstGraphBuilder* owner() const { return owner_; } @@ -322,6 +308,8 @@ class AstGraphBuilder::AstContext BASE_EMBEDDED { int original_height_; #endif + BailoutId bailout_id_; + private: Expression::Context kind_; AstGraphBuilder* owner_; @@ -332,10 +320,11 @@ class AstGraphBuilder::AstContext BASE_EMBEDDED { // Context to evaluate expression for its side effects only. class AstGraphBuilder::AstEffectContext V8_FINAL : public AstContext { public: - explicit AstEffectContext(AstGraphBuilder* owner) - : AstContext(owner, Expression::kEffect) {} + explicit AstEffectContext(AstGraphBuilder* owner, BailoutId bailout_id) + : AstContext(owner, Expression::kEffect, bailout_id) {} virtual ~AstEffectContext(); virtual void ProduceValue(Node* value) V8_OVERRIDE; + virtual void ProduceValueWithLazyBailout(Node* value) V8_OVERRIDE; virtual Node* ConsumeValue() V8_OVERRIDE; }; @@ -343,10 +332,11 @@ class AstGraphBuilder::AstEffectContext V8_FINAL : public AstContext { // Context to evaluate expression for its value (and side effects). class AstGraphBuilder::AstValueContext V8_FINAL : public AstContext { public: - explicit AstValueContext(AstGraphBuilder* owner) - : AstContext(owner, Expression::kValue) {} + explicit AstValueContext(AstGraphBuilder* owner, BailoutId bailout_id) + : AstContext(owner, Expression::kValue, bailout_id) {} virtual ~AstValueContext(); virtual void ProduceValue(Node* value) V8_OVERRIDE; + virtual void ProduceValueWithLazyBailout(Node* value) V8_OVERRIDE; virtual Node* ConsumeValue() V8_OVERRIDE; }; @@ -354,10 +344,11 @@ class AstGraphBuilder::AstValueContext V8_FINAL : public AstContext { // Context to evaluate expression for a condition value (and side effects). class AstGraphBuilder::AstTestContext V8_FINAL : public AstContext { public: - explicit AstTestContext(AstGraphBuilder* owner) - : AstContext(owner, Expression::kTest) {} + explicit AstTestContext(AstGraphBuilder* owner, BailoutId bailout_id) + : AstContext(owner, Expression::kTest, bailout_id) {} virtual ~AstTestContext(); virtual void ProduceValue(Node* value) V8_OVERRIDE; + virtual void ProduceValueWithLazyBailout(Node* value) V8_OVERRIDE; virtual Node* ConsumeValue() V8_OVERRIDE; }; diff --git a/src/compiler/graph-builder.cc b/src/compiler/graph-builder.cc index 62972e0..9c414f1 100644 --- a/src/compiler/graph-builder.cc +++ b/src/compiler/graph-builder.cc @@ -30,8 +30,6 @@ StructuredGraphBuilder::StructuredGraphBuilder(Graph* graph, Node* StructuredGraphBuilder::MakeNode(Operator* op, int value_input_count, Node** value_inputs) { - DCHECK(op->InputCount() == value_input_count); - bool has_context = OperatorProperties::HasContextInput(op); bool has_control = OperatorProperties::GetControlInputCount(op) == 1; bool has_effect = OperatorProperties::GetEffectInputCount(op) == 1; -- 2.7.4