From 5097f3d37c65e87b646584d51fd90e381fd1bfb2 Mon Sep 17 00:00:00 2001 From: Toon Verwaest Date: Tue, 21 Mar 2017 17:31:37 +0100 Subject: [PATCH] Reland "[ic] Migrate StoreGlobal to data handler" The problem was that transitioning element stores had a similar shape to the new StoreGlobal case. The problem was fixed by https://chromium-review.googlesource.com/c/457341/ BUG=v8:5561 Change-Id: If996e9b37809ba8edf6dcb228b116b77021ce7bc Reviewed-on: https://chromium-review.googlesource.com/457324 Reviewed-by: Igor Sheludko Commit-Queue: Toon Verwaest Cr-Commit-Position: refs/heads/master@{#43992} --- src/code-stub-assembler.cc | 2 +- src/code-stub-assembler.h | 3 +- src/code-stubs.cc | 82 ------------------------------ src/code-stubs.h | 44 ---------------- src/counters.h | 4 +- src/ic/accessor-assembler.cc | 64 ++++++++++++++++++++++- src/ic/handler-configuration-inl.h | 32 ++++++------ src/ic/handler-configuration.h | 32 ++++++------ src/ic/ic-inl.h | 2 +- src/ic/ic.cc | 63 +++++------------------ 10 files changed, 114 insertions(+), 214 deletions(-) diff --git a/src/code-stub-assembler.cc b/src/code-stub-assembler.cc index f980784091e6..183a04495313 100644 --- a/src/code-stub-assembler.cc +++ b/src/code-stub-assembler.cc @@ -2960,7 +2960,7 @@ Node* CodeStubAssembler::IsJSArray(Node* object) { } Node* CodeStubAssembler::IsWeakCell(Node* object) { - return HasInstanceType(object, WEAK_CELL_TYPE); + return IsWeakCellMap(LoadMap(object)); } Node* CodeStubAssembler::IsBoolean(Node* object) { diff --git a/src/code-stub-assembler.h b/src/code-stub-assembler.h index 987f70c23588..a605cb3311c3 100644 --- a/src/code-stub-assembler.h +++ b/src/code-stub-assembler.h @@ -48,7 +48,8 @@ enum class PrimitiveType { kBoolean, kNumber, kString, kSymbol }; V(TrueValue, True) \ V(Tuple2Map, Tuple2Map) \ V(Tuple3Map, Tuple3Map) \ - V(UndefinedValue, Undefined) + V(UndefinedValue, Undefined) \ + V(WeakCellMap, WeakCellMap) // Provides JavaScript-specific "macro-assembler" functionality on top of the // CodeAssembler. By factoring the JavaScript-isms out of the CodeAssembler, diff --git a/src/code-stubs.cc b/src/code-stubs.cc index 7b7e8b0a3cf8..dcf6b2e3a5e0 100644 --- a/src/code-stubs.cc +++ b/src/code-stubs.cc @@ -1490,88 +1490,6 @@ void SubStringStub::GenerateAssembly( assembler.Parameter(Descriptor::kContext))); } -void StoreGlobalStub::GenerateAssembly( - compiler::CodeAssemblerState* state) const { - typedef CodeStubAssembler::Label Label; - typedef compiler::Node Node; - CodeStubAssembler assembler(state); - - assembler.Comment("StoreGlobalStub: cell_type=%d, constant_type=%d", - cell_type(), - PropertyCellType::kConstantType == cell_type() - ? static_cast(constant_type()) - : -1); - - Node* receiver = assembler.Parameter(Descriptor::kReceiver); - Node* name = assembler.Parameter(Descriptor::kName); - Node* value = assembler.Parameter(Descriptor::kValue); - Node* slot = assembler.Parameter(Descriptor::kSlot); - Node* vector = assembler.Parameter(Descriptor::kVector); - Node* context = assembler.Parameter(Descriptor::kContext); - - Label miss(&assembler); - - Node* weak_cell = assembler.HeapConstant(isolate()->factory()->NewWeakCell( - StoreGlobalStub::property_cell_placeholder(isolate()))); - Node* cell = assembler.LoadWeakCellValue(weak_cell); - assembler.GotoIf(assembler.TaggedIsSmi(cell), &miss); - - // Load the payload of the global parameter cell. A hole indicates that the - // cell has been invalidated and that the store must be handled by the - // runtime. - Node* cell_contents = - assembler.LoadObjectField(cell, PropertyCell::kValueOffset); - - PropertyCellType cell_type = this->cell_type(); - if (cell_type == PropertyCellType::kConstant || - cell_type == PropertyCellType::kUndefined) { - // This is always valid for all states a cell can be in. - assembler.GotoIf(assembler.WordNotEqual(cell_contents, value), &miss); - } else { - assembler.GotoIf(assembler.IsTheHole(cell_contents), &miss); - - // When dealing with constant types, the type may be allowed to change, as - // long as optimized code remains valid. - bool value_is_smi = false; - if (cell_type == PropertyCellType::kConstantType) { - switch (constant_type()) { - case PropertyCellConstantType::kSmi: - assembler.GotoIfNot(assembler.TaggedIsSmi(value), &miss); - value_is_smi = true; - break; - case PropertyCellConstantType::kStableMap: { - // It is sufficient here to check that the value and cell contents - // have identical maps, no matter if they are stable or not or if they - // are the maps that were originally in the cell or not. If optimized - // code will deopt when a cell has a unstable map and if it has a - // dependency on a stable map, it will deopt if the map destabilizes. - assembler.GotoIf(assembler.TaggedIsSmi(value), &miss); - assembler.GotoIf(assembler.TaggedIsSmi(cell_contents), &miss); - Node* expected_map = assembler.LoadMap(cell_contents); - Node* map = assembler.LoadMap(value); - assembler.GotoIf(assembler.WordNotEqual(expected_map, map), &miss); - break; - } - } - } - if (value_is_smi) { - assembler.StoreObjectFieldNoWriteBarrier(cell, PropertyCell::kValueOffset, - value); - } else { - assembler.StoreObjectField(cell, PropertyCell::kValueOffset, value); - } - } - - assembler.Return(value); - - assembler.Bind(&miss); - { - assembler.Comment("Miss"); - assembler.TailCallRuntime(Runtime::kStoreIC_Miss, context, value, slot, - vector, receiver, name); - } -} - void KeyedLoadSloppyArgumentsStub::GenerateAssembly( compiler::CodeAssemblerState* state) const { typedef CodeStubAssembler::Label Label; diff --git a/src/code-stubs.h b/src/code-stubs.h index e747c4356c0d..0cbad3cc5e5f 100644 --- a/src/code-stubs.h +++ b/src/code-stubs.h @@ -90,7 +90,6 @@ class Node; V(StringAdd) \ V(GetProperty) \ V(StoreFastElement) \ - V(StoreGlobal) \ V(StoreInterceptor) \ V(LoadIndexedInterceptor) \ V(GrowArrayElements) @@ -895,49 +894,6 @@ class KeyedStoreSloppyArgumentsStub : public TurboFanCodeStub { DEFINE_TURBOFAN_CODE_STUB(KeyedStoreSloppyArguments, TurboFanCodeStub); }; -class StoreGlobalStub : public TurboFanCodeStub { - public: - StoreGlobalStub(Isolate* isolate, PropertyCellType type, - Maybe constant_type) - : TurboFanCodeStub(isolate) { - PropertyCellConstantType encoded_constant_type = - constant_type.FromMaybe(PropertyCellConstantType::kSmi); - minor_key_ = CellTypeBits::encode(type) | - ConstantTypeBits::encode(encoded_constant_type); - } - - Code::Kind GetCodeKind() const override { return Code::HANDLER; } - ExtraICState GetExtraICState() const override { return Code::STORE_IC; } - - static Handle property_cell_placeholder(Isolate* isolate) { - return isolate->factory()->uninitialized_value(); - } - - Handle GetCodeCopyFromTemplate(Handle cell) { - FindAndReplacePattern pattern; - pattern.Add(handle(property_cell_placeholder(isolate())->map()), - isolate()->factory()->NewWeakCell(cell)); - return CodeStub::GetCodeCopy(pattern); - } - - PropertyCellType cell_type() const { - return CellTypeBits::decode(minor_key_); - } - - PropertyCellConstantType constant_type() const { - DCHECK(PropertyCellType::kConstantType == cell_type()); - return ConstantTypeBits::decode(minor_key_); - } - - private: - class CellTypeBits : public BitField {}; - class ConstantTypeBits - : public BitField {}; - - DEFINE_CALL_INTERFACE_DESCRIPTOR(StoreWithVector); - DEFINE_TURBOFAN_CODE_STUB(StoreGlobal, TurboFanCodeStub); -}; - class CallApiCallbackStub : public PlatformCodeStub { public: static const int kArgBits = 3; diff --git a/src/counters.h b/src/counters.h index 5a97455fa7ca..9dd4d4ef7d04 100644 --- a/src/counters.h +++ b/src/counters.h @@ -815,8 +815,8 @@ class RuntimeCallTimer final { V(StoreIC_StoreField) \ V(StoreIC_StoreFieldDH) \ V(StoreIC_StoreFieldStub) \ - V(StoreIC_StoreGlobal) \ - V(StoreIC_StoreGlobalTransition) \ + V(StoreIC_StoreGlobalDH) \ + V(StoreIC_StoreGlobalTransitionDH) \ V(StoreIC_StoreInterceptorStub) \ V(StoreIC_StoreNormalDH) \ V(StoreIC_StoreScriptContextFieldStub) \ diff --git a/src/ic/accessor-assembler.cc b/src/ic/accessor-assembler.cc index 24bf05054b05..bb82759386cd 100644 --- a/src/ic/accessor-assembler.cc +++ b/src/ic/accessor-assembler.cc @@ -580,7 +580,8 @@ void AccessorAssembler::HandleStoreICHandlerCase( const StoreICParameters* p, Node* handler, Label* miss, ElementSupport support_elements) { Label if_smi_handler(this), if_nonsmi_handler(this); - Label if_proto_handler(this), if_element_handler(this), call_handler(this); + Label if_proto_handler(this), if_element_handler(this), call_handler(this), + store_global(this); Branch(TaggedIsSmi(handler), &if_smi_handler, &if_nonsmi_handler); @@ -628,6 +629,7 @@ void AccessorAssembler::HandleStoreICHandlerCase( if (support_elements == kSupportElements) { GotoIf(IsTuple2Map(handler_map), &if_element_handler); } + GotoIf(IsWeakCellMap(handler_map), &store_global); Branch(IsCodeMap(handler_map), &call_handler, &if_proto_handler); } @@ -646,6 +648,66 @@ void AccessorAssembler::HandleStoreICHandlerCase( TailCallStub(descriptor, handler, p->context, p->receiver, p->name, p->value, p->slot, p->vector); } + + Bind(&store_global); + { + Node* cell = LoadWeakCellValue(handler, miss); + CSA_ASSERT(this, IsPropertyCell(cell)); + + // Load the payload of the global parameter cell. A hole indicates that + // the cell has been invalidated and that the store must be handled by the + // runtime. + Node* cell_contents = LoadObjectField(cell, PropertyCell::kValueOffset); + Node* details = + LoadAndUntagToWord32ObjectField(cell, PropertyCell::kDetailsOffset); + Node* type = DecodeWord32(details); + + Label constant(this), store(this), not_smi(this); + + GotoIf( + Word32Equal( + type, Int32Constant(static_cast(PropertyCellType::kConstant))), + &constant); + + GotoIf(IsTheHole(cell_contents), miss); + + GotoIf( + Word32Equal( + type, Int32Constant(static_cast(PropertyCellType::kMutable))), + &store); + CSA_ASSERT(this, + Word32Or(Word32Equal(type, + Int32Constant(static_cast( + PropertyCellType::kConstantType))), + Word32Equal(type, + Int32Constant(static_cast( + PropertyCellType::kUndefined))))); + + GotoIfNot(TaggedIsSmi(cell_contents), ¬_smi); + GotoIfNot(TaggedIsSmi(p->value), miss); + Goto(&store); + + Bind(¬_smi); + { + GotoIf(TaggedIsSmi(p->value), miss); + Node* expected_map = LoadMap(cell_contents); + Node* map = LoadMap(p->value); + GotoIfNot(WordEqual(expected_map, map), miss); + Goto(&store); + } + + Bind(&store); + { + StoreObjectField(cell, PropertyCell::kValueOffset, p->value); + Return(p->value); + } + + Bind(&constant); + { + GotoIfNot(WordEqual(cell_contents, p->value), miss); + Return(p->value); + } + } } void AccessorAssembler::HandleStoreICElementHandlerCase( diff --git a/src/ic/handler-configuration-inl.h b/src/ic/handler-configuration-inl.h index 2c4dcec06610..2b9dc04b5a93 100644 --- a/src/ic/handler-configuration-inl.h +++ b/src/ic/handler-configuration-inl.h @@ -92,15 +92,15 @@ Handle LoadHandler::LoadElement(Isolate* isolate, return handle(Smi::FromInt(config), isolate); } -Handle StoreHandler::StoreNormal(Isolate* isolate) { +Handle StoreHandler::StoreNormal(Isolate* isolate) { int config = KindBits::encode(kStoreNormal); return handle(Smi::FromInt(config), isolate); } -Handle StoreHandler::StoreField(Isolate* isolate, Kind kind, - int descriptor, FieldIndex field_index, - Representation representation, - bool extend_storage) { +Handle StoreHandler::StoreField(Isolate* isolate, Kind kind, + int descriptor, FieldIndex field_index, + Representation representation, + bool extend_storage) { StoreHandler::FieldRepresentation field_rep; switch (representation.kind()) { case Representation::kSmi: @@ -117,7 +117,7 @@ Handle StoreHandler::StoreField(Isolate* isolate, Kind kind, break; default: UNREACHABLE(); - return Handle::null(); + return Handle::null(); } DCHECK(kind == kStoreField || kind == kTransitionToField || @@ -134,26 +134,26 @@ Handle StoreHandler::StoreField(Isolate* isolate, Kind kind, return handle(Smi::FromInt(config), isolate); } -Handle StoreHandler::StoreField(Isolate* isolate, int descriptor, - FieldIndex field_index, - PropertyConstness constness, - Representation representation) { +Handle StoreHandler::StoreField(Isolate* isolate, int descriptor, + FieldIndex field_index, + PropertyConstness constness, + Representation representation) { DCHECK_IMPLIES(!FLAG_track_constant_fields, constness == kMutable); Kind kind = constness == kMutable ? kStoreField : kStoreConstField; return StoreField(isolate, kind, descriptor, field_index, representation, false); } -Handle StoreHandler::TransitionToField(Isolate* isolate, int descriptor, - FieldIndex field_index, - Representation representation, - bool extend_storage) { +Handle StoreHandler::TransitionToField(Isolate* isolate, int descriptor, + FieldIndex field_index, + Representation representation, + bool extend_storage) { return StoreField(isolate, kTransitionToField, descriptor, field_index, representation, extend_storage); } -Handle StoreHandler::TransitionToConstant(Isolate* isolate, - int descriptor) { +Handle StoreHandler::TransitionToConstant(Isolate* isolate, + int descriptor) { DCHECK(!FLAG_track_constant_fields); int config = StoreHandler::KindBits::encode(StoreHandler::kTransitionToConstant) | diff --git a/src/ic/handler-configuration.h b/src/ic/handler-configuration.h index 669c2cf85151..ab117d5c9bd5 100644 --- a/src/ic/handler-configuration.h +++ b/src/ic/handler-configuration.h @@ -191,32 +191,30 @@ class StoreHandler { static const int kFirstPrototypeIndex = 3; // Creates a Smi-handler for storing a field to fast object. - static inline Handle StoreField(Isolate* isolate, int descriptor, - FieldIndex field_index, - PropertyConstness constness, - Representation representation); + static inline Handle StoreField(Isolate* isolate, int descriptor, + FieldIndex field_index, + PropertyConstness constness, + Representation representation); // Creates a Smi-handler for storing a property to a slow object. - static inline Handle StoreNormal(Isolate* isolate); + static inline Handle StoreNormal(Isolate* isolate); // Creates a Smi-handler for transitioning store to a field. - static inline Handle TransitionToField(Isolate* isolate, - int descriptor, - FieldIndex field_index, - Representation representation, - bool extend_storage); + static inline Handle TransitionToField(Isolate* isolate, int descriptor, + FieldIndex field_index, + Representation representation, + bool extend_storage); // Creates a Smi-handler for transitioning store to a constant field (in this // case the only thing that needs to be done is an update of a map). - static inline Handle TransitionToConstant(Isolate* isolate, - int descriptor); + static inline Handle TransitionToConstant(Isolate* isolate, + int descriptor); private: - static inline Handle StoreField(Isolate* isolate, Kind kind, - int descriptor, - FieldIndex field_index, - Representation representation, - bool extend_storage); + static inline Handle StoreField(Isolate* isolate, Kind kind, + int descriptor, FieldIndex field_index, + Representation representation, + bool extend_storage); }; } // namespace internal diff --git a/src/ic/ic-inl.h b/src/ic/ic-inl.h index 26a36c19d5ab..8ac3bd99da74 100644 --- a/src/ic/ic-inl.h +++ b/src/ic/ic-inl.h @@ -86,7 +86,7 @@ Code* IC::target() const { bool IC::IsHandler(Object* object) { return (object->IsSmi() && (object != nullptr)) || object->IsTuple2() || - object->IsTuple3() || object->IsFixedArray() || + object->IsTuple3() || object->IsFixedArray() || object->IsWeakCell() || (object->IsCode() && Code::cast(object)->is_handler()); } diff --git a/src/ic/ic.cc b/src/ic/ic.cc index 298dc9008165..953a2b0d6d56 100644 --- a/src/ic/ic.cc +++ b/src/ic/ic.cc @@ -1705,7 +1705,7 @@ Handle StoreIC::StoreTransition(Handle receiver_map, Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate()); if (validity_cell.is_null()) { DCHECK_EQ(0, checks_count); - validity_cell = handle(Smi::FromInt(0), isolate()); + validity_cell = handle(Smi::kZero, isolate()); } Handle transition_cell = Map::WeakCellForMap(transition); @@ -1724,22 +1724,14 @@ Handle StoreIC::StoreTransition(Handle receiver_map, return handler_array; } -static Handle PropertyCellStoreHandler(Isolate* isolate, - Handle store_target, - Handle name, - Handle cell, - PropertyCellType type) { - auto constant_type = Nothing(); - if (type == PropertyCellType::kConstantType) { - constant_type = Just(cell->GetConstantType()); - } - StoreGlobalStub stub(isolate, type, constant_type); - auto code = stub.GetCodeCopyFromTemplate(cell); - // TODO(verwaest): Move caching of these NORMAL stubs outside as well. - HeapObject::UpdateMapCodeCache(store_target, name, code); - return code; +namespace { + +Handle StoreGlobal(Isolate* isolate, Handle cell) { + return isolate->factory()->NewWeakCell(cell); } +} // namespace + Handle StoreIC::GetMapIndependentHandler(LookupIterator* lookup) { DCHECK_NE(LookupIterator::JSPROXY, lookup->state()); @@ -1752,7 +1744,8 @@ Handle StoreIC::GetMapIndependentHandler(LookupIterator* lookup) { case LookupIterator::TRANSITION: { auto store_target = lookup->GetStoreTarget(); if (store_target->IsJSGlobalObject()) { - break; // Custom-compiled handler. + TRACE_HANDLER_STATS(isolate(), StoreIC_StoreGlobalTransitionDH); + return StoreGlobal(isolate(), lookup->transition_cell()); } // Currently not handled by CompileStoreTransition. if (!holder->HasFastProperties()) { @@ -1829,7 +1822,8 @@ Handle StoreIC::GetMapIndependentHandler(LookupIterator* lookup) { DCHECK_EQ(kData, lookup->property_details().kind()); if (lookup->is_dictionary_holder()) { if (holder->IsJSGlobalObject()) { - break; // Custom-compiled handler. + TRACE_HANDLER_STATS(isolate(), StoreIC_StoreGlobalDH); + return StoreGlobal(isolate(), lookup->GetPropertyCell()); } TRACE_HANDLER_STATS(isolate(), StoreIC_StoreNormalDH); DCHECK(holder.is_identical_to(receiver)); @@ -1877,24 +1871,6 @@ Handle StoreIC::CompileHandler(LookupIterator* lookup, DCHECK(!receiver->IsAccessCheckNeeded() || lookup->name()->IsPrivate()); switch (lookup->state()) { - case LookupIterator::TRANSITION: { - auto store_target = lookup->GetStoreTarget(); - if (store_target->IsJSGlobalObject()) { - TRACE_HANDLER_STATS(isolate(), StoreIC_StoreGlobalTransition); - Handle cell = lookup->transition_cell(); - cell->set_value(*value); - Handle code = - PropertyCellStoreHandler(isolate(), receiver, lookup->name(), cell, - PropertyCellType::kConstant); - cell->set_value(isolate()->heap()->the_hole_value()); - return code; - } - UNREACHABLE(); - } - - case LookupIterator::INTERCEPTOR: - UNREACHABLE(); - case LookupIterator::ACCESSOR: { DCHECK(holder->HasFastProperties()); Handle accessors = lookup->GetAccessors(); @@ -1940,20 +1916,9 @@ Handle StoreIC::CompileHandler(LookupIterator* lookup, } } - case LookupIterator::DATA: { - DCHECK(lookup->is_dictionary_holder()); - DCHECK(holder->IsJSGlobalObject()); - TRACE_HANDLER_STATS(isolate(), StoreIC_StoreGlobal); - DCHECK(holder.is_identical_to(receiver) || - receiver->map()->prototype() == *holder); - auto cell = lookup->GetPropertyCell(); - auto updated_type = - PropertyCell::UpdatedType(cell, value, lookup->property_details()); - auto code = PropertyCellStoreHandler(isolate(), receiver, - lookup->name(), cell, updated_type); - return code; - } - + case LookupIterator::DATA: + case LookupIterator::TRANSITION: + case LookupIterator::INTERCEPTOR: case LookupIterator::INTEGER_INDEXED_EXOTIC: case LookupIterator::ACCESS_CHECK: case LookupIterator::JSPROXY: