Fix bug in field type tracking and polymorphic inlining.

When inlining implicit getters via the polymorphic inliner
(and not through the flow graph optimizer) the fields loaded
must be added to the list of guarded fields that trigger
deoptimization when a store violated the field type guard.

Also, this CL avoids adding fields to the list from inlining
candidates that do not get inlined after all. Previously, the
optimizer pass on the callee graph would add guarded fields
even if the final graph does not get inlined.

TEST=tests/language/vm/optimized_guarded_field_test.dart
R=kmillikin@google.com

Review URL: https://codereview.chromium.org//24096018

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@27655 260f80e4-7a28-3924-810f-c04153c831b5

runtime/vm/compiler.cc

@@ -271,6 +271,7 @@ static bool CompileParsedFunctionHelper(ParsedFunction* parsed_function,
     isolate->set_long_jump_base(&bailout_jump);
     if (setjmp(*bailout_jump.Set()) == 0) {
       FlowGraph* flow_graph = NULL;
+      GrowableArray<const Field*> guarded_fields;
       // TimerScope needs an isolate to be properly terminated in case of a
       // LongJump.
       {
@@ -293,6 +294,7 @@ static bool CompileParsedFunctionHelper(ParsedFunction* parsed_function,
         FlowGraphBuilder builder(parsed_function,
                                  ic_data_array,
                                  NULL,  // NULL = not inlining.
+                                 &guarded_fields,
                                  osr_id);
         flow_graph = builder.BuildGraph();
       }
@@ -326,13 +328,12 @@ static bool CompileParsedFunctionHelper(ParsedFunction* parsed_function,
       // Collect all instance fields that are loaded in the graph and
       // have non-generic type feedback attached to them that can
       // potentially affect optimizations.
-      GrowableArray<const Field*> guarded_fields(10);
       if (optimized) {
         TimerScope timer(FLAG_compiler_stats,
                          &CompilerStats::graphoptimizer_timer,
                          isolate);
 
-        FlowGraphOptimizer optimizer(flow_graph, &guarded_fields);
+        FlowGraphOptimizer optimizer(flow_graph);
         optimizer.ApplyICData();
         DEBUG_ASSERT(flow_graph->VerifyUseLists());
 
@@ -358,7 +359,7 @@ static bool CompileParsedFunctionHelper(ParsedFunction* parsed_function,
           optimizer.ApplyClassIds();
           DEBUG_ASSERT(flow_graph->VerifyUseLists());
 
-          FlowGraphInliner inliner(flow_graph, &guarded_fields);
+          FlowGraphInliner inliner(flow_graph);
           inliner.Inline();
           // Use lists are maintained and validated by the inliner.
           DEBUG_ASSERT(flow_graph->VerifyUseLists());

runtime/vm/flow_graph.cc

@@ -24,6 +24,7 @@ FlowGraph::FlowGraph(const FlowGraphBuilder& builder,
   : parent_(),
     current_ssa_temp_index_(0),
     max_block_id_(max_block_id),
+    builder_(builder),
     parsed_function_(*builder.parsed_function()),
     num_copied_params_(builder.num_copied_params()),
     num_non_copied_params_(builder.num_non_copied_params()),

runtime/vm/flow_graph.h

@@ -48,6 +48,10 @@ class FlowGraph : public ZoneAllocated {
             GraphEntryInstr* graph_entry,
             intptr_t max_block_id);
 
+  const FlowGraphBuilder& builder() const {
+    return builder_;
+  }
+
   // Function properties.
   const ParsedFunction& parsed_function() const {
     return parsed_function_;
@@ -246,6 +250,7 @@ class FlowGraph : public ZoneAllocated {
   intptr_t max_block_id_;
 
   // Flow graph fields.
+  const FlowGraphBuilder& builder_;
   const ParsedFunction& parsed_function_;
   const intptr_t num_copied_params_;
   const intptr_t num_non_copied_params_;

runtime/vm/flow_graph_builder.cc

@@ -45,6 +45,7 @@ static const String& PrivateCoreLibName(const String& str) {
 FlowGraphBuilder::FlowGraphBuilder(ParsedFunction* parsed_function,
                                    const Array& ic_data_array,
                                    InlineExitCollector* exit_collector,
+                                   GrowableArray<const Field*>* guarded_fields,
                                    intptr_t osr_id)
   : parsed_function_(parsed_function),
     ic_data_array_(ic_data_array),
@@ -55,6 +56,7 @@ FlowGraphBuilder::FlowGraphBuilder(ParsedFunction* parsed_function,
         : 0),
     num_stack_locals_(parsed_function->num_stack_locals()),
     exit_collector_(exit_collector),
+    guarded_fields_(guarded_fields),
     last_used_block_id_(0),  // 0 is used for the graph entry.
     context_level_(0),
     try_index_(CatchClauseNode::kInvalidTryIndex),
@@ -70,6 +72,20 @@ void FlowGraphBuilder::AddCatchEntry(CatchBlockEntryInstr* entry) {
 }
 
 
+void FlowGraphBuilder::AddToGuardedFields(const Field& field) const {
+  if ((field.guarded_cid() == kDynamicCid) ||
+      (field.guarded_cid() == kIllegalCid)) {
+    return;
+  }
+  for (intptr_t j = 0; j < guarded_fields_->length(); j++) {
+    if ((*guarded_fields_)[j]->raw() == field.raw()) {
+      return;
+    }
+  }
+  guarded_fields_->Add(&field);
+}
+
+
 void InlineExitCollector::PrepareGraphs(FlowGraph* callee_graph) {
   ASSERT(callee_graph->graph_entry()->SuccessorCount() == 1);
   ASSERT(callee_graph->max_block_id() > caller_graph_->max_block_id());
@@ -3039,6 +3055,17 @@ void EffectGraphVisitor::VisitLoadInstanceFieldNode(
       node->field().Offset(),
       AbstractType::ZoneHandle(node->field().type()));
   load->set_field(&node->field());
+  if (owner()->exit_collector() != NULL) {
+    // While inlining into an optimized function, the field has
+    // to be added to the list of guarded fields of the caller.
+    if (node->field().guarded_cid() != kIllegalCid) {
+      if (!node->field().is_nullable() ||
+          (node->field().guarded_cid() == kNullCid)) {
+        load->set_result_cid(node->field().guarded_cid());
+      }
+      owner()->AddToGuardedFields(node->field());
+    }
+  }
   ReturnDefinition(load);
 }
 

runtime/vm/flow_graph_builder.h

@@ -105,6 +105,7 @@ class FlowGraphBuilder: public ValueObject {
   FlowGraphBuilder(ParsedFunction* parsed_function,
                    const Array& ic_data_array,
                    InlineExitCollector* exit_collector,
+                   GrowableArray<const Field*>* guarded_fields,
                    intptr_t osr_id);
 
   FlowGraph* BuildGraph();
@@ -147,6 +148,12 @@ class FlowGraphBuilder: public ValueObject {
   bool IsInlining() const { return (exit_collector_ != NULL); }
   InlineExitCollector* exit_collector() const { return exit_collector_; }
 
+  GrowableArray<const Field*>* guarded_fields() const {
+    return guarded_fields_;
+  }
+
+  void AddToGuardedFields(const Field& field) const;
+
   intptr_t args_pushed() const { return args_pushed_; }
   void add_args_pushed(intptr_t n) { args_pushed_ += n; }
 
@@ -169,6 +176,7 @@ class FlowGraphBuilder: public ValueObject {
   const intptr_t num_non_copied_params_;
   const intptr_t num_stack_locals_;  // Does not include any parameters.
   InlineExitCollector* const exit_collector_;
+  GrowableArray<const Field*>* guarded_fields_;
 
   intptr_t last_used_block_id_;
   intptr_t context_level_;

runtime/vm/flow_graph_inliner.cc

@@ -382,8 +382,7 @@ class PolymorphicInliner : public ValueObject {
 
 class CallSiteInliner : public ValueObject {
  public:
-  CallSiteInliner(FlowGraph* flow_graph,
-                  GrowableArray<const Field*>* guarded_fields)
+  explicit CallSiteInliner(FlowGraph* flow_graph)
       : caller_graph_(flow_graph),
         inlined_(false),
         initial_size_(flow_graph->InstructionCount()),
@@ -391,8 +390,7 @@ class CallSiteInliner : public ValueObject {
         inlining_depth_(1),
         collected_call_sites_(NULL),
         inlining_call_sites_(NULL),
-        function_cache_(),
-        guarded_fields_(guarded_fields) { }
+        function_cache_() { }
 
   FlowGraph* caller_graph() const { return caller_graph_; }
 
@@ -544,9 +542,11 @@ class CallSiteInliner : public ValueObject {
       // Build the callee graph.
       InlineExitCollector* exit_collector =
           new InlineExitCollector(caller_graph_, call);
+      GrowableArray<const Field*> inlined_guarded_fields;
       FlowGraphBuilder builder(parsed_function,
                                ic_data_array,
                                exit_collector,
+                               &inlined_guarded_fields,
                                Isolate::kNoDeoptId);
       builder.SetInitialBlockId(caller_graph_->max_block_id());
       FlowGraph* callee_graph;
@@ -607,7 +607,7 @@ class CallSiteInliner : public ValueObject {
                          &CompilerStats::graphinliner_opt_timer,
                          isolate);
         // TODO(zerny): Do more optimization passes on the callee graph.
-        FlowGraphOptimizer optimizer(callee_graph, guarded_fields_);
+        FlowGraphOptimizer optimizer(callee_graph);
         optimizer.ApplyICData();
         DEBUG_ASSERT(callee_graph->VerifyUseLists());
       }
@@ -668,6 +668,13 @@ class CallSiteInliner : public ValueObject {
       call_data->callee_graph = callee_graph;
       call_data->parameter_stubs = param_stubs;
       call_data->exit_collector = exit_collector;
+
+      // When inlined, we add the guarded fields of the callee to the caller's
+      // list of guarded fields.
+      for (intptr_t i = 0; i < inlined_guarded_fields.length(); ++i) {
+        caller_graph_->builder().AddToGuardedFields(*inlined_guarded_fields[i]);
+      }
+
       TRACE_INLINING(OS::Print("     Success\n"));
       return true;
     } else {
@@ -998,7 +1005,6 @@ class CallSiteInliner : public ValueObject {
   CallSites* collected_call_sites_;
   CallSites* inlining_call_sites_;
   GrowableArray<ParsedFunction*> function_cache_;
-  GrowableArray<const Field*>* guarded_fields_;
 
   DISALLOW_COPY_AND_ASSIGN(CallSiteInliner);
 };
@@ -1153,8 +1159,7 @@ static Instruction* AppendInstruction(Instruction* first,
 
 
 bool PolymorphicInliner::TryInlineRecognizedMethod(const Function& target) {
-  FlowGraphOptimizer optimizer(owner_->caller_graph(),
-                               NULL);  // No guarded fields needed.
+  FlowGraphOptimizer optimizer(owner_->caller_graph());
   TargetEntryInstr* entry;
   Definition* last;
   if (optimizer.TryInlineRecognizedMethod(target,
@@ -1475,7 +1480,7 @@ void FlowGraphInliner::Inline() {
     printer.PrintBlocks();
   }
 
-  CallSiteInliner inliner(flow_graph_, guarded_fields_);
+  CallSiteInliner inliner(flow_graph_);
   inliner.InlineCalls();
 
   if (inliner.inlined()) {

runtime/vm/flow_graph_inliner.h

@@ -15,9 +15,7 @@ template <typename T> class GrowableArray;
 
 class FlowGraphInliner : ValueObject {
  public:
-  FlowGraphInliner(FlowGraph* flow_graph,
-                   GrowableArray<const Field*>* guarded_fields)
-      : flow_graph_(flow_graph), guarded_fields_(guarded_fields) {}
+  explicit FlowGraphInliner(FlowGraph* flow_graph) : flow_graph_(flow_graph) { }
 
   // The flow graph is destructively updated upon inlining.
   void Inline();
@@ -26,7 +24,6 @@ class FlowGraphInliner : ValueObject {
 
  private:
   FlowGraph* flow_graph_;
-  GrowableArray<const Field*>* guarded_fields_;
 
   DISALLOW_COPY_AND_ASSIGN(FlowGraphInliner);
 };

runtime/vm/flow_graph_optimizer.cc

@@ -1570,20 +1570,6 @@ bool FlowGraphOptimizer::MethodExtractorNeedsClassCheck(
 }
 
 
-void FlowGraphOptimizer::AddToGuardedFields(const Field& field) {
-  if ((field.guarded_cid() == kDynamicCid) ||
-      (field.guarded_cid() == kIllegalCid)) {
-    return;
-  }
-  for (intptr_t j = 0; j < guarded_fields_->length(); j++) {
-    if ((*guarded_fields_)[j]->raw() == field.raw()) {
-      return;
-    }
-  }
-  guarded_fields_->Add(&field);
-}
-
-
 void FlowGraphOptimizer::InlineImplicitInstanceGetter(InstanceCallInstr* call) {
   ASSERT(call->HasICData());
   const ICData& ic_data = *call->ic_data();
@@ -1610,7 +1596,7 @@ void FlowGraphOptimizer::InlineImplicitInstanceGetter(InstanceCallInstr* call) {
     if (!field.is_nullable() || (field.guarded_cid() == kNullCid)) {
       load->set_result_cid(field.guarded_cid());
     }
-    AddToGuardedFields(field);
+    flow_graph_->builder().AddToGuardedFields(field);
   }
 
   // Discard the environment from the original instruction because the load

runtime/vm/flow_graph_optimizer.h

@@ -16,11 +16,9 @@ class ParsedFunction;
 
 class FlowGraphOptimizer : public FlowGraphVisitor {
  public:
-  FlowGraphOptimizer(FlowGraph* flow_graph,
-                     GrowableArray<const Field*>* guarded_fields)
+  explicit FlowGraphOptimizer(FlowGraph* flow_graph)
       : FlowGraphVisitor(flow_graph->reverse_postorder()),
-        flow_graph_(flow_graph),
-        guarded_fields_(guarded_fields) { }
+        flow_graph_(flow_graph) { }
   virtual ~FlowGraphOptimizer() {}
 
   FlowGraph* flow_graph() const { return flow_graph_; }
@@ -196,10 +194,7 @@ class FlowGraphOptimizer : public FlowGraphVisitor {
                                     Definition* left_instr,
                                     Definition* right_instr);
 
-  void AddToGuardedFields(const Field& field);
-
   FlowGraph* flow_graph_;
-  GrowableArray<const Field*>* guarded_fields_;
 
   DISALLOW_COPY_AND_ASSIGN(FlowGraphOptimizer);
 };

tests/language/vm/optimized_guarded_field_test.dart

@@ -0,0 +1,35 @@
+// 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.
+// Test correct handling of phis with only environment uses that were inserted
+// by store to load forwarding.
+// VMOptions=--optimization_counter_threshold=10
+
+import "package:expect/expect.dart";
+
+class A {
+  var foo;
+}
+
+class B {
+  get foo => null;
+}
+
+test(obj) => obj.foo == null ? "null" : "other";
+
+main() {
+  var a = new A();
+  var b = new B();
+  // Trigger optimization of test with a polymorphic load.
+  // The guarded type of foo is null.
+  test(a);
+  test(b);
+  for (var i = 0; i < 20; ++i) test(a);
+  Expect.equals("null", test(a));
+  Expect.equals("null", test(b));
+
+  // Store a non-null object into foo to trigger deoptimization of test.
+  a.foo = 123;
+  Expect.equals("other", test(a));
+  Expect.equals("null", test(b));
+}