From 5694a140e34c3fa1fda8182f6683c920c9453ed7 Mon Sep 17 00:00:00 2001 From: "srdjan@google.com" Date: Wed, 13 Feb 2013 18:44:27 +0000 Subject: [PATCH] Fix allocation of array tables (use store barrier if needed, store values directly instead of via stack). Review URL: https://codereview.chromium.org//12212050 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@18464 260f80e4-7a28-3924-810f-c04153c831b5 --- runtime/vm/ast.h | 10 +++++- runtime/vm/compiler.cc | 3 ++ runtime/vm/flow_graph_builder.cc | 41 +++++++++++++++------- runtime/vm/flow_graph_optimizer.cc | 2 +- runtime/vm/intermediate_language.h | 12 +++---- runtime/vm/intermediate_language_ia32.cc | 8 +---- runtime/vm/intermediate_language_x64.cc | 8 +---- runtime/vm/parser.cc | 43 ++++++++++++++++++++---- runtime/vm/parser.h | 16 +++++++++ runtime/vm/symbols.h | 1 + 10 files changed, 101 insertions(+), 43 deletions(-) diff --git a/runtime/vm/ast.h b/runtime/vm/ast.h index 721de0792a4..07e23144388 100644 --- a/runtime/vm/ast.h +++ b/runtime/vm/ast.h @@ -265,17 +265,22 @@ class ArgumentDefinitionTestNode : public AstNode { class ArrayNode : public AstNode { public: - ArrayNode(intptr_t token_pos, const AbstractType& type) + ArrayNode(intptr_t token_pos, + const AbstractType& type, + const LocalVariable& temp) : AstNode(token_pos), type_(type), + temp_local_(temp), elements_() { CheckFields(); } ArrayNode(intptr_t token_pos, const AbstractType& type, + const LocalVariable& temp, const GrowableArray& elements) : AstNode(token_pos), type_(type), + temp_local_(temp), elements_(elements.length()) { CheckFields(); for (intptr_t i = 0; i < elements.length(); i++) { @@ -295,10 +300,13 @@ class ArrayNode : public AstNode { const AbstractType& type() const { return type_; } + const LocalVariable& temp_local() const { return temp_local_; } + DECLARE_COMMON_NODE_FUNCTIONS(ArrayNode); private: const AbstractType& type_; + const LocalVariable& temp_local_; // Store allocated array while filling it. GrowableArray elements_; void CheckFields() { diff --git a/runtime/vm/compiler.cc b/runtime/vm/compiler.cc index ae0db89fb6b..46bda28f7ae 100644 --- a/runtime/vm/compiler.cc +++ b/runtime/vm/compiler.cc @@ -636,6 +636,9 @@ RawObject* Compiler::ExecuteOnce(SequenceNode* fragment) { parsed_function->set_expression_temp_var( ParsedFunction::CreateExpressionTempVar(0)); fragment->scope()->AddVariable(parsed_function->expression_temp_var()); + parsed_function->set_array_literal_var( + ParsedFunction::CreateArrayLiteralVar(0)); + fragment->scope()->AddVariable(parsed_function->array_literal_var()); parsed_function->AllocateVariables(); // Non-optimized code generator. diff --git a/runtime/vm/flow_graph_builder.cc b/runtime/vm/flow_graph_builder.cc index ce2e218b5d1..909d9375ccf 100644 --- a/runtime/vm/flow_graph_builder.cc +++ b/runtime/vm/flow_graph_builder.cc @@ -1656,24 +1656,37 @@ void EffectGraphVisitor::VisitArgumentDefinitionTestNode( void EffectGraphVisitor::VisitArrayNode(ArrayNode* node) { - // Translate the array elements and collect their values. - ZoneGrowableArray* arguments = - new ZoneGrowableArray(node->length()); - for (int i = 0; i < node->length(); ++i) { - ValueGraphVisitor for_value(owner(), temp_index()); - node->ElementAt(i)->Visit(&for_value); - Append(for_value); - arguments->Add(PushArgument(for_value.value())); - } const AbstractTypeArguments& type_args = AbstractTypeArguments::ZoneHandle(node->type().arguments()); Value* element_type = BuildInstantiatedTypeArguments(node->token_pos(), type_args); CreateArrayInstr* create = new CreateArrayInstr(node->token_pos(), - arguments, + node->length(), node->type(), element_type); - ReturnDefinition(create); + Value* array_val = Bind(create); + Definition* store = BuildStoreTemp(node->temp_local(), array_val); + Do(store); + + const intptr_t class_id = create->Type()->ToCid(); + const intptr_t deopt_id = Isolate::kNoDeoptId; + for (int i = 0; i < node->length(); ++i) { + Value* array = Bind( + new LoadLocalInstr(node->temp_local(), owner()->context_level())); + Value* index = Bind(new ConstantInstr(Smi::ZoneHandle(Smi::New(i)))); + ValueGraphVisitor for_value(owner(), temp_index()); + node->ElementAt(i)->Visit(&for_value); + Append(for_value); + // No store barrier needed for constants. + const bool emit_store_barrier = !for_value.value()->BindsToConstant(); + StoreIndexedInstr* store = new StoreIndexedInstr( + array, index, for_value.value(), + emit_store_barrier, class_id, deopt_id); + Do(store); + } + + ReturnDefinition( + new LoadLocalInstr(node->temp_local(), owner()->context_level())); } @@ -3041,8 +3054,10 @@ StaticCallInstr* EffectGraphVisitor::BuildStaticNoSuchMethodCall( arguments->Add(new LiteralNode(args_pos, args_descriptor)); // The third argument is an array containing the original method arguments, // including the receiver. - ArrayNode* args_array = - new ArrayNode(args_pos, Type::ZoneHandle(Type::ArrayType())); + ArrayNode* args_array = new ArrayNode( + args_pos, + Type::ZoneHandle(Type::ArrayType()), + *owner()->parsed_function().array_literal_var()); for (intptr_t i = 0; i < method_arguments->length(); i++) { args_array->AddElement(method_arguments->NodeAt(i)); } diff --git a/runtime/vm/flow_graph_optimizer.cc b/runtime/vm/flow_graph_optimizer.cc index f7db803b3ef..bc05d4224ae 100644 --- a/runtime/vm/flow_graph_optimizer.cc +++ b/runtime/vm/flow_graph_optimizer.cc @@ -3841,7 +3841,7 @@ void ConstantPropagator::VisitLoadField(LoadFieldInstr* instr) { if ((instr->recognized_kind() == MethodRecognizer::kObjectArrayLength) && (instr->value()->definition()->IsCreateArray())) { const intptr_t length = - instr->value()->definition()->AsCreateArray()->ArgumentCount(); + instr->value()->definition()->AsCreateArray()->num_elements(); const Object& result = Smi::ZoneHandle(Smi::New(length)); SetValue(instr, result); return; diff --git a/runtime/vm/intermediate_language.h b/runtime/vm/intermediate_language.h index 5f3be97cd6c..7e875e16fc2 100644 --- a/runtime/vm/intermediate_language.h +++ b/runtime/vm/intermediate_language.h @@ -3003,16 +3003,13 @@ class AllocateObjectWithBoundsCheckInstr : public TemplateDefinition<2> { class CreateArrayInstr : public TemplateDefinition<1> { public: CreateArrayInstr(intptr_t token_pos, - ZoneGrowableArray* arguments, + intptr_t num_elements, const AbstractType& type, Value* element_type) : token_pos_(token_pos), - arguments_(arguments), + num_elements_(num_elements), type_(type) { #if defined(DEBUG) - for (int i = 0; i < ArgumentCount(); ++i) { - ASSERT(ArgumentAt(i) != NULL); - } ASSERT(element_type != NULL); ASSERT(type_.IsZoneHandle()); ASSERT(!type_.IsNull()); @@ -3024,10 +3021,9 @@ class CreateArrayInstr : public TemplateDefinition<1> { DECLARE_INSTRUCTION(CreateArray) virtual CompileType* ComputeInitialType() const; - virtual intptr_t ArgumentCount() const { return arguments_->length(); } + intptr_t num_elements() const { return num_elements_; } intptr_t token_pos() const { return token_pos_; } - PushArgumentInstr* ArgumentAt(intptr_t i) const { return (*arguments_)[i]; } const AbstractType& type() const { return type_; } Value* element_type() const { return inputs_[0]; } @@ -3039,7 +3035,7 @@ class CreateArrayInstr : public TemplateDefinition<1> { private: const intptr_t token_pos_; - ZoneGrowableArray* const arguments_; + const intptr_t num_elements_; const AbstractType& type_; DISALLOW_COPY_AND_ASSIGN(CreateArrayInstr); diff --git a/runtime/vm/intermediate_language_ia32.cc b/runtime/vm/intermediate_language_ia32.cc index d0e5efdf38d..5e54a82107b 100644 --- a/runtime/vm/intermediate_language_ia32.cc +++ b/runtime/vm/intermediate_language_ia32.cc @@ -1589,18 +1589,12 @@ LocationSummary* CreateArrayInstr::MakeLocationSummary() const { void CreateArrayInstr::EmitNativeCode(FlowGraphCompiler* compiler) { // Allocate the array. EDX = length, ECX = element type. ASSERT(locs()->in(0).reg() == ECX); - __ movl(EDX, Immediate(Smi::RawValue(ArgumentCount()))); + __ movl(EDX, Immediate(Smi::RawValue(num_elements()))); compiler->GenerateCall(token_pos(), &StubCode::AllocateArrayLabel(), PcDescriptors::kOther, locs()); ASSERT(locs()->out().reg() == EAX); - - // Pop the element values from the stack into the array. - __ leal(EDX, FieldAddress(EAX, Array::data_offset())); - for (int i = ArgumentCount() - 1; i >= 0; --i) { - __ popl(Address(EDX, i * kWordSize)); - } } diff --git a/runtime/vm/intermediate_language_x64.cc b/runtime/vm/intermediate_language_x64.cc index 87aa256c996..c3a3eec047a 100644 --- a/runtime/vm/intermediate_language_x64.cc +++ b/runtime/vm/intermediate_language_x64.cc @@ -1426,18 +1426,12 @@ LocationSummary* CreateArrayInstr::MakeLocationSummary() const { void CreateArrayInstr::EmitNativeCode(FlowGraphCompiler* compiler) { // Allocate the array. R10 = length, RBX = element type. ASSERT(locs()->in(0).reg() == RBX); - __ movq(R10, Immediate(Smi::RawValue(ArgumentCount()))); + __ movq(R10, Immediate(Smi::RawValue(num_elements()))); compiler->GenerateCall(token_pos(), &StubCode::AllocateArrayLabel(), PcDescriptors::kOther, locs()); ASSERT(locs()->out().reg() == RAX); - - // Pop the element values from the stack into the array. - __ leaq(R10, FieldAddress(RAX, Array::data_offset())); - for (int i = ArgumentCount() - 1; i >= 0; --i) { - __ popq(Address(R10, i * kWordSize)); - } } diff --git a/runtime/vm/parser.cc b/runtime/vm/parser.cc index cee4d82ffeb..1baa491ed28 100644 --- a/runtime/vm/parser.cc +++ b/runtime/vm/parser.cc @@ -114,6 +114,13 @@ LocalVariable* ParsedFunction::CreateExpressionTempVar(intptr_t token_pos) { } +LocalVariable* ParsedFunction::CreateArrayLiteralVar(intptr_t token_pos) { + return new LocalVariable(token_pos, + Symbols::ArrayLiteralVar(), + Type::ZoneHandle(Type::ArrayType())); +} + + void ParsedFunction::SetNodeSequence(SequenceNode* node_sequence) { ASSERT(node_sequence_ == NULL); ASSERT(node_sequence != NULL); @@ -758,6 +765,10 @@ void Parser::ParseFunction(ParsedFunction* parsed_function) { UNREACHABLE(); } + parsed_function->set_array_literal_var( + ParsedFunction::CreateArrayLiteralVar(func.token_pos())); + node_sequence->scope()->AddVariable(parsed_function->array_literal_var()); + if (!HasReturnNode(node_sequence)) { // Add implicit return node. node_sequence->Add(new ReturnNode(func.end_token_pos())); @@ -1375,6 +1386,18 @@ static const String& PrivateCoreLibName(const String& str) { } +LocalVariable* Parser::BuildArrayTempLocal(intptr_t token_pos) { + char name[64]; + OS::SNPrint(name, 64, ":arrlit%"Pd, token_pos); + LocalVariable* temp = + new LocalVariable(token_pos, + String::ZoneHandle(Symbols::New(name)), + Type::ZoneHandle(Type::ArrayType())); + current_block_->scope->AddVariable(temp); + return temp; +} + + StaticCallNode* Parser::BuildInvocationMirrorAllocation( intptr_t call_pos, const String& function_name, @@ -1393,7 +1416,8 @@ StaticCallNode* Parser::BuildInvocationMirrorAllocation( // The third argument is an array containing the original function arguments, // including the receiver. ArrayNode* args_array = new ArrayNode( - args_pos, Type::ZoneHandle(Type::ArrayType())); + args_pos, Type::ZoneHandle(Type::ArrayType()), + *BuildArrayTempLocal(call_pos)); for (intptr_t i = 0; i < function_args.length(); i++) { args_array->AddElement(function_args.NodeAt(i)); } @@ -8683,7 +8707,8 @@ AstNode* Parser::ParseListLiteral(intptr_t type_pos, } factory_type_args = factory_type_args.Canonicalize(); ArgumentListNode* factory_param = new ArgumentListNode(literal_pos); - ArrayNode* list = new ArrayNode(TokenPos(), type, element_list); + const LocalVariable& temp_local = *BuildArrayTempLocal(type_pos); + ArrayNode* list = new ArrayNode(TokenPos(), type, temp_local, element_list); factory_param->Add(list); return CreateConstructorCallNode(literal_pos, factory_type_args, @@ -8773,8 +8798,6 @@ AstNode* Parser::ParseMapLiteral(intptr_t type_pos, ASSERT(map_type_arguments.IsNull() || (map_type_arguments.Length() == 2)); map_type_arguments ^= map_type_arguments.Canonicalize(); - // The kv_pair array is temporary and of element type dynamic. It is passed - // to the factory to initialize a properly typed map. GrowableArray kv_pairs_list; // Parse the map entries. Note: there may be an optional extra // comma after the last entry. @@ -8902,8 +8925,13 @@ AstNode* Parser::ParseMapLiteral(intptr_t type_pos, } factory_type_args = factory_type_args.Canonicalize(); ArgumentListNode* factory_param = new ArgumentListNode(literal_pos); + // The kv_pair array is temporary and of element type dynamic. It is passed + // to the factory to initialize a properly typed map. ArrayNode* kv_pairs = new ArrayNode( - TokenPos(), Type::ZoneHandle(Type::ArrayType()), kv_pairs_list); + TokenPos(), + Type::ZoneHandle(Type::ArrayType()), + *BuildArrayTempLocal(type_pos), + kv_pairs_list); factory_param->Add(kv_pairs); return CreateConstructorCallNode(literal_pos, factory_type_args, @@ -9279,7 +9307,10 @@ AstNode* Parser::ParseStringLiteral() { } else { ArgumentListNode* interpolate_arg = new ArgumentListNode(TokenPos()); ArrayNode* values = new ArrayNode( - TokenPos(), Type::ZoneHandle(Type::ArrayType()), values_list); + TokenPos(), + Type::ZoneHandle(Type::ArrayType()), + *BuildArrayTempLocal(TokenPos()), + values_list); interpolate_arg->Add(values); primary = MakeStaticCall(Symbols::StringBase(), PrivateCoreLibName(Symbols::Interpolate()), diff --git a/runtime/vm/parser.h b/runtime/vm/parser.h index 6ca97c4230f..89b83055764 100644 --- a/runtime/vm/parser.h +++ b/runtime/vm/parser.h @@ -40,6 +40,7 @@ class ParsedFunction : public ZoneAllocated { saved_current_context_var_(NULL), saved_entry_context_var_(NULL), expression_temp_var_(NULL), + array_literal_var_(NULL), first_parameter_index_(0), first_stack_local_index_(0), num_copied_params_(0), @@ -102,6 +103,17 @@ class ParsedFunction : public ZoneAllocated { } static LocalVariable* CreateExpressionTempVar(intptr_t token_pos); + void set_array_literal_var(LocalVariable* local) { + ASSERT((local != NULL) && (array_literal_var_ == NULL)); + array_literal_var_ = local; + } + LocalVariable* array_literal_var() const { + ASSERT(array_literal_var_ != NULL); + return array_literal_var_; + } + + static LocalVariable* CreateArrayLiteralVar(intptr_t token_pos); + int first_parameter_index() const { return first_parameter_index_; } int first_stack_local_index() const { return first_stack_local_index_; } int num_copied_params() const { return num_copied_params_; } @@ -117,6 +129,9 @@ class ParsedFunction : public ZoneAllocated { LocalVariable* saved_current_context_var_; LocalVariable* saved_entry_context_var_; LocalVariable* expression_temp_var_; + // TODO(hausner): Remove once ArrayNode creation is removed from flow + // graph builder. + LocalVariable* array_literal_var_; int first_parameter_index_; int first_stack_local_index_; @@ -612,6 +627,7 @@ class Parser : public ValueObject { const Function& constructor, ArgumentListNode* arguments); + LocalVariable* BuildArrayTempLocal(intptr_t token_pos); const Script& script_; TokenStream::Iterator tokens_iterator_; diff --git a/runtime/vm/symbols.h b/runtime/vm/symbols.h index caec90601df..f21565f90f6 100644 --- a/runtime/vm/symbols.h +++ b/runtime/vm/symbols.h @@ -59,6 +59,7 @@ class ObjectPointerVisitor; V(SavedTryContextVar, ":saved_try_context_var") \ V(ExceptionVar, ":exception_var") \ V(StacktraceVar, ":stacktrace_var") \ + V(ArrayLiteralVar, ":array_literal_var") \ V(ListLiteralElement, "list literal element") \ V(ForInIter, ":for-in-iter") \ V(Library, "library") \