From 89984f72410b52d7821d820e1002b910d7dcc0d7 Mon Sep 17 00:00:00 2001 From: "jarin@chromium.org" Date: Wed, 13 Aug 2014 09:23:35 +0000 Subject: [PATCH] Refactor building of lazy bailouts in AstGraphBuilder. BUG= R=mstarzinger@chromium.org Review URL: https://codereview.chromium.org/464033002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23096 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, 77 insertions(+), 84 deletions(-) diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc index 49a6715..9359719 100644 --- a/src/compiler/ast-graph-builder.cc +++ b/src/compiler/ast-graph-builder.cc @@ -238,12 +238,8 @@ Node* AstGraphBuilder::Environment::Checkpoint(BailoutId ast_id) { AstGraphBuilder::AstContext::AstContext(AstGraphBuilder* own, - Expression::Context kind, - BailoutId bailout_id) - : bailout_id_(bailout_id), - kind_(kind), - owner_(own), - outer_(own->ast_context()) { + Expression::Context kind) + : kind_(kind), owner_(own), outer_(own->ast_context()) { owner()->set_ast_context(this); // Push. #ifdef DEBUG original_height_ = environment()->stack_height(); @@ -271,28 +267,6 @@ 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. } @@ -359,7 +333,7 @@ void AstGraphBuilder::VisitForValues(ZoneList* exprs) { void AstGraphBuilder::VisitForValue(Expression* expr) { - AstValueContext for_value(this, expr->id()); + AstValueContext for_value(this); if (!HasStackOverflow()) { expr->Accept(this); } @@ -367,7 +341,7 @@ void AstGraphBuilder::VisitForValue(Expression* expr) { void AstGraphBuilder::VisitForEffect(Expression* expr) { - AstEffectContext for_effect(this, expr->id()); + AstEffectContext for_effect(this); if (!HasStackOverflow()) { expr->Accept(this); } @@ -375,7 +349,7 @@ void AstGraphBuilder::VisitForEffect(Expression* expr) { void AstGraphBuilder::VisitForTest(Expression* expr) { - AstTestContext for_condition(this, expr->id()); + AstTestContext for_condition(this); if (!HasStackOverflow()) { expr->Accept(this); } @@ -713,7 +687,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { Node* exit_cond = NewNode(javascript()->LessThan(), index, cache_length); // TODO(jarin): provide real bailout id. - BuildLazyBailout(exit_cond, BailoutId::None()); + PrepareFrameState(exit_cond, BailoutId::None()); for_loop.BreakUnless(exit_cond); // TODO(dcarney): this runtime call should be a handful of // simplified instructions that @@ -753,7 +727,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { Node* res = ProcessArguments( javascript()->Call(3, NO_CALL_FUNCTION_FLAGS), 3); // TODO(jarin): provide real bailout id. - BuildLazyBailout(res, BailoutId::None()); + PrepareFrameState(res, BailoutId::None()); Node* property_missing = NewNode(javascript()->StrictEqual(), res, jsgraph()->ZeroConstant()); { @@ -765,7 +739,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); environment()->Poke(0, index_inc); // TODO(jarin): provide real bailout id. - BuildLazyBailout(index_inc, BailoutId::None()); + PrepareFrameState(index_inc, BailoutId::None()); for_loop.Continue(); is_property_missing.Else(); is_property_missing.End(); @@ -784,7 +758,7 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); environment()->Poke(0, index_inc); // TODO(jarin): provide real bailout id. - BuildLazyBailout(index_inc, BailoutId::None()); + PrepareFrameState(index_inc, BailoutId::None()); for_loop.EndBody(); for_loop.EndLoop(); environment()->Drop(5); @@ -932,7 +906,7 @@ void AstGraphBuilder::VisitObjectLiteral(ObjectLiteral* expr) { PrintableUnique name = MakeUnique(key->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), literal, value); - BuildLazyBailout(store, key->id()); + PrepareFrameState(store, key->id()); } else { VisitForEffect(property->value()); } @@ -1024,7 +998,7 @@ void AstGraphBuilder::VisitArrayLiteral(ArrayLiteral* expr) { Node* value = environment()->Pop(); Node* index = jsgraph()->Constant(i); Node* store = NewNode(javascript()->StoreProperty(), literal, index, value); - BuildLazyBailout(store, expr->GetIdForElement(i)); + PrepareFrameState(store, expr->GetIdForElement(i)); } environment()->Pop(); // Array literal index. @@ -1056,7 +1030,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. - BuildLazyBailout(store, BailoutId::None()); + PrepareFrameState(store, BailoutId::None()); break; } case KEYED_PROPERTY: { @@ -1068,7 +1042,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. - BuildLazyBailout(store, BailoutId::None()); + PrepareFrameState(store, BailoutId::None()); break; } } @@ -1112,14 +1086,14 @@ void AstGraphBuilder::VisitAssignment(Assignment* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); old_value = NewNode(javascript()->LoadNamed(name), object); - BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); + PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); break; } case KEYED_PROPERTY: { Node* key = environment()->Top(); Node* object = environment()->Peek(1); old_value = NewNode(javascript()->LoadProperty(), object, key); - BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); + PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); break; } } @@ -1129,7 +1103,7 @@ void AstGraphBuilder::VisitAssignment(Assignment* expr) { Node* left = environment()->Pop(); Node* value = BuildBinaryOp(left, right, expr->binary_op()); environment()->Push(value); - BuildLazyBailout(value, expr->binary_operation()->id()); + PrepareFrameState(value, expr->binary_operation()->id()); } else { VisitForValue(expr->value()); } @@ -1148,14 +1122,14 @@ void AstGraphBuilder::VisitAssignment(Assignment* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), object, value); - BuildLazyBailout(store, expr->AssignmentId()); + PrepareFrameState(store, expr->AssignmentId()); break; } case KEYED_PROPERTY: { Node* key = environment()->Pop(); Node* object = environment()->Pop(); Node* store = NewNode(javascript()->StoreProperty(), object, key, value); - BuildLazyBailout(store, expr->AssignmentId()); + PrepareFrameState(store, expr->AssignmentId()); break; } } @@ -1198,7 +1172,8 @@ void AstGraphBuilder::VisitProperty(Property* expr) { Node* object = environment()->Pop(); value = NewNode(javascript()->LoadProperty(), object, key); } - ast_context()->ProduceValueWithLazyBailout(value); + PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); + ast_context()->ProduceValue(value); } @@ -1242,7 +1217,7 @@ void AstGraphBuilder::VisitCall(Call* expr) { Node* key = environment()->Pop(); callee_value = NewNode(javascript()->LoadProperty(), object, key); } - BuildLazyBailoutWithPushedNode(callee_value, property->LoadId()); + PrepareFrameState(callee_value, property->LoadId(), PUSH_OUTPUT); 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, @@ -1297,7 +1272,8 @@ 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); - ast_context()->ProduceValueWithLazyBailout(value); + PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); + ast_context()->ProduceValue(value); } @@ -1311,7 +1287,8 @@ 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); - ast_context()->ProduceValueWithLazyBailout(value); + PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); + ast_context()->ProduceValue(value); } @@ -1327,7 +1304,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). - BuildLazyBailout(callee_value, BailoutId::None()); + PrepareFrameState(callee_value, BailoutId::None()); environment()->Push(receiver_value); // Evaluate all arguments to the JS runtime call. @@ -1337,7 +1314,8 @@ 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); - ast_context()->ProduceValueWithLazyBailout(value); + PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); + ast_context()->ProduceValue(value); } @@ -1359,7 +1337,8 @@ void AstGraphBuilder::VisitCallRuntime(CallRuntime* expr) { Runtime::FunctionId functionId = function->function_id; Operator* call = javascript()->Runtime(functionId, args->length()); Node* value = ProcessArguments(call, args->length()); - ast_context()->ProduceValueWithLazyBailout(value); + PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); + ast_context()->ProduceValue(value); } @@ -1406,7 +1385,7 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); old_value = NewNode(javascript()->LoadNamed(name), object); - BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); + PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); stack_depth = 1; break; } @@ -1416,7 +1395,7 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { Node* key = environment()->Top(); Node* object = environment()->Peek(1); old_value = NewNode(javascript()->LoadProperty(), object, key); - BuildLazyBailoutWithPushedNode(old_value, property->LoadId()); + PrepareFrameState(old_value, property->LoadId(), PUSH_OUTPUT); stack_depth = 2; break; } @@ -1433,7 +1412,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). - BuildLazyBailout(value, BailoutId::None()); + PrepareFrameState(value, BailoutId::None()); // Store the value. switch (assign_type) { @@ -1448,14 +1427,14 @@ void AstGraphBuilder::VisitCountOperation(CountOperation* expr) { PrintableUnique name = MakeUnique(property->key()->AsLiteral()->AsPropertyName()); Node* store = NewNode(javascript()->StoreNamed(name), object, value); - BuildLazyBailout(store, expr->AssignmentId()); + PrepareFrameState(store, expr->AssignmentId()); break; } case KEYED_PROPERTY: { Node* key = environment()->Pop(); Node* object = environment()->Pop(); Node* store = NewNode(javascript()->StoreProperty(), object, key, value); - BuildLazyBailout(store, expr->AssignmentId()); + PrepareFrameState(store, expr->AssignmentId()); break; } } @@ -1480,7 +1459,8 @@ void AstGraphBuilder::VisitBinaryOperation(BinaryOperation* expr) { Node* right = environment()->Pop(); Node* left = environment()->Pop(); Node* value = BuildBinaryOp(left, right, expr->op()); - ast_context()->ProduceValueWithLazyBailout(value); + PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine()); + ast_context()->ProduceValue(value); } } } @@ -1530,7 +1510,7 @@ void AstGraphBuilder::VisitCompareOperation(CompareOperation* expr) { Node* value = NewNode(op, left, right); ast_context()->ProduceValue(value); - BuildLazyBailout(value, expr->id()); + PrepareFrameState(value, expr->id()); } @@ -1760,7 +1740,7 @@ Node* AstGraphBuilder::BuildVariableLoad(Variable* variable, PrintableUnique name = MakeUnique(variable->name()); Operator* op = javascript()->LoadNamed(name, contextual_mode); Node* node = NewNode(op, global); - BuildLazyBailoutWithPushedNode(node, bailout_id); + PrepareFrameState(node, bailout_id, PUSH_OUTPUT); return node; } case Variable::PARAMETER: @@ -1861,7 +1841,7 @@ Node* AstGraphBuilder::BuildVariableAssignment(Variable* variable, Node* value, PrintableUnique name = MakeUnique(variable->name()); Operator* op = javascript()->StoreNamed(name); Node* store = NewNode(op, global, value); - BuildLazyBailout(store, bailout_id); + PrepareFrameState(store, bailout_id); return store; } case Variable::PARAMETER: @@ -2013,11 +1993,16 @@ Node* AstGraphBuilder::BuildBinaryOp(Node* left, Node* right, Token::Value op) { } -void AstGraphBuilder::BuildLazyBailout(Node* node, BailoutId ast_id) { +void AstGraphBuilder::PrepareFrameState(Node* node, BailoutId ast_id, + OutputFrameStateCombine combine) { 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 = @@ -2039,17 +2024,14 @@ void AstGraphBuilder::BuildLazyBailout(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 861bd5b..c9ed8d6 100644 --- a/src/compiler/ast-graph-builder.h +++ b/src/compiler/ast-graph-builder.h @@ -171,8 +171,18 @@ class AstGraphBuilder : public StructuredGraphBuilder, public AstVisitor { // Dispatched from VisitForInStatement. void VisitForInAssignment(Expression* expr, Node* value); - void BuildLazyBailout(Node* node, BailoutId ast_id); - void BuildLazyBailoutWithPushedNode(Node* node, BailoutId ast_id); + // 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(); DEFINE_AST_VISITOR_SUBCLASS_MEMBERS(); DISALLOW_COPY_AND_ASSIGN(AstGraphBuilder); @@ -282,10 +292,15 @@ 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. @@ -295,8 +310,7 @@ class AstGraphBuilder::AstContext BASE_EMBEDDED { void ReplaceValue() { ProduceValue(ConsumeValue()); } protected: - AstContext(AstGraphBuilder* owner, Expression::Context kind, - BailoutId bailout_id); + AstContext(AstGraphBuilder* owner, Expression::Context kind); virtual ~AstContext(); AstGraphBuilder* owner() const { return owner_; } @@ -308,8 +322,6 @@ class AstGraphBuilder::AstContext BASE_EMBEDDED { int original_height_; #endif - BailoutId bailout_id_; - private: Expression::Context kind_; AstGraphBuilder* owner_; @@ -320,11 +332,10 @@ 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, BailoutId bailout_id) - : AstContext(owner, Expression::kEffect, bailout_id) {} + explicit AstEffectContext(AstGraphBuilder* owner) + : AstContext(owner, Expression::kEffect) {} virtual ~AstEffectContext(); virtual void ProduceValue(Node* value) V8_OVERRIDE; - virtual void ProduceValueWithLazyBailout(Node* value) V8_OVERRIDE; virtual Node* ConsumeValue() V8_OVERRIDE; }; @@ -332,11 +343,10 @@ 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, BailoutId bailout_id) - : AstContext(owner, Expression::kValue, bailout_id) {} + explicit AstValueContext(AstGraphBuilder* owner) + : AstContext(owner, Expression::kValue) {} virtual ~AstValueContext(); virtual void ProduceValue(Node* value) V8_OVERRIDE; - virtual void ProduceValueWithLazyBailout(Node* value) V8_OVERRIDE; virtual Node* ConsumeValue() V8_OVERRIDE; }; @@ -344,11 +354,10 @@ 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, BailoutId bailout_id) - : AstContext(owner, Expression::kTest, bailout_id) {} + explicit AstTestContext(AstGraphBuilder* owner) + : AstContext(owner, Expression::kTest) {} 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 9c414f1..62972e0 100644 --- a/src/compiler/graph-builder.cc +++ b/src/compiler/graph-builder.cc @@ -30,6 +30,8 @@ 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