From 7dea1d8a2c8b545dc99da2b8a2bed0ca70d8298a Mon Sep 17 00:00:00 2001 From: "fschneider@google.com" Date: Thu, 19 Sep 2013 11:21:11 +0000 Subject: [PATCH] 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 | 7 ++-- runtime/vm/flow_graph.cc | 1 + runtime/vm/flow_graph.h | 5 +++ runtime/vm/flow_graph_builder.cc | 27 ++++++++++++++ runtime/vm/flow_graph_builder.h | 8 +++++ runtime/vm/flow_graph_inliner.cc | 23 +++++++----- runtime/vm/flow_graph_inliner.h | 5 +-- runtime/vm/flow_graph_optimizer.cc | 16 +-------- runtime/vm/flow_graph_optimizer.h | 9 ++--- .../vm/optimized_guarded_field_test.dart | 35 +++++++++++++++++++ 10 files changed, 98 insertions(+), 38 deletions(-) create mode 100644 tests/language/vm/optimized_guarded_field_test.dart diff --git a/runtime/vm/compiler.cc b/runtime/vm/compiler.cc index 1f2d1c2ac68..bae54397970 100644 --- a/runtime/vm/compiler.cc +++ b/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 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 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()); diff --git a/runtime/vm/flow_graph.cc b/runtime/vm/flow_graph.cc index 7a7632737e3..e0a71b0ac21 100644 --- a/runtime/vm/flow_graph.cc +++ b/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()), diff --git a/runtime/vm/flow_graph.h b/runtime/vm/flow_graph.h index 7e75d7e8c89..7b4be08bec5 100644 --- a/runtime/vm/flow_graph.h +++ b/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_; diff --git a/runtime/vm/flow_graph_builder.cc b/runtime/vm/flow_graph_builder.cc index b152dd92313..42a4ffab5c7 100644 --- a/runtime/vm/flow_graph_builder.cc +++ b/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* 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); } diff --git a/runtime/vm/flow_graph_builder.h b/runtime/vm/flow_graph_builder.h index e3d01c30739..912e38691ad 100644 --- a/runtime/vm/flow_graph_builder.h +++ b/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* 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* 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* guarded_fields_; intptr_t last_used_block_id_; intptr_t context_level_; diff --git a/runtime/vm/flow_graph_inliner.cc b/runtime/vm/flow_graph_inliner.cc index c62c2444b65..910e26521f2 100644 --- a/runtime/vm/flow_graph_inliner.cc +++ b/runtime/vm/flow_graph_inliner.cc @@ -382,8 +382,7 @@ class PolymorphicInliner : public ValueObject { class CallSiteInliner : public ValueObject { public: - CallSiteInliner(FlowGraph* flow_graph, - GrowableArray* 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 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 function_cache_; - GrowableArray* 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()) { diff --git a/runtime/vm/flow_graph_inliner.h b/runtime/vm/flow_graph_inliner.h index 57318e3ecf1..dfee0d5d392 100644 --- a/runtime/vm/flow_graph_inliner.h +++ b/runtime/vm/flow_graph_inliner.h @@ -15,9 +15,7 @@ template class GrowableArray; class FlowGraphInliner : ValueObject { public: - FlowGraphInliner(FlowGraph* flow_graph, - GrowableArray* 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* guarded_fields_; DISALLOW_COPY_AND_ASSIGN(FlowGraphInliner); }; diff --git a/runtime/vm/flow_graph_optimizer.cc b/runtime/vm/flow_graph_optimizer.cc index 329fe19a382..44aad69720e 100644 --- a/runtime/vm/flow_graph_optimizer.cc +++ b/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 diff --git a/runtime/vm/flow_graph_optimizer.h b/runtime/vm/flow_graph_optimizer.h index 0401d3f2ce6..8c9f6fe7356 100644 --- a/runtime/vm/flow_graph_optimizer.h +++ b/runtime/vm/flow_graph_optimizer.h @@ -16,11 +16,9 @@ class ParsedFunction; class FlowGraphOptimizer : public FlowGraphVisitor { public: - FlowGraphOptimizer(FlowGraph* flow_graph, - GrowableArray* 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* guarded_fields_; DISALLOW_COPY_AND_ASSIGN(FlowGraphOptimizer); }; diff --git a/tests/language/vm/optimized_guarded_field_test.dart b/tests/language/vm/optimized_guarded_field_test.dart new file mode 100644 index 00000000000..2ef5dcbfc49 --- /dev/null +++ b/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)); +}