From ed585c083c919d3136d42ca51a3439c4507fc176 Mon Sep 17 00:00:00 2001 From: "jkummerow@chromium.org" Date: Fri, 10 Oct 2014 13:27:52 +0000 Subject: [PATCH] Fix type feedback for name-keyed stores BUG=chromium:422212 LOG=n R=mvstanton@chromium.org Review URL: https://codereview.chromium.org/648703002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24529 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/ast.h | 10 ++++++++++ src/hydrogen.cc | 10 ++++++++-- src/ic/ic-compiler.cc | 14 +++++++------- src/ic/ic-compiler.h | 3 --- src/ic/ic.h | 6 ++++++ src/objects.h | 3 +++ src/type-info.cc | 17 +++++++++++------ src/type-info.h | 7 +++++-- src/typing.cc | 12 +++++++++--- 9 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/ast.h b/src/ast.h index 5386d7d0f..a3fb9626b 100644 --- a/src/ast.h +++ b/src/ast.h @@ -376,6 +376,10 @@ class Expression : public AstNode { UNREACHABLE(); return STANDARD_STORE; } + virtual IcCheckType GetKeyType() { + UNREACHABLE(); + return ELEMENT; + } // TODO(rossberg): this should move to its own AST node eventually. virtual void RecordToBooleanTypeFeedback(TypeFeedbackOracle* oracle); @@ -2097,10 +2101,12 @@ class CountOperation FINAL : public Expression { virtual SmallMapList* GetReceiverTypes() OVERRIDE { return &receiver_types_; } + virtual IcCheckType GetKeyType() OVERRIDE { return key_type_; } virtual KeyedAccessStoreMode GetStoreMode() OVERRIDE { return store_mode_; } Type* type() const { return type_; } + void set_key_type(IcCheckType type) { key_type_ = type; } void set_store_mode(KeyedAccessStoreMode mode) { store_mode_ = mode; } void set_type(Type* type) { type_ = type; } @@ -2127,6 +2133,7 @@ class CountOperation FINAL : public Expression { private: Token::Value op_; bool is_prefix_ : 1; + IcCheckType key_type_ : 1; KeyedAccessStoreMode store_mode_ : 5; // Windows treats as signed, // must have extra bit. Type* type_; @@ -2239,10 +2246,12 @@ class Assignment FINAL : public Expression { virtual SmallMapList* GetReceiverTypes() OVERRIDE { return &receiver_types_; } + virtual IcCheckType GetKeyType() OVERRIDE { return key_type_; } virtual KeyedAccessStoreMode GetStoreMode() OVERRIDE { return store_mode_; } void set_is_uninitialized(bool b) { is_uninitialized_ = b; } + void set_key_type(IcCheckType key_type) { key_type_ = key_type; } void set_store_mode(KeyedAccessStoreMode mode) { store_mode_ = mode; } protected: @@ -2267,6 +2276,7 @@ class Assignment FINAL : public Expression { Expression* value_; BinaryOperation* binary_operation_; bool is_uninitialized_ : 1; + IcCheckType key_type_ : 1; KeyedAccessStoreMode store_mode_ : 5; // Windows treats as signed, // must have extra bit. SmallMapList receiver_types_; diff --git a/src/hydrogen.cc b/src/hydrogen.cc index 2c4435cc4..e95694749 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -7174,8 +7174,14 @@ HValue* HOptimizedGraphBuilder::HandleKeyedElementAccess( bool monomorphic = ComputeReceiverTypes(expr, obj, &types, zone()); bool force_generic = false; - if (access_type == STORE && - (monomorphic || (types != NULL && !types->is_empty()))) { + if (access_type == STORE && expr->GetKeyType() == PROPERTY) { + // Non-Generic accesses assume that elements are being accessed, and will + // deopt for non-index keys, which the IC knows will occur. + // TODO(jkummerow): Consider adding proper support for property accesses. + force_generic = true; + monomorphic = false; + } else if (access_type == STORE && + (monomorphic || (types != NULL && !types->is_empty()))) { // Stores can't be mono/polymorphic if their prototype chain has dictionary // elements. However a receiver map that has dictionary elements itself // should be left to normal mono/poly behavior (the other maps may benefit diff --git a/src/ic/ic-compiler.cc b/src/ic/ic-compiler.cc index aeae4ba90..24715645d 100644 --- a/src/ic/ic-compiler.cc +++ b/src/ic/ic-compiler.cc @@ -57,6 +57,13 @@ Handle PropertyICCompiler::ComputeMonomorphic( CacheHolderFlag flag; Handle stub_holder = IC::GetICCacheHolder(*type, isolate, &flag); + if (kind == Code::KEYED_STORE_IC) { + // Always set the "property" bit. + extra_ic_state = + KeyedStoreIC::IcCheckTypeField::update(extra_ic_state, PROPERTY); + DCHECK(STANDARD_STORE == + KeyedStoreIC::GetKeyedAccessStoreMode(extra_ic_state)); + } Handle ic; // There are multiple string maps that all use the same prototype. That @@ -68,13 +75,6 @@ Handle PropertyICCompiler::ComputeMonomorphic( if (!ic.is_null()) return ic; } -#ifdef DEBUG - if (kind == Code::KEYED_STORE_IC) { - DCHECK(STANDARD_STORE == - KeyedStoreIC::GetKeyedAccessStoreMode(extra_ic_state)); - } -#endif - PropertyICCompiler ic_compiler(isolate, kind, extra_ic_state, flag); ic = ic_compiler.CompileMonomorphic(type, handler, name, PROPERTY); diff --git a/src/ic/ic-compiler.h b/src/ic/ic-compiler.h index 3b12157a0..97c07d0ec 100644 --- a/src/ic/ic-compiler.h +++ b/src/ic/ic-compiler.h @@ -11,9 +11,6 @@ namespace v8 { namespace internal { -enum IcCheckType { ELEMENT, PROPERTY }; - - class PropertyICCompiler : public PropertyAccessCompiler { public: // Finds the Code object stored in the Heap::non_monomorphic_cache(). diff --git a/src/ic/ic.h b/src/ic/ic.h index 15479371c..7a6ee616b 100644 --- a/src/ic/ic.h +++ b/src/ic/ic.h @@ -542,6 +542,8 @@ class KeyedStoreIC : public StoreIC { class ExtraICStateKeyedAccessStoreMode : public BitField {}; // NOLINT + class IcCheckTypeField : public BitField {}; + static ExtraICState ComputeExtraICState(StrictMode flag, KeyedAccessStoreMode mode) { return StrictModeState::encode(flag) | @@ -553,6 +555,10 @@ class KeyedStoreIC : public StoreIC { return ExtraICStateKeyedAccessStoreMode::decode(extra_state); } + static IcCheckType GetKeyType(ExtraICState extra_state) { + return IcCheckTypeField::decode(extra_state); + } + KeyedStoreIC(FrameDepth depth, Isolate* isolate) : StoreIC(depth, isolate) { DCHECK(target()->is_keyed_store_stub()); } diff --git a/src/objects.h b/src/objects.h index 1faae8613..277df06cc 100644 --- a/src/objects.h +++ b/src/objects.h @@ -232,6 +232,9 @@ static inline bool IsGrowStoreMode(KeyedAccessStoreMode store_mode) { } +enum IcCheckType { ELEMENT, PROPERTY }; + + // Setter that skips the write barrier if mode is SKIP_WRITE_BARRIER. enum WriteBarrierMode { SKIP_WRITE_BARRIER, UPDATE_WRITE_BARRIER }; diff --git a/src/type-info.cc b/src/type-info.cc index f9947799b..6594bdb59 100644 --- a/src/type-info.cc +++ b/src/type-info.cc @@ -101,16 +101,21 @@ byte TypeFeedbackOracle::ForInType(FeedbackVectorSlot feedback_vector_slot) { } -KeyedAccessStoreMode TypeFeedbackOracle::GetStoreMode( - TypeFeedbackId ast_id) { +void TypeFeedbackOracle::GetStoreModeAndKeyType( + TypeFeedbackId ast_id, KeyedAccessStoreMode* store_mode, + IcCheckType* key_type) { Handle maybe_code = GetInfo(ast_id); if (maybe_code->IsCode()) { Handle code = Handle::cast(maybe_code); if (code->kind() == Code::KEYED_STORE_IC) { - return KeyedStoreIC::GetKeyedAccessStoreMode(code->extra_ic_state()); + ExtraICState extra_ic_state = code->extra_ic_state(); + *store_mode = KeyedStoreIC::GetKeyedAccessStoreMode(extra_ic_state); + *key_type = KeyedStoreIC::GetKeyType(extra_ic_state); + return; } } - return STANDARD_STORE; + *store_mode = STANDARD_STORE; + *key_type = ELEMENT; } @@ -274,10 +279,10 @@ void TypeFeedbackOracle::AssignmentReceiverTypes( void TypeFeedbackOracle::KeyedAssignmentReceiverTypes( TypeFeedbackId id, SmallMapList* receiver_types, - KeyedAccessStoreMode* store_mode) { + KeyedAccessStoreMode* store_mode, IcCheckType* key_type) { receiver_types->Clear(); CollectReceiverTypes(id, receiver_types); - *store_mode = GetStoreMode(id); + GetStoreModeAndKeyType(id, store_mode, key_type); } diff --git a/src/type-info.h b/src/type-info.h index 5c99e71d5..5b13810d9 100644 --- a/src/type-info.h +++ b/src/type-info.h @@ -36,7 +36,9 @@ class TypeFeedbackOracle: public ZoneObject { // be possible. byte ForInType(FeedbackVectorSlot feedback_vector_slot); - KeyedAccessStoreMode GetStoreMode(TypeFeedbackId id); + void GetStoreModeAndKeyType(TypeFeedbackId id, + KeyedAccessStoreMode* store_mode, + IcCheckType* key_type); void PropertyReceiverTypes(TypeFeedbackId id, Handle name, SmallMapList* receiver_types); @@ -48,7 +50,8 @@ class TypeFeedbackOracle: public ZoneObject { SmallMapList* receiver_types); void KeyedAssignmentReceiverTypes(TypeFeedbackId id, SmallMapList* receiver_types, - KeyedAccessStoreMode* store_mode); + KeyedAccessStoreMode* store_mode, + IcCheckType* key_type); void CountReceiverTypes(TypeFeedbackId id, SmallMapList* receiver_types); diff --git a/src/typing.cc b/src/typing.cc index a94cf623e..1cfaf64f6 100644 --- a/src/typing.cc +++ b/src/typing.cc @@ -444,9 +444,11 @@ void AstTyper::VisitAssignment(Assignment* expr) { oracle()->AssignmentReceiverTypes(id, name, expr->GetReceiverTypes()); } else { KeyedAccessStoreMode store_mode; - oracle()->KeyedAssignmentReceiverTypes( - id, expr->GetReceiverTypes(), &store_mode); + IcCheckType key_type; + oracle()->KeyedAssignmentReceiverTypes(id, expr->GetReceiverTypes(), + &store_mode, &key_type); expr->set_store_mode(store_mode); + expr->set_key_type(key_type); } } } @@ -587,7 +589,11 @@ void AstTyper::VisitUnaryOperation(UnaryOperation* expr) { void AstTyper::VisitCountOperation(CountOperation* expr) { // Collect type feedback. TypeFeedbackId store_id = expr->CountStoreFeedbackId(); - expr->set_store_mode(oracle()->GetStoreMode(store_id)); + KeyedAccessStoreMode store_mode; + IcCheckType key_type; + oracle()->GetStoreModeAndKeyType(store_id, &store_mode, &key_type); + expr->set_store_mode(store_mode); + expr->set_key_type(key_type); oracle()->CountReceiverTypes(store_id, expr->GetReceiverTypes()); expr->set_type(oracle()->CountType(expr->CountBinOpFeedbackId())); // TODO(rossberg): merge the count type with the generic expression type. -- 2.34.1