Unify the Count Operation assignment with other assignments.

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 9d4fe2e50529e779b45cde69b4c3140274d8a3a5..c608498b34092735fcf681dbebe0bbde315bf673 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -6365,6 +6365,7 @@ bool HOptimizedGraphBuilder::TryStorePolymorphicAsMonomorphic(
     BailoutId assignment_id,
     HValue* object,
     HValue* value,
+    HValue* result,
     SmallMapList* types,
     Handle<String> name) {
   // Use monomorphic store if property lookup results in the same field index
@@ -6412,10 +6413,12 @@ bool HOptimizedGraphBuilder::TryStorePolymorphicAsMonomorphic(
       store = BuildStoreNamedField(
           object, name, value, types->at(count - 1), &lookup),
       true);
+  if (result != value) Push(result);

[Sven Panne, 2013/06/24 06:58:13]

This comparison (and similar ones elsewhere) are quite confusing, and admittedly
I don't fully understand them. This should be refactored somehow, perhaps using
an enum describing the intention instead of the HValue*.

We should strive to make things understandable locally, without having to have
hydrogen.cc in one's head... :-)

[danno, 2013/06/27 08:34:10]

+1 to sven's comment. It's really difficult to understand what the comparison
between result and value is supposed to check or verify. The enum sounds like a
good idea.

... and some comments would help, too. Why is it OK, for example, to put the
result higher up the environment stack the the value when the simulate is made?
will this really do the right thing? Don't you want to push the result after the
value, and then after the simulate pop the result, pop the value and re-push the
value?

   Push(value);
   store->set_position(position);
   AddInstruction(store);
   AddSimulate(assignment_id);
+  if (result != value) Drop(1);
   ast_context()->ReturnValue(Pop());
   return true;
 }
@@ -6427,10 +6430,11 @@ void HOptimizedGraphBuilder::HandlePolymorphicStoreNamedField(
     BailoutId assignment_id,
     HValue* object,
     HValue* value,
+    HValue* result,
     SmallMapList* types,
     Handle<String> name) {
   if (TryStorePolymorphicAsMonomorphic(
-          position, assignment_id, object, value, types, name)) {
+          position, assignment_id, object, value, result, types, name)) {
     return;
   }
 
@@ -6461,7 +6465,10 @@ void HOptimizedGraphBuilder::HandlePolymorphicStoreNamedField(
       instr->set_position(position);
       // Goto will add the HSimulate for the store.
       AddInstruction(instr);
-      if (!ast_context()->IsEffect()) Push(value);
+      if (!ast_context()->IsEffect()) {
+        if (value != result) Push(result);
+        Push(value);
+      }
       current_block()->Goto(join);
 
       set_current_block(if_false);
@@ -6479,7 +6486,10 @@ void HOptimizedGraphBuilder::HandlePolymorphicStoreNamedField(
     AddInstruction(instr);
 
     if (join != NULL) {
-      if (!ast_context()->IsEffect()) Push(value);
+      if (!ast_context()->IsEffect()) {
+        if (result != value) Push(result);
+        Push(value);
+      }
       current_block()->Goto(join);
     } else {
       // The HSimulate for the store should not see the stored value in
@@ -6489,18 +6499,20 @@ void HOptimizedGraphBuilder::HandlePolymorphicStoreNamedField(
         if (ast_context()->IsEffect()) {
           AddSimulate(id, REMOVABLE_SIMULATE);
         } else {
+          if (result != value) Push(result);
           Push(value);
           AddSimulate(id, REMOVABLE_SIMULATE);
-          Drop(1);
+          Drop(result != value ? 2 : 1);
         }
       }
-      return ast_context()->ReturnValue(value);
+      return ast_context()->ReturnValue(result);
     }
   }
 
   ASSERT(join != NULL);
   join->SetJoinId(id);
   set_current_block(join);
+  if (result != value) Drop(1);

[danno, 2013/07/09 16:22:04]

This is a bug

   if (!ast_context()->IsEffect()) ast_context()->ReturnValue(Pop());
 }
 
@@ -6517,7 +6529,7 @@ void HOptimizedGraphBuilder::HandlePropertyAssignment(Assignment* expr) {
     HValue* object = environment()->ExpressionStackAt(1);
 
     return BuildStoreNamed(expr, expr->id(), expr->position(),
-                           expr->AssignmentId(), prop, object, value);
+                           expr->AssignmentId(), prop, object, value, value);
   } else {
     // Keyed store.
     CHECK_ALIVE(VisitForValue(prop->key()));
@@ -6582,7 +6594,8 @@ void HOptimizedGraphBuilder::BuildStoreNamed(Expression* expr,
                                              BailoutId assignment_id,
                                              Property* prop,
                                              HValue* object,
-                                             HValue* value) {
+                                             HValue* value,
+                                             HValue* result) {
   Literal* key = prop->key()->AsLiteral();
   Handle<String> name = Handle<String>::cast(key->handle());
   ASSERT(!name.is_null());
@@ -6600,7 +6613,8 @@ void HOptimizedGraphBuilder::BuildStoreNamed(Expression* expr,
     Handle<JSObject> holder;
     if (LookupSetter(map, name, &setter, &holder)) {
       AddCheckConstantFunction(holder, object, map);
-      if (FLAG_inline_accessors &&
+      if (result == value &&
+          FLAG_inline_accessors &&
           TryInlineSetter(setter, id, assignment_id, value)) {
         return;
       }
@@ -6615,22 +6629,23 @@ void HOptimizedGraphBuilder::BuildStoreNamed(Expression* expr,
                                                      value,
                                                      map));
     }
-
   } else if (types != NULL && types->length() > 1) {
     Drop(2);
     return HandlePolymorphicStoreNamedField(
-        id, position, assignment_id, object, value, types, name);
+        id, position, assignment_id, object, value, result, types, name);
   } else {
     Drop(2);
     instr = BuildStoreNamedGeneric(object, name, value);
   }
 
+  if (value != result) Push(result);
   Push(value);
   instr->set_position(position);
   AddInstruction(instr);
   if (instr->HasObservableSideEffects()) {
     AddSimulate(assignment_id, REMOVABLE_SIMULATE);
   }
+  if (value != result) Drop(1);
   return ast_context()->ReturnValue(Pop());
 }
 
@@ -6762,7 +6777,7 @@ void HOptimizedGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
       }
 
       return BuildStoreNamed(prop, expr->id(), expr->position(),
-                             expr->AssignmentId(), prop, object, instr);
+                             expr->AssignmentId(), prop, object, instr, instr);
     } else {
       // Keyed property.
       CHECK_ALIVE(VisitForValue(prop->obj()));
@@ -9254,35 +9269,11 @@ void HOptimizedGraphBuilder::VisitCountOperation(CountOperation* expr) {
       }
 
       after = BuildIncrement(returns_original_input, expr);
-      input = Pop();
-
-      HInstruction* store;
-      if (!monomorphic || map->is_observed()) {
-        // If we don't know the monomorphic type, do a generic store.
-        CHECK_ALIVE(store = BuildStoreNamedGeneric(object, name, after));
-      } else {
-        Handle<JSFunction> setter;
-        Handle<JSObject> holder;
-        if (LookupSetter(map, name, &setter, &holder)) {
-          store = BuildCallSetter(object, after, map, setter, holder);
-        } else {
-          CHECK_ALIVE(store = BuildStoreNamedMonomorphic(object,
-                                                         name,
-                                                         after,
-                                                         map));
-        }
-      }
-      AddInstruction(store);
 
-      // 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 (returns_original_input) environment()->SetExpressionStackAt(1, input);
-      if (store->HasObservableSideEffects()) {
-        AddSimulate(expr->AssignmentId(), REMOVABLE_SIMULATE);
-      }
+      HValue* result = returns_original_input ? Pop() : after;
 
+      return BuildStoreNamed(prop, expr->id(), expr->position(),
+                             expr->AssignmentId(), prop, object, after, result);
     } else {
       // Keyed property.
       if (returns_original_input) Push(graph()->GetConstantUndefined());