Fix a bug in allocation sinking and load elimination.

runtime/vm/flow_graph_optimizer.cc

 // Copyright (c) 2013, 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/flow_graph_optimizer.h"
 
 #include "vm/bit_vector.h"
 #include "vm/cha.h"
 #include "vm/flow_graph_builder.h"
 #include "vm/flow_graph_compiler.h"
@@ skipping 4029 matching lines @@
       if (comma) {
         OS::Print(", ");
       }
       OS::Print("%s", places_[it.Current()]->ToCString());
       comma = true;
     }
   }
 
   const PhiPlaceMoves* phi_moves() const { return phi_moves_; }
 
+  // Returns true if the result of AllocateObject can be aliased by some
+  // other SSA variable and false otherwise. Currently simply checks if
+  // this value is stored in a field, escapes to another function or
+  // participates in a phi.
+  static bool CanBeAliased(AllocateObjectInstr* alloc) {
+    if (alloc->identity() == AllocateObjectInstr::kUnknown) {
+      bool escapes = false;
+      for (Value* use = alloc->input_use_list();
+           use != NULL;
+           use = use->next_use()) {
+        Instruction* instr = use->instruction();
+        if (instr->IsPushArgument() ||
+            (instr->IsStoreVMField() && (use->use_index() != 1)) ||

[Vyacheslav Egorov (Google), 2013/06/27 13:15:45]

Good catch!

+            (instr->IsStoreInstanceField() && (use->use_index() != 0)) ||
+            (instr->IsStoreStaticField()) ||
+            (instr->IsPhi())) {
+          escapes = true;
+          break;
+        }
+      }
+
+      alloc->set_identity(escapes ? AllocateObjectInstr::kAliased
+                                  : AllocateObjectInstr::kNotAliased);
+    }
+
+    return alloc->identity() != AllocateObjectInstr::kNotAliased;
+  }
+
  private:
   // Get id assigned to the given field. Assign a new id if the field is seen
   // for the first time.
   intptr_t GetFieldId(intptr_t instance_id, const Field& field) {
     intptr_t id = field_ids_.Lookup(FieldIdPair::Key(instance_id, &field));
     if (id == 0) {
       id = ++max_field_id_;
       field_ids_.Insert(FieldIdPair(FieldIdPair::Key(instance_id, &field), id));
     }
     return id;
@@ skipping 27 matching lines @@
 
     return GetFieldId(instance_id, field);
   }
 
   // Get or create an identifier for a static field.
   intptr_t GetStaticFieldId(const Field& field) {
     ASSERT(field.is_static());
     return GetFieldId(kAnyInstance, field);
   }
 
-  // Returns true if the result of AllocateObject can be aliased by some
-  // other SSA variable and false otherwise. Currently simply checks if
-  // this value is stored in a field, escapes to another function or
-  // participates in a phi.
-  bool CanBeAliased(AllocateObjectInstr* alloc) {
-    if (alloc->identity() == AllocateObjectInstr::kUnknown) {
-      bool escapes = false;
-      for (Value* use = alloc->input_use_list();
-           use != NULL;
-           use = use->next_use()) {
-        Instruction* instr = use->instruction();
-        if (instr->IsPushArgument() ||
-            (instr->IsStoreVMField() && (use->use_index() != 0)) ||
-            (instr->IsStoreInstanceField() && (use->use_index() != 0)) ||
-            (instr->IsStoreStaticField()) ||
-            (instr->IsPhi())) {
-          escapes = true;
-          break;
-        }
-      }
-
-      alloc->set_identity(escapes ? AllocateObjectInstr::kAliased
-                                  : AllocateObjectInstr::kNotAliased);
-    }
-
-    return alloc->identity() != AllocateObjectInstr::kNotAliased;
-  }
-
   // Returns true if the given load is unaffected by external side-effects.
   // This essentially means that no stores to the same location can
   // occur in other functions.
   bool IsIndependentFromEffects(Place* place) {
     if (place->IsFinalField()) {
       // Note that we can't use LoadField's is_immutable attribute here because
       // some VM-fields (those that have no corresponding Field object and
       // accessed through offset alone) can share offset but have different
       // immutability properties.
       // One example is the length property of growable and fixed size list. If
@@ skipping 365 matching lines @@
           gen->RemoveAll(aliased_set_->aliased_by_effects());
           continue;
         }
 
         Definition* defn = instr->AsDefinition();
         if (defn == NULL) {
           continue;
         }
 
         // For object allocation forward initial values of the fields to
-        // subsequent loads.
-        // For simplicity we ignore escaping objects and objects that have
-        // type arguments.
+        // subsequent loads. For simplicity we ignore escaping objects.
+        //
         // The reason to ignore escaping objects is that final fields are
         // initialized in constructor that potentially can be not inlined into
         // the function that we are currently optimizing. However at the same
         // time we assume that values of the final fields can be forwarded
         // across side-effects. If we add 'null' as known values for these
         // fields here we will incorrectly propagate this null across
         // constructor invocation.
         // TODO(vegorov): record null-values at least for not final fields of
         // escaping object.
         AllocateObjectInstr* alloc = instr->AsAllocateObject();
         if ((alloc != NULL) &&
-            (alloc->identity() == AllocateObjectInstr::kNotAliased) &&
+            !AliasedSet::CanBeAliased(alloc) &&
             HasSimpleTypeArguments(alloc)) {
           for (Value* use = alloc->input_use_list();
                use != NULL;
                use = use->next_use()) {
             // Look for all immediate loads from this object.
             if (use->use_index() != 0) {
               continue;
             }
 
             LoadFieldInstr* load = use->instruction()->AsLoadField();
@@ skipping 2422 matching lines @@
        block_it.Advance()) {
     BlockEntryInstr* block = block_it.Current();
     for (ForwardInstructionIterator it(block); !it.Done(); it.Advance()) {
       AllocateObjectInstr* alloc = it.Current()->AsAllocateObject();
       if ((alloc != NULL) && IsAllocationSinkingCandidate(alloc)) {
         if (FLAG_trace_optimization) {
           OS::Print("discovered allocation sinking candidate: v%"Pd"\n",
                     alloc->ssa_temp_index());
         }
 
-        if (alloc->identity() == AllocateObjectInstr::kAliased) {
-          // Allocation might have been classified as aliased earlier due to
-          // some operations that are now eliminated.
-          alloc->set_identity(AllocateObjectInstr::kNotAliased);
-        }
+        // All sinking candidate are known to be not aliased.
+        alloc->set_identity(AllocateObjectInstr::kNotAliased);
 
         candidates.Add(alloc);
       }
     }
   }
 
   // Insert MaterializeObject instructions that will describe the state of the
   // object at all deoptimization points. Each inserted materialization looks
   // like this (where v_0 is allocation that we are going to eliminate):
   //   v_1     <- LoadField(v_0, field_1)
@@ skipping 157 matching lines @@
 
   // Insert materializations at environment uses.
   const Class& cls = Class::Handle(alloc->constructor().Owner());
   for (intptr_t i = 0; i < exits.length(); i++) {
     CreateMaterializationAt(exits[i], alloc, cls, *fields);
   }
 }
 
 
 }  // namespace dart