Unify the Count Operation assignment with other assignments.

src/hydrogen.cc

 // Copyright 2012 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
@@ skipping 6347 matching lines @@
   instr->set_position(expr->position());
   return ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
 bool HOptimizedGraphBuilder::TryStorePolymorphicAsMonomorphic(
     int position,
     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
   // for all maps. Requires special map check on the set of all handled maps.
   if (types->length() > kMaxStorePolymorphism) return false;
 
   // TODO(verwaest): Merge the checking logic with the code in
   // TryLoadPolymorphicAsMonomorphic.
   LookupResult lookup(isolate());
   int count;
@@ skipping 27 matching lines @@
   if (count != types->length()) return false;
 
   // Everything matched; can use monomorphic store.
   BuildCheckNonSmi(object);
   AddInstruction(HCheckMaps::New(object, types, zone()));
   HInstruction* store;
   CHECK_ALIVE_OR_RETURN(
       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;
 }
 
 
 void HOptimizedGraphBuilder::HandlePolymorphicStoreNamedField(
     BailoutId id,
     int position,
     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;
   }
 
   // TODO(ager): We should recognize when the prototype chains for different
   // maps are identical. In that case we can avoid repeatedly generating the
   // same prototype map checks.
   int count = 0;
   HBasicBlock* join = NULL;
   for (int i = 0; i < types->length() && count < kMaxStorePolymorphism; ++i) {
     Handle<Map> map = types->at(i);
@@ skipping 10 matching lines @@
           new(zone()) HCompareMap(object, map, if_true, if_false);
       current_block()->Finish(compare);
 
       set_current_block(if_true);
       HInstruction* instr;
       CHECK_ALIVE(
           instr = BuildStoreNamedField(object, name, value, map, &lookup));
       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);
     }
   }
 
   // Finish up.  Unconditionally deoptimize if we've handled all the maps we
   // know about and do not want to handle ones we've never seen.  Otherwise
   // use a generic IC.
   if (count == types->length() && FLAG_deoptimize_uncommon_cases) {
     current_block()->FinishExitWithDeoptimization(HDeoptimize::kNoUses);
   } else {
     HInstruction* instr = BuildStoreNamedGeneric(object, name, value);
     instr->set_position(position);
     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
       // effect contexts (it is not materialized at expr->id() in the
       // unoptimized code).
       if (instr->HasObservableSideEffects()) {
         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());
 }
 
 
 void HOptimizedGraphBuilder::HandlePropertyAssignment(Assignment* expr) {
   Property* prop = expr->target()->AsProperty();
   ASSERT(prop != NULL);
   CHECK_ALIVE(VisitForValue(prop->obj()));
 
   if (prop->key()->IsPropertyName()) {
     // Named store.
     CHECK_ALIVE(VisitForValue(expr->value()));
     HValue* value = environment()->ExpressionStackAt(0);
     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()));
     CHECK_ALIVE(VisitForValue(expr->value()));
     HValue* value = environment()->ExpressionStackAt(0);
     HValue* key = environment()->ExpressionStackAt(1);
     HValue* object = environment()->ExpressionStackAt(2);
     bool has_side_effects = false;
     HandleKeyedElementAccess(object, key, value, expr, expr->AssignmentId(),
                              expr->position(),
@@ skipping 44 matching lines @@
   }
 }
 
 
 void HOptimizedGraphBuilder::BuildStoreNamed(Expression* expr,
                                              BailoutId id,
                                              int position,
                                              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());
 
   HInstruction* instr = NULL;
   SmallMapList* types = expr->GetReceiverTypes();
   bool monomorphic = expr->IsMonomorphic();
   Handle<Map> map;
   if (monomorphic) {
     map = types->first();
     if (map->is_dictionary_map()) monomorphic = false;
   }
   if (monomorphic) {
     Handle<JSFunction> setter;
     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;
       }
       Drop(2);
       AddInstruction(new(zone()) HPushArgument(object));
       AddInstruction(new(zone()) HPushArgument(value));
       instr = new(zone()) HCallConstantFunction(setter, 2);
     } else {
       Drop(2);
       CHECK_ALIVE(instr = BuildStoreNamedMonomorphic(object,
                                                      name,
                                                      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());
 }
 
 
 void HOptimizedGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
   Expression* target = expr->target();
   VariableProxy* proxy = target->AsVariableProxy();
   Property* prop = target->AsProperty();
   ASSERT(proxy == NULL || prop == NULL);
 
@@ skipping 111 matching lines @@
       HValue* right = Pop();
       HValue* left = Pop();
 
       HInstruction* instr = BuildBinaryOperation(operation, left, right);
       PushAndAdd(instr);
       if (instr->HasObservableSideEffects()) {
         AddSimulate(operation->id(), REMOVABLE_SIMULATE);
       }
 
       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()));
       CHECK_ALIVE(VisitForValue(prop->key()));
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
       bool has_side_effects = false;
       HValue* load = HandleKeyedElementAccess(
           obj, key, NULL, prop, prop->LoadId(), RelocInfo::kNoPosition,
@@ skipping 2471 matching lines @@
       } else if (types != NULL && types->length() > 1) {
         load = TryLoadPolymorphicAsMonomorphic(prop, object, types, name);
       }
       if (load == NULL) load = BuildLoadNamedGeneric(object, name, prop);
       PushAndAdd(load);
       if (load->HasObservableSideEffects()) {
         AddSimulate(prop->LoadId(), REMOVABLE_SIMULATE);
       }
 
       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);
+      HValue* result = returns_original_input ? Pop() : after;
 
-      // 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);
-      }
-
+      return BuildStoreNamed(prop, expr->id(), expr->position(),
+                             expr->AssignmentId(), prop, object, after, result);
     } else {
       // Keyed property.
       if (returns_original_input) Push(graph()->GetConstantUndefined());
 
       CHECK_ALIVE(VisitForValue(prop->obj()));
       CHECK_ALIVE(VisitForValue(prop->key()));
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
       bool has_side_effects = false;
@@ skipping 2288 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify(false);  // No full verify.
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal