Reland: Fix bug in field type tracking and polymorphic inlining.

Original CL: https://codereview.chromium.org/24096018/

This fixes the bug by changing FlowGraphBuilder to a zone object
because it is needed by the inliner later in the pipeline.

R=kmillikin@google.com

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

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

runtime/vm/compiler.cc

@@ -270,7 +270,9 @@ static bool CompileParsedFunctionHelper(ParsedFunction* parsed_function,
     LongJump bailout_jump;
     isolate->set_long_jump_base(&bailout_jump);
     if (setjmp(*bailout_jump.Set()) == 0) {
+      FlowGraphBuilder* builder = NULL;
       FlowGraph* flow_graph = NULL;
+      GrowableArray<const Field*> guarded_fields;
       // TimerScope needs an isolate to be properly terminated in case of a
       // LongJump.
       {
@@ -290,11 +292,12 @@ static bool CompileParsedFunctionHelper(ParsedFunction* parsed_function,
         }
 
         // Build the flow graph.
-        FlowGraphBuilder builder(parsed_function,
-                                 ic_data_array,
-                                 NULL,  // NULL = not inlining.
-                                 osr_id);
-        flow_graph = builder.BuildGraph();
+        builder = new FlowGraphBuilder(parsed_function,
+                                       ic_data_array,
+                                       NULL,  // NULL = not inlining.
+                                       &guarded_fields,
+                                       osr_id);
+        flow_graph = builder->BuildGraph();
       }
 
       if (FLAG_print_flow_graph ||
@@ -326,13 +329,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 +360,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

@@ -98,13 +98,14 @@ class InlineExitCollector: public ZoneAllocated {
 
 
 // Build a flow graph from a parsed function's AST.
-class FlowGraphBuilder: public ValueObject {
+class FlowGraphBuilder: public ZoneAllocated {
  public:
   // The inlining context is NULL if not inlining.  The osr_id is the deopt
   // id of the OSR entry or Isolate::kNoDeoptId if not compiling for OSR.
   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,17 +542,19 @@ class CallSiteInliner : public ValueObject {
       // Build the callee graph.
       InlineExitCollector* exit_collector =
           new InlineExitCollector(caller_graph_, call);
-      FlowGraphBuilder builder(parsed_function,
-                               ic_data_array,
-                               exit_collector,
-                               Isolate::kNoDeoptId);
-      builder.SetInitialBlockId(caller_graph_->max_block_id());
+      GrowableArray<const Field*> inlined_guarded_fields;
+      FlowGraphBuilder* builder = new FlowGraphBuilder(parsed_function,
+                                                       ic_data_array,
+                                                       exit_collector,
+                                                       &inlined_guarded_fields,
+                                                       Isolate::kNoDeoptId);
+      builder->SetInitialBlockId(caller_graph_->max_block_id());
       FlowGraph* callee_graph;
       {
         TimerScope timer(FLAG_compiler_stats,
                          &CompilerStats::graphinliner_build_timer,
                          isolate);
-        callee_graph = builder.BuildGraph();
+        callee_graph = builder->BuildGraph();
       }
 
       // The parameter stubs are a copy of the actual arguments providing
@@ -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));
+}