Fix error in postfix ++ in Crankshaft.

src/hydrogen.cc

 // Copyright 2011 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 1766 matching lines @@
         : constant->CopyToRepresentation(to);
   }
 
   if (new_value == NULL) {
     new_value =
         new(zone()) HChange(value, value->representation(), to, is_truncating);
   }
 
   new_value->InsertBefore(next);
   use_value->SetOperandAt(use_index, new_value);
+
+  // The only purpose of a HForceRepresentation is to represent the value
+  // after the (possible) HChange instruction.  We make it disappear.
+  if (use_value->IsForceRepresentation()) {
+    use_value->DeleteAndReplaceWith(new_value);

[William Hesse, 2011/05/16 16:19:48]

We should be able to delete this copy.

[fschneider, 2011/05/17 11:03:15]

Yes, I think you can do it below and you only need to to DeleteAndReplaceWith
once.

+  }
 }
 
 
 void HGraph::InsertRepresentationChangesForValue(HValue* value) {
   Representation r = value->representation();
   if (r.IsNone()) return;
   if (value->HasNoUses()) return;
 
   for (HUseIterator it(value->uses()); !it.Done(); it.Advance()) {
     HValue* use_value = it.value();
     int use_index = it.index();
     Representation req = use_value->RequiredInputRepresentation(use_index);
     if (req.IsNone() || req.Equals(r)) continue;
     InsertRepresentationChangeForUse(value, use_value, use_index, req);
   }
   if (value->HasNoUses()) {
     ASSERT(value->IsConstant());
     value->DeleteAndReplaceWith(NULL);
   }
+
+  // The only purpose of a HForceRepresentation is to represent the value
+  // after the (possible) HChange instruction.  We make it disappear.
+  if (value->IsForceRepresentation()) {
+    value->DeleteAndReplaceWith(HForceRepresentation::cast(value)->value());
+  }
 }
 
 
 void HGraph::InsertRepresentationChanges() {
   HPhase phase("Insert representation changes", this);
 
 
   // Compute truncation flag for phis: Initially assume that all
   // int32-phis allow truncation and iteratively remove the ones that
   // are used in an operation that does not allow a truncating
@@ skipping 2876 matching lines @@
     materialize_true = NULL;
   }
 
   HBasicBlock* join =
     CreateJoin(materialize_false, materialize_true, expr->id());
   set_current_block(join);
   if (join != NULL) ast_context()->ReturnValue(Pop());
 }
 
 
-HInstruction* HGraphBuilder::BuildIncrement(HValue* value,
-                                            bool increment,
+HInstruction* HGraphBuilder::BuildIncrement(bool returns_original_input,
                                             CountOperation* expr) {
-  HConstant* delta = increment
-      ? graph_->GetConstant1()
-      : graph_->GetConstantMinus1();
-  HInstruction* instr = new(zone()) HAdd(value, delta);
+  // The input to the count operation is on top of the expression stack.
   TypeInfo info = oracle()->IncrementType(expr);
   Representation rep = ToRepresentation(info);
   if (rep.IsTagged()) {
     rep = Representation::Integer32();
   }
+
+  if (returns_original_input) {
+    // If we need to return ToNumber(input), we force a representation change.

[fschneider, 2011/05/17 11:03:15]

maybe explain why:

// to have get a HValue for the return value here and the actual conversion is
inserted later.

+    HInstruction* number_input = new(zone()) HForceRepresentation(Pop(), rep);
+    AddInstruction(number_input);
+    Push(number_input);
+  }
+
+  // The addition has no side effects, so we do not need
+  // to simulate the expression stack after this instruction.
+  // Any later failures deopt to the load of the input or earlier.
+  HConstant* delta = (expr->op() == Token::INC)
+      ? graph_->GetConstant1()
+      : graph_->GetConstantMinus1();
+  HInstruction* instr = new(zone()) HAdd(Top(), delta);
   TraceRepresentation(expr->op(), info, instr, rep);
   instr->AssumeRepresentation(rep);
+  AddInstruction(instr);
   return instr;
 }
 
 
 void HGraphBuilder::VisitCountOperation(CountOperation* expr) {
   ASSERT(!HasStackOverflow());
   ASSERT(current_block() != NULL);
   ASSERT(current_block()->HasPredecessor());
   Expression* target = expr->expression();
   VariableProxy* proxy = target->AsVariableProxy();
   Variable* var = proxy->AsVariable();
   Property* prop = target->AsProperty();
-  ASSERT(var == NULL || prop == NULL);
-  bool inc = expr->op() == Token::INC;
+  if (var == NULL && prop == NULL) {
+    return Bailout("invalid lhs in count operation");
+  }
+
+  // Match the full code generator stack by simulating an extra stack
+  // element for postfix operations in a non-effect context.  The return
+  // value is ToNumber(input).
+  bool returns_original_input =
+      expr->is_postfix() && !ast_context()->IsEffect();
+  HValue* input = NULL;  // The input to the count operation, after ToNumber.

[fschneider, 2011/05/17 11:03:15]

just say ToNumber(input)?

+  HValue* after = NULL;  // The result after incrementing or decrementing.
 
   if (var != NULL) {
+    // Argument of the count operation is a variable, not a property.
+    ASSERT(prop == NULL);
     CHECK_ALIVE(VisitForValue(target));
 
-    // Match the full code generator stack by simulating an extra stack
-    // element for postfix operations in a non-effect context.
-    bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
-    HValue* before = has_extra ? Top() : Pop();
-    HInstruction* after = BuildIncrement(before, inc, expr);
-    AddInstruction(after);
+    after = BuildIncrement(returns_original_input, expr);
+    input = returns_original_input ? Top() : Pop();
     Push(after);
 
     if (var->is_global()) {
       HandleGlobalVariableAssignment(var,
                                      after,
                                      expr->position(),
                                      expr->AssignmentId());
     } else if (var->IsStackAllocated()) {
       Bind(var, after);
     } else if (var->IsContextSlot()) {
       HValue* context = BuildContextChainWalk(var);
       int index = var->AsSlot()->index();
       HStoreContextSlot* instr =
           new(zone()) HStoreContextSlot(context, index, after);
       AddInstruction(instr);
       if (instr->HasSideEffects()) AddSimulate(expr->AssignmentId());
     } else {
       return Bailout("lookup variable in count operation");
     }
-    Drop(has_extra ? 2 : 1);
-    ast_context()->ReturnValue(expr->is_postfix() ? before : after);
 
-  } else if (prop != NULL) {
+  } else {
+    // Argument of the count operation is a property.
+    ASSERT(prop != NULL);
     prop->RecordTypeFeedback(oracle());
 
     if (prop->key()->IsPropertyName()) {
       // Named property.
-
-      // Match the full code generator stack by simulating an extra stack
-      // element for postfix operations in a non-effect context.
-      bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
-      if (has_extra) Push(graph_->GetConstantUndefined());
+      if (returns_original_input) Push(graph_->GetConstantUndefined());
 
       CHECK_ALIVE(VisitForValue(prop->obj()));
       HValue* obj = Top();
 
       HInstruction* load = NULL;
       if (prop->IsMonomorphic()) {
         Handle<String> name = prop->key()->AsLiteral()->AsPropertyName();
         Handle<Map> map = prop->GetReceiverTypes()->first();
         load = BuildLoadNamed(obj, prop, map, name);
       } else {
         load = BuildLoadNamedGeneric(obj, prop);
       }
       PushAndAdd(load);
       if (load->HasSideEffects()) AddSimulate(expr->CountId());
 
-      HValue* before = Pop();
-      // There is no deoptimization to after the increment, so we don't need
-      // to simulate the expression stack after this instruction.
-      HInstruction* after = BuildIncrement(before, inc, expr);
-      AddInstruction(after);
+      after = BuildIncrement(returns_original_input, expr);
+      input = Pop();
 
       HInstruction* store = BuildStoreNamed(obj, after, prop);
       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 (has_extra) environment()->SetExpressionStackAt(1, before);
+      if (returns_original_input) environment()->SetExpressionStackAt(1, input);
       if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
-      Drop(has_extra ? 2 : 1);
 
-      ast_context()->ReturnValue(expr->is_postfix() ? before : after);
-
-    } else {
+   } else {
       // Keyed property.
-
-      // Match the full code generator stack by simulate an extra stack element
-      // for postfix operations in a non-effect context.
-      bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
-      if (has_extra) Push(graph_->GetConstantUndefined());
+      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);
 
       HInstruction* load = BuildLoadKeyed(obj, key, prop);
       PushAndAdd(load);
       if (load->HasSideEffects()) AddSimulate(expr->CountId());
 
-      HValue* before = Pop();
-      // There is no deoptimization to after the increment, so we don't need
-      // to simulate the expression stack after this instruction.
-      HInstruction* after = BuildIncrement(before, inc, expr);
-      AddInstruction(after);
+      after = BuildIncrement(returns_original_input, expr);
+      input = Pop();
 
       expr->RecordTypeFeedback(oracle());
       HInstruction* store = BuildStoreKeyed(obj, key, after, expr);
       AddInstruction(store);
 
       // Drop the key from the bailout environment.  Overwrite the receiver
       // with the result of the operation, and the placeholder with the
       // original value if necessary.
       Drop(1);
       environment()->SetExpressionStackAt(0, after);
-      if (has_extra) environment()->SetExpressionStackAt(1, before);
+      if (returns_original_input) environment()->SetExpressionStackAt(1, input);
       if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
-      Drop(has_extra ? 2 : 1);
+    }
+  }
 
-      ast_context()->ReturnValue(expr->is_postfix() ? before : after);
-    }
-
-  } else {
-    return Bailout("invalid lhs in count operation");
-  }
+  Drop(returns_original_input ? 2 : 1);
+  ast_context()->ReturnValue(expr->is_postfix() ? input : after);
 }
 
 
 HCompareSymbolEq* HGraphBuilder::BuildSymbolCompare(HValue* left,
                                                     HValue* right,
                                                     Token::Value op) {
   ASSERT(op == Token::EQ || op == Token::EQ_STRICT);
   AddInstruction(new(zone()) HCheckNonSmi(left));
   AddInstruction(HCheckInstanceType::NewIsSymbol(left));
   AddInstruction(new(zone()) HCheckNonSmi(right));
@@ skipping 1276 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify();
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal