Collect type feedback for fields.

runtime/vm/compiler.cc

Index: runtime/vm/compiler.cc
diff --git a/runtime/vm/compiler.cc b/runtime/vm/compiler.cc
index bc4444030a93d7b96b96c1b496a5f026b73960fc..e0e7ae3f5ef710ae62b4c4afee6562f515879f46 100644
--- a/runtime/vm/compiler.cc
+++ b/runtime/vm/compiler.cc
@@ -112,6 +112,38 @@ static void InstallUnoptimizedCode(const Function& function) {
 }
 
 
+static ZoneGrowableArray<Field*>* CollectGuardedFieldLoads(
+  FlowGraph* flow_graph) {
+  ZoneGrowableArray<Field*>* result =
+    new ZoneGrowableArray<Field*>(10);
+
+  for (intptr_t i = 1; i < flow_graph->reverse_postorder().length(); i++) {
+    BlockEntryInstr* entry = flow_graph->reverse_postorder()[i];
+    for (ForwardInstructionIterator it(entry); !it.Done(); it.Advance()) {
+      LoadFieldInstr* load_field = it.Current()->AsLoadField();
+      if (load_field == NULL || load_field->field() == NULL) continue;
+
+      Field* field = load_field->field();
+      if (field->guarded_cid() == kDynamicCid) continue;

[Kevin Millikin (Google), 2013/03/12 12:14:56]

It's slightly nicer to write:

if (load_field == NULL) continue;
Field* field = load_field->field();
if ((field == NULL) || (field->guarded_cid() == kDynamicCid)) continue;

[Vyacheslav Egorov (Google), 2013/03/12 16:54:40]

Done.

+
+      bool found = false;
+      for (intptr_t j = 0; j < result->length(); j++) {
+        if ((*result)[j]->raw() == field->raw()) {
+          found = true;
+          break;
+        }
+      }
+
+      if (!found) {

[Kevin Millikin (Google), 2013/03/12 12:14:56]

if (!found) result->Add(field);

to match the style you used earlier.

[Vyacheslav Egorov (Google), 2013/03/12 16:54:40]

Aligned all conditions here to use the same line breaking style.

+        result->Add(field);
+      }
+    }
+  }
+
+  return result;
+}
+
+
 // Return false if bailed out.
 static bool CompileParsedFunctionHelper(const ParsedFunction& parsed_function,
                                         bool optimized) {
@@ -167,6 +199,8 @@ static bool CompileParsedFunctionHelper(const ParsedFunction& parsed_function,
       FlowGraphPrinter::PrintGraph("Before Optimizations", flow_graph);
     }
 
+    ZoneGrowableArray<Field*>* guarded_fields = NULL;
+
     if (optimized) {
       TimerScope timer(FLAG_compiler_stats,
                        &CompilerStats::graphoptimizer_timer,
@@ -193,6 +227,8 @@ static bool CompileParsedFunctionHelper(const ParsedFunction& parsed_function,
         DEBUG_ASSERT(flow_graph->VerifyUseLists());
       }
 
+      guarded_fields = CollectGuardedFieldLoads(flow_graph);

[Kevin Millikin (Google), 2013/03/12 12:14:56]

CollectGuardedFieldLoads might be better as a member function on FlowGraph.

Does it need to be here in the pipeline?  I would think as late as possible
would be better.

[Vyacheslav Egorov (Google), 2013/03/12 16:54:40]

Moved to FlowGraph.
Doing it too late might lead to a situtation where load field disappeared from
the graph after it was used for optimization. 

Canonical example would be:

if (this.foo /* not null field foo */ == null) {
  return;
}

load of x.foo should completely disappear (it does not disappear now, but it can
in the future).

+
       // Propagate types and eliminate more type tests.
       if (FLAG_propagate_types) {
         FlowGraphTypePropagator propagator(flow_graph);
@@ -259,6 +295,11 @@ static bool CompileParsedFunctionHelper(const ParsedFunction& parsed_function,
       }
 
       // The final canonicalization pass before the code generation.
+      if (FLAG_propagate_types) {

[Kevin Millikin (Google), 2013/03/12 12:14:56]

I think this (and also the other site) needs a comment about what propagate
types will optimize and why we chose this position in the pipeline.  Otherwise
we won't be able to figure it out (and why it's duplicated) in a month.

+        FlowGraphTypePropagator propagator(flow_graph);
+        propagator.Propagate();
+        DEBUG_ASSERT(flow_graph->VerifyUseLists());
+      }
       optimizer.Canonicalize();
       DEBUG_ASSERT(flow_graph->VerifyUseLists());
 
@@ -296,6 +337,7 @@ static bool CompileParsedFunctionHelper(const ParsedFunction& parsed_function,
       graph_compiler.FinalizeExceptionHandlers(code);
       graph_compiler.FinalizeComments(code);
       graph_compiler.FinalizeStaticCallTargetsTable(code);
+
       if (optimized) {
         CodePatcher::PatchEntry(Code::Handle(function.CurrentCode()));
         function.SetCode(code);
@@ -303,6 +345,15 @@ static bool CompileParsedFunctionHelper(const ParsedFunction& parsed_function,
           OS::Print("--> patching entry %#"Px"\n",
                     Code::Handle(function.unoptimized_code()).EntryPoint());
         }
+
+        const Array& guarded_fields_arr = Array::Handle(
+            Array::New(guarded_fields->length(), Heap::kOld));

[Kevin Millikin (Google), 2013/03/12 12:14:56]

To fit the pattern, this should be a FinalizeGuardedFieldLoads or some such
member function on the compiler class.

[Vyacheslav Egorov (Google), 2013/03/12 16:54:40]

It does not set anything on the code itself now, so I would prefer to leave it
here.

+        for (intptr_t i = 0; i < guarded_fields->length(); i++) {
+          const Field& field = *(*guarded_fields)[i];
+          guarded_fields_arr.SetAt(i, field);
+          field.RegisterDependentCode(code);
+        }
+        code.set_guarded_fields(guarded_fields_arr);
       } else {
         function.set_unoptimized_code(code);
         function.SetCode(code);