Fix wrong order of execution in compound assignment nodes (located by Kevin).

runtime/vm/parser.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/parser.h"
 
 #include "vm/bigint_operations.h"
 #include "vm/class_finalizer.h"
 #include "vm/compiler.h"
 #include "vm/compiler_stats.h"
@@ skipping 5901 matching lines @@
     intptr_t token_index = node->token_index();
     node = NULL;  // Do not use it.
     // The array object access may not have side effects.
     // First, evaluate the array object expression if it might have side
     // effects.
     if (!IsLocalOrLiteralNode(load_indexed->array())) {
       LocalVariable* temp =
           CreateTempConstVariable(token_index, token_id, "lia");
       AstNode* save =
           new StoreLocalNode(token_index, *temp, load_indexed->array());
-      current_block_->statements->Add(save);
-      AstNode* load = new LoadLocalNode(token_index, *temp);
       load_indexed = new LoadIndexedNode(token_index,
-                                         load,
+                                         save,
                                          load_indexed->index_expr());
     }
     // Second, evaluate the index expression and store in a temporary
     // variable if it might have side effects.
     if (!IsLocalOrLiteralNode(load_indexed->index_expr())) {
       LocalVariable* temp =
           CreateTempConstVariable(token_index, token_id, "lix");
       AstNode* save =
           new StoreLocalNode(token_index, *temp, load_indexed->index_expr());
-      current_block_->statements->Add(save);
-      AstNode* load = new LoadLocalNode(token_index, *temp);
       load_indexed = new LoadIndexedNode(token_index,
                                          load_indexed->array(),
-                                         load);
+                                         save);
     }
     return load_indexed;
   }
   if (node->IsInstanceGetterNode()) {
     InstanceGetterNode* getter = node->AsInstanceGetterNode();
     intptr_t token_index = node->token_index();
     intptr_t token_id = node->id();
     node = NULL;  // Do not use it.
     if (!IsLocalOrLiteralNode(getter->receiver())) {
       LocalVariable* temp =
           CreateTempConstVariable(token_index, token_id, "igr");
       AstNode* save =
           new StoreLocalNode(token_index, *temp, getter->receiver());
-      current_block_->statements->Add(save);
-      AstNode* load = new LoadLocalNode(token_index, *temp);
-      getter = new InstanceGetterNode(token_index, load, getter->field_name());
+      getter = new InstanceGetterNode(token_index, save, getter->field_name());
     }
     return getter;
   }
   return node;
 }
 
 
 // TODO(srdjan): Implement other optimizations.
 AstNode* Parser::OptimizeBinaryOpNode(intptr_t op_pos,
                                       Token::Kind binary_op,
@@ skipping 64 matching lines @@
   if (expr->IsLiteralNode()) {
     return expr;
   }
   if (expr->EvalConstExpr() == NULL) {
     ErrorMsg(expr_pos, "expression must be a compile time constant");
   }
   return new LiteralNode(expr_pos, EvaluateConstExpr(expr));
 }
 
 
+// A compound assignment consists of a store and a load part. In order
+// to control inputs with potential side effects, the store part stores any
+// side effect creating inputs into locals.
+// Here we convert the inputs if the load local from the StoreLocals to
+// LoadLocals.
+static AstNode* ModifyCompoundAssignmentLoad(AstNode* node) {
+  if (node->IsLoadIndexedNode()) {
+    LoadIndexedNode* load_indexed = node->AsLoadIndexedNode();
+    if (load_indexed->array()->IsStoreLocalNode()) {
+      StoreLocalNode* store = load_indexed->array()->AsStoreLocalNode();
+      LoadLocalNode* load = new LoadLocalNode(store->token_index(),
+                                              store->local());
+      load_indexed = new LoadIndexedNode(load_indexed->token_index(),
+                                         load,
+                                         load_indexed->index_expr());
+    }
+    if (load_indexed->index_expr()->IsStoreLocalNode()) {
+      StoreLocalNode* store = load_indexed->index_expr()->AsStoreLocalNode();
+      LoadLocalNode* load = new LoadLocalNode(store->token_index(),
+                                              store->local());
+      load_indexed = new LoadIndexedNode(load_indexed->token_index(),
+                                         load_indexed->array(),
+                                         load);
+    }
+    return load_indexed;
+  }
+  if (node->IsInstanceGetterNode()) {
+    InstanceGetterNode* getter = node->AsInstanceGetterNode();
+    if (getter->receiver()->IsStoreLocalNode()) {
+      StoreLocalNode* store = getter->receiver()->AsStoreLocalNode();
+      LoadLocalNode* load = new LoadLocalNode(store->token_index(),
+                                              store->local());
+      getter = new InstanceGetterNode(getter->token_index(),
+                                      load,
+                                      getter->field_name());
+    }
+    return getter;
+  }
+  return node;
+}
+
+
 AstNode* Parser::ParseExpr(bool require_compiletime_const) {
   TRACE_PARSER("ParseExpr");
   const intptr_t expr_pos = token_index_;
   AstNode* expr = ParseConditionalExpr();
   if (!Token::IsAssignmentOperator(CurrentToken())) {
     if (require_compiletime_const) {
       expr = FoldConstExpr(expr_pos, expr);
     }
     return expr;
   }
   // Assignment expressions.
   Token::Kind assignment_op = CurrentToken();
   const intptr_t assignment_pos = token_index_;
   ConsumeToken();
   const intptr_t right_expr_pos = token_index_;
   if (require_compiletime_const && (assignment_op != Token::kASSIGN)) {
     ErrorMsg(right_expr_pos, "expression must be a compile time constant");
   }
   AstNode* right_expr = ParseExpr(require_compiletime_const);
+  AstNode* left_expr = expr;
   if (assignment_op != Token::kASSIGN) {
-    expr = AsSideEffectFreeNode(expr);
+    // Compound assignment: store inputs with side effects into temp. locals.
+    left_expr = AsSideEffectFreeNode(expr);
+    expr = ModifyCompoundAssignmentLoad(left_expr);

[Kevin Millikin (Google), 2012/05/01 08:40:13]

Cute.

ModifyCompoundAssignmentLoad is only safe if the assignments found in the
subexpressions of left_expr are the ones introduced by AsSideEffectFreeNode, and
not ones written by the programmer.

We could force that by combining the two functions with a combined API like:

left_expr = Modify...(&expr)

or else

expr = Modify...(&left_expr).

[srdjan, 2012/05/01 16:27:55]

Good point, will combine them.

   }
   right_expr =
       ExpandAssignableOp(assignment_pos, assignment_op, expr, right_expr);
-  AstNode* assign_expr = expr->MakeAssignmentNode(right_expr);
+  AstNode* assign_expr = left_expr->MakeAssignmentNode(right_expr);
   if (assign_expr == NULL) {
     ErrorMsg(assignment_pos,
              "left hand side of '%s' is not assignable",
              Token::Str(assignment_op));
   }
   return assign_expr;
 }
 
 
 LiteralNode* Parser::ParseConstExpr() {
@@ skipping 209 matching lines @@
                                           const String& field_name) {
   TRACE_PARSER("ParseInstanceFieldAccess");
   AstNode* access = NULL;
   const intptr_t call_pos = token_index_;
   if (Token::IsAssignmentOperator(CurrentToken())) {
     Token::Kind assignment_op = CurrentToken();
     ConsumeToken();
     AstNode* value = ParseExpr(kAllowConst);
     AstNode* load_access =
         new InstanceGetterNode(call_pos, receiver, field_name);
+    AstNode* left_load_access = load_access;
     if (assignment_op != Token::kASSIGN) {
-      load_access = AsSideEffectFreeNode(load_access);
+      // Compound assignment: store inputs with side effects into temp. locals.
+      left_load_access = AsSideEffectFreeNode(load_access);
+      load_access = ModifyCompoundAssignmentLoad(left_load_access);
     }
     value = ExpandAssignableOp(call_pos, assignment_op, load_access, value);
-    access = load_access->MakeAssignmentNode(value);
+    access = left_load_access->MakeAssignmentNode(value);
   } else {
     access = CallGetter(call_pos, receiver, field_name);
   }
   return access;
 }
 
 
 AstNode* Parser::GenerateStaticFieldLookup(const Field& field,
                                            intptr_t ident_pos) {
   // If the static field has an initializer, initialize the field at compile
@@ skipping 2104 matching lines @@
 void Parser::SkipQualIdent() {
   ASSERT(IsIdentifier());
   ConsumeToken();
   if (CurrentToken() == Token::kPERIOD) {
     ConsumeToken();  // Consume the kPERIOD token.
     ExpectIdentifier("identifier expected after '.'");
   }
 }
 
 }  // namespace dart