From 3b86f823d0a8127f1099ea1139175f3f20ed6070 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Wed, 30 May 2018 13:44:43 +0000 Subject: [PATCH] [vm/corelib] Remove GrowableArrayMarker hack. GrowableArrayMarker was a class that implemented int and was used to enable implementation of default List factory constructor in pure Dart: factory List([int length = GROWABLE_ARRAY_MARKER]) { return identical(length, GROWABLE_ARRAY_MARKER) ? new _GrowableList(0) : new _List(length); } Its existence complicated all kinds of things in the VM and it is finally time to remove it. Instead we build List factory body directly in IL. This CL also provides inlining rule for `new List(n)` case. Change-Id: I870751658a4ac17fce649c9ac70395ff88a5436c Reviewed-on: https://dart-review.googlesource.com/57262 Commit-Queue: Vyacheslav Egorov Reviewed-by: Martin Kustermann --- .../transformer/bench_vector.dart.expect | 4 +- runtime/lib/array.cc | 6 ++ runtime/lib/array_patch.dart | 20 +---- runtime/vm/bootstrap_natives.h | 1 + runtime/vm/compiler/backend/inliner.cc | 22 +++++ .../compiler/frontend/flow_graph_builder.cc | 81 +++++++++++++++++++ .../vm/compiler/frontend/flow_graph_builder.h | 4 + runtime/vm/compiler/frontend/kernel_to_il.cc | 65 +++++++++++++++ runtime/vm/compiler/method_recognizer.h | 1 + 9 files changed, 183 insertions(+), 21 deletions(-) diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/bench_vector.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/bench_vector.dart.expect index e7c4fe32a43..49b8a686d65 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/bench_vector.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/bench_vector.dart.expect @@ -17,7 +17,7 @@ class _Vector extends core::Object { } operator *([@vm.inferred-type.metadata=#lib::_Vector?] self::_Vector a) → core::double { core::double result = 0.0; - for (core::int i = 0; [@vm.inferred-type.metadata=dart.core::bool?] i.{core::num::<}([@vm.direct-call.metadata=#lib::_Vector::_length] [@vm.inferred-type.metadata=!] this.{self::_Vector::_length}); i = i.{core::num::+}(1)) + for (core::int i = 0; [@vm.direct-call.metadata=dart.core::_IntegerImplementation:: args) → dynamic { core::Stopwatch timer = let final core::Stopwatch #t4 = new core::Stopwatch::•() in let final dynamic #t5 = [@vm.direct-call.metadata=dart.core::Stopwatch::start] #t4.{core::Stopwatch::start}() in #t4; - for (core::int i = 0; [@vm.inferred-type.metadata=dart.core::bool?] i.{core::num::<}(100000000); i = i.{core::num::+}(1)) { + for (core::int i = 0; [@vm.direct-call.metadata=dart.core::_IntegerImplementation:: { @patch - factory List([int length = _GROWABLE_ARRAY_MARKER]) { - if (identical(length, _GROWABLE_ARRAY_MARKER)) { - return new _GrowableList(0); - } - // All error handling on the length parameter is done at the implementation - // of new _List. - return new _List(length); - } + factory List([int length]) native "List_new"; @patch factory List.filled(int length, E fill, {bool growable: false}) { diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h index 8ed4b237d0c..eb76282b4a0 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/runtime/vm/bootstrap_natives.h @@ -111,6 +111,7 @@ namespace dart { V(RegExp_getGroupCount, 1) \ V(RegExp_ExecuteMatch, 3) \ V(RegExp_ExecuteMatchSticky, 3) \ + V(List_new, 2) \ V(List_allocate, 2) \ V(List_getIndexed, 2) \ V(List_setIndexed, 3) \ diff --git a/runtime/vm/compiler/backend/inliner.cc b/runtime/vm/compiler/backend/inliner.cc index 9e2bd30cf25..8ee80fe139a 100644 --- a/runtime/vm/compiler/backend/inliner.cc +++ b/runtime/vm/compiler/backend/inliner.cc @@ -3502,6 +3502,28 @@ bool FlowGraphInliner::TryInlineRecognizedMethod( return true; } + case MethodRecognizer::kListFactory: { + // We only want to inline new List(n) which decreases code size and + // improves performance. We don't want to inline new List(). + if (call->ArgumentCount() != 2) { + return false; + } + + const auto type = new (Z) Value(call->ArgumentAt(0)); + const auto num_elements = new (Z) Value(call->ArgumentAt(1)); + *entry = new (Z) + TargetEntryInstr(flow_graph->allocate_block_id(), + call->GetBlock()->try_index(), Thread::kNoDeoptId); + (*entry)->InheritDeoptTarget(Z, call); + *last = new (Z) CreateArrayInstr(call->token_pos(), type, num_elements, + call->deopt_id()); + flow_graph->AppendTo( + *entry, *last, + call->deopt_id() != Thread::kNoDeoptId ? call->env() : NULL, + FlowGraph::kValue); + return true; + } + case MethodRecognizer::kObjectArrayAllocate: { Value* num_elements = new (Z) Value(call->ArgumentAt(1)); if (num_elements->BindsToConstant() && diff --git a/runtime/vm/compiler/frontend/flow_graph_builder.cc b/runtime/vm/compiler/frontend/flow_graph_builder.cc index aa65f19d918..e7ecaac989e 100644 --- a/runtime/vm/compiler/frontend/flow_graph_builder.cc +++ b/runtime/vm/compiler/frontend/flow_graph_builder.cc @@ -3247,6 +3247,87 @@ void EffectGraphVisitor::VisitNativeBodyNode(NativeBodyNode* node) { length_load->set_recognized_kind(MethodRecognizer::kObjectArrayLength); return ReturnDefinition(length_load); } + case MethodRecognizer::kListFactory: { + // factory List([int length]) { + // return (:arg_desc.positional_count == 2) ? new _List(length) + // : new _GrowableList(0); + // } + ASSERT(owner_->parsed_function().has_arg_desc_var()); + const auto type_args_parameter = node->scope()->LookupVariable( + Symbols::TypeArgumentsParameter(), true); + const auto length_parameter = + node->scope()->LookupVariable(Symbols::Length(), true); + const Library& core_lib = Library::Handle(Z, Library::CoreLibrary()); + + // Build: :arg_desc.positional_count == 2 + TestGraphVisitor comparison(owner(), token_pos); + auto arg_descriptor = comparison.Bind(new (Z) LoadLocalInstr( + *owner_->parsed_function().arg_desc_var(), token_pos)); + auto positional_count = comparison.Bind(new (Z) LoadFieldInstr( + arg_descriptor, ArgumentsDescriptor::positional_count_offset(), + AbstractType::ZoneHandle(Z, Type::SmiType()), token_pos)); + auto constant_1 = comparison.Bind( + new (Z) ConstantInstr(Smi::ZoneHandle(Z, Smi::New(2)))); + comparison.ReturnDefinition(new (Z) StrictCompareInstr( + token_pos, Token::kEQ_STRICT, positional_count, constant_1, false, + owner()->GetNextDeoptId())); // No number check. + + // Build: :expr_temp = new _List(length) + ValueGraphVisitor allocate_non_growable(owner()); + { + auto arguments = new (Z) ZoneGrowableArray(Z, 2); + arguments->Add( + allocate_non_growable.PushArgument(allocate_non_growable.Bind( + new (Z) LoadLocalInstr(*type_args_parameter, token_pos)))); + arguments->Add( + allocate_non_growable.PushArgument(allocate_non_growable.Bind( + new (Z) LoadLocalInstr(*length_parameter, token_pos)))); + const Class& cls = Class::Handle( + Z, core_lib.LookupClass( + Library::PrivateCoreLibName(Symbols::_List()))); + ASSERT(!cls.IsNull()); + const intptr_t kTypeArgsLen = 0; + const Function& func = Function::ZoneHandle( + Z, cls.LookupFactoryAllowPrivate(Symbols::_ListFactory())); + ASSERT(!func.IsNull()); + allocate_non_growable.ReturnDefinition(new (Z) StaticCallInstr( + token_pos, func, kTypeArgsLen, + Object::null_array(), // No names. + arguments, owner()->ic_data_array(), owner()->GetNextDeoptId(), + ICData::kStatic)); + allocate_non_growable.Do( + BuildStoreExprTemp(allocate_non_growable.value(), token_pos)); + } + + // Build: :expr_temp = new _GrowableList(0) + ValueGraphVisitor allocate_growable(owner()); + { + auto arguments = new (Z) ZoneGrowableArray(Z, 1); + arguments->Add(allocate_growable.PushArgument(allocate_growable.Bind( + new (Z) LoadLocalInstr(*type_args_parameter, token_pos)))); + arguments->Add(allocate_growable.PushArgument(allocate_growable.Bind( + new (Z) ConstantInstr(Smi::ZoneHandle(Z, Smi::New(0)))))); + const Class& cls = Class::Handle( + Z, core_lib.LookupClass( + Library::PrivateCoreLibName(Symbols::_GrowableList()))); + ASSERT(!cls.IsNull()); + const intptr_t kTypeArgsLen = 0; + const Function& func = Function::ZoneHandle( + Z, + cls.LookupFactoryAllowPrivate(Symbols::_GrowableListFactory())); + ASSERT(!func.IsNull()); + allocate_growable.ReturnDefinition(new (Z) StaticCallInstr( + token_pos, func, kTypeArgsLen, + Object::null_array(), // No names. + arguments, owner()->ic_data_array(), owner()->GetNextDeoptId(), + ICData::kStatic)); + allocate_growable.Do( + BuildStoreExprTemp(allocate_growable.value(), token_pos)); + } + + Join(comparison, allocate_non_growable, allocate_growable); + return ReturnDefinition(BuildLoadExprTemp(token_pos)); + } case MethodRecognizer::kObjectArrayAllocate: { LocalVariable* type_args_parameter = node->scope()->LookupVariable( Symbols::TypeArgumentsParameter(), true); diff --git a/runtime/vm/compiler/frontend/flow_graph_builder.h b/runtime/vm/compiler/frontend/flow_graph_builder.h index f7371030d4d..057cf566f0b 100644 --- a/runtime/vm/compiler/frontend/flow_graph_builder.h +++ b/runtime/vm/compiler/frontend/flow_graph_builder.h @@ -492,6 +492,8 @@ class ValueGraphVisitor : public EffectGraphVisitor { Value* value_; private: + friend class EffectGraphVisitor; + // Helper to set the output state to return a Value. virtual void ReturnValue(Value* value) { value_ = value; } @@ -539,6 +541,8 @@ class TestGraphVisitor : public ValueGraphVisitor { TokenPosition condition_token_pos() const { return condition_token_pos_; } private: + friend class EffectGraphVisitor; + // Construct and concatenate a Branch instruction to this graph fragment. // Closes the fragment and sets the output parameters. virtual void ReturnValue(Value* value); diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index 34aa36e2b0c..73619a74269 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -2135,6 +2135,71 @@ Fragment FlowGraphBuilder::NativeFunctionBody(intptr_t first_positional_offset, Array::length_offset(), Type::ZoneHandle(Z, Type::SmiType()), kSmiCid); break; + case MethodRecognizer::kListFactory: { + // factory List([int length]) { + // return (:arg_desc.positional_count == 2) ? new _List(length) + // : new _GrowableList(0); + // } + const Library& core_lib = Library::Handle(Z, Library::CoreLibrary()); + + TargetEntryInstr *allocate_non_growable, *allocate_growable; + + body += LoadArgDescriptor(); + body += + LoadField(ArgumentsDescriptor::positional_count_offset(), kSmiCid); + body += IntConstant(2); + body += BranchIfStrictEqual(&allocate_non_growable, &allocate_growable); + + JoinEntryInstr* join = BuildJoinEntry(); + + { + const Class& cls = Class::Handle( + Z, core_lib.LookupClass( + Library::PrivateCoreLibName(Symbols::_List()))); + ASSERT(!cls.IsNull()); + const Function& func = Function::ZoneHandle( + Z, cls.LookupFactoryAllowPrivate(Symbols::_ListFactory())); + ASSERT(!func.IsNull()); + + Fragment allocate(allocate_non_growable); + allocate += LoadLocal(scopes_->type_arguments_variable); + allocate += PushArgument(); + allocate += LoadLocal(LookupVariable(first_positional_offset)); + allocate += PushArgument(); + allocate += + StaticCall(TokenPosition::kNoSource, func, 2, ICData::kStatic); + allocate += StoreLocal(TokenPosition::kNoSource, + parsed_function_->expression_temp_var()); + allocate += Drop(); + allocate += Goto(join); + } + + { + const Class& cls = Class::Handle( + Z, core_lib.LookupClass( + Library::PrivateCoreLibName(Symbols::_GrowableList()))); + ASSERT(!cls.IsNull()); + const Function& func = Function::ZoneHandle( + Z, cls.LookupFactoryAllowPrivate(Symbols::_GrowableListFactory())); + ASSERT(!func.IsNull()); + + Fragment allocate(allocate_growable); + allocate += LoadLocal(scopes_->type_arguments_variable); + allocate += PushArgument(); + allocate += IntConstant(0); + allocate += PushArgument(); + allocate += + StaticCall(TokenPosition::kNoSource, func, 2, ICData::kStatic); + allocate += StoreLocal(TokenPosition::kNoSource, + parsed_function_->expression_temp_var()); + allocate += Drop(); + allocate += Goto(join); + } + + body = Fragment(body.entry, join); + body += LoadLocal(parsed_function_->expression_temp_var()); + break; + } case MethodRecognizer::kObjectArrayAllocate: body += LoadLocal(scopes_->type_arguments_variable); body += LoadLocal(LookupVariable(first_positional_offset)); diff --git a/runtime/vm/compiler/method_recognizer.h b/runtime/vm/compiler/method_recognizer.h index afffc776e45..6f97c6086b7 100644 --- a/runtime/vm/compiler/method_recognizer.h +++ b/runtime/vm/compiler/method_recognizer.h @@ -19,6 +19,7 @@ namespace dart { V(::, identical, ObjectIdentical, Bool, 0x49c6e96a) \ V(ClassID, getID, ClassIDgetID, Smi, 0x7b18b257) \ V(Object, Object., ObjectConstructor, Dynamic, 0x681617fe) \ + V(List, ., ListFactory, Dynamic, 0x629f8324) \ V(_List, ., ObjectArrayAllocate, Array, 0x2121902f) \ V(_TypedList, _getInt8, ByteArrayBaseGetInt8, Smi, 0x7041895a) \ V(_TypedList, _getUint8, ByteArrayBaseGetUint8, Smi, 0x336fa3ea) \