Reviewers: Mads Ager,
Message:
There was no bug for stores, because we used an internal AssignmentId that
corresponds to immediately after the assignment (not after the expression).
I'm not thrilled with this as a long-term. I'd rather get rid of the
HSimulate
before every HGoto and the SetJoinId stuff.
Description:
Fix an bug in deoptimization after polymorphic calls in effect contexts.
For polymorphic calls (also loads and stores) we construct a type switch
graph that has a basic block merging all the variants. There is an
environment simulation before the goto at the end of all the predecessor
blocks. This simulation is used to define the environment on entry to the
successor block, and captures the return value of the call. In effect
contexts, this value should not be present in the environment.
The fix is to use the AST context to decide whether to have this value in
the join node's environment at all.
BUG=1014
Please review this at http://codereview.chromium.org/6065014/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/hydrogen.cc
Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index
63a651b1e9dd2ce4341c23262893ecabc40adb4a..0d92b2eebe5be196e17be4614eaeb10873ebf652
100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -3103,8 +3103,15 @@ HBasicBlock*
HGraphBuilder::BuildTypeSwitch(ZoneMapList* maps,
// this basic block the current basic block.
HBasicBlock* join_block = graph_->CreateBasicBlock();
for (int i = 0; i < subgraphs->length(); ++i) {
- if (subgraphs->at(i)->HasExit()) {
- subgraphs->at(i)->exit_block()->Goto(join_block);
+ HSubgraph* subgraph = subgraphs->at(i);
+ if (subgraph->HasExit()) {
+ // In an effect context the value of the type switch is not needed.
+ // There is no need to merge it at the join block only to discard it.
+ HBasicBlock* subgraph_exit = subgraph->exit_block();
+ if (ast_context()->IsEffect()) {
+ subgraph_exit->last_environment()->Drop(1);
+ }
+ subgraph_exit->Goto(join_block);
}
}
@@ -3242,7 +3249,8 @@ void
HGraphBuilder::HandlePolymorphicStoreNamedField(Assignment* expr,
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());
} else {
// Build subgraph for generic store through IC.
{
@@ -3260,11 +3268,14 @@ void
HGraphBuilder::HandlePolymorphicStoreNamedField(Assignment* expr,
}
HBasicBlock* new_exit_block =
- BuildTypeSwitch(&maps, &subgraphs, object, expr->AssignmentId());
+ BuildTypeSwitch(&maps, &subgraphs, object, expr->id());
subgraph()->set_exit_block(new_exit_block);
+ // In an effect context, we did not materialized the value in the
+ // predecessor environments so there's no need to handle it here.
+ if (subgraph()->HasExit() && !ast_context()->IsEffect()) {
+ ast_context()->ReturnValue(Pop());
+ }
}
-
- if (subgraph()->HasExit()) ast_context()->ReturnValue(Pop());
}
@@ -3548,8 +3559,7 @@ void
HGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
if (maps.length() == 0) {
HInstruction* instr = BuildLoadNamedGeneric(object, expr);
instr->set_position(expr->position());
- PushAndAdd(instr);
- if (instr->HasSideEffects()) AddSimulate(expr->id());
+ ast_context()->ReturnInstruction(instr, expr->id());
} else {
// Build subgraph for generic load through IC.
{
@@ -3568,9 +3578,12 @@ void
HGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
HBasicBlock* new_exit_block =
BuildTypeSwitch(&maps, &subgraphs, object, expr->id());
subgraph()->set_exit_block(new_exit_block);
+ // In an effect context, we did not materialized the value in the
+ // predecessor environments so there's no need to handle it here.
+ if (subgraph()->HasExit() && !ast_context()->IsEffect()) {
+ ast_context()->ReturnValue(Pop());
+ }
}
-
- if (subgraph()->HasExit()) ast_context()->ReturnValue(Pop());
}
@@ -3856,7 +3869,11 @@ void HGraphBuilder::HandlePolymorphicCallNamed(Call*
expr,
HBasicBlock* new_exit_block =
BuildTypeSwitch(&maps, &subgraphs, receiver, expr->id());
subgraph()->set_exit_block(new_exit_block);
- if (new_exit_block != NULL) ast_context()->ReturnValue(Pop());
+ // In an effect context, we did not materialized the value in the
+ // predecessor environments so there's no need to handle it here.
+ if (new_exit_block != NULL && !ast_context()->IsEffect()) {
+ ast_context()->ReturnValue(Pop());
+ }
}
}
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
