[ic] Restore PROPERTY key tracking in keyed ICs

Non-vectorized KeyedLoadICs used to remember whether they had seen Names
as keys; Crankshaft uses this information to avoid emitting elements
accesses which would always deopt. This CL restores that functionality
for vector ICs.

BUG=chromium:594183
LOG=y
R=mvstanton@chromium.org

Review URL: https://codereview.chromium.org/1912593002

Cr-Commit-Position: refs/heads/master@{#35706}

src/ic/ic.cc

@@ -501,13 +501,21 @@ static bool MigrateDeprecated(Handle<Object> object) {
   return true;
 }
 
-
-void IC::ConfigureVectorState(IC::State new_state) {
+void IC::ConfigureVectorState(IC::State new_state, Handle<Object> key) {
   DCHECK(UseVector());
   if (new_state == PREMONOMORPHIC) {
     nexus()->ConfigurePremonomorphic();
   } else if (new_state == MEGAMORPHIC) {
+    if (kind() == Code::LOAD_IC || kind() == Code::STORE_IC) {
       nexus()->ConfigureMegamorphic();
+    } else if (kind() == Code::KEYED_LOAD_IC) {
+      KeyedLoadICNexus* nexus = casted_nexus<KeyedLoadICNexus>();
+      nexus->ConfigureMegamorphicKeyed(key->IsName() ? PROPERTY : ELEMENT);
+    } else {
+      DCHECK(kind() == Code::KEYED_STORE_IC);
+      KeyedStoreICNexus* nexus = casted_nexus<KeyedStoreICNexus>();
+      nexus->ConfigureMegamorphicKeyed(key->IsName() ? PROPERTY : ELEMENT);
+    }
   } else {
     UNREACHABLE();
   }
@@ -590,7 +598,7 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
     // Rewrite to the generic keyed load stub.
     if (FLAG_use_ic) {
       DCHECK(UseVector());
-      ConfigureVectorState(MEGAMORPHIC);
+      ConfigureVectorState(MEGAMORPHIC, name);
       TRACE_GENERIC_IC(isolate(), "LoadIC", "name as array index");
       TRACE_IC("LoadIC", name);
     }
@@ -777,7 +785,7 @@ void IC::PatchCache(Handle<Name> name, Handle<Code> code) {
         CopyICToMegamorphicCache(name);
       }
       DCHECK(UseVector());
-      ConfigureVectorState(MEGAMORPHIC);
+      ConfigureVectorState(MEGAMORPHIC, name);
     // Fall through.
     case MEGAMORPHIC:
       UpdateMegamorphicCache(*receiver_map(), *name, *code);
@@ -875,7 +883,7 @@ void LoadIC::UpdateCaches(LookupIterator* lookup) {
   if (state() == UNINITIALIZED) {
     // This is the first time we execute this inline cache. Set the target to
     // the pre monomorphic stub to delay setting the monomorphic state.
-    ConfigureVectorState(PREMONOMORPHIC);
+    ConfigureVectorState(PREMONOMORPHIC, Handle<Object>());
     TRACE_IC("LoadIC", lookup->name());
     return;
   }
@@ -1250,7 +1258,7 @@ MaybeHandle<Object> KeyedLoadIC::Load(Handle<Object> object,
   }
 
   if (!is_vector_set()) {
-    ConfigureVectorState(MEGAMORPHIC);
+    ConfigureVectorState(MEGAMORPHIC, key);
     TRACE_GENERIC_IC(isolate(), "KeyedLoadIC", "set generic");
   }
   TRACE_IC("LoadIC", key);
@@ -1342,7 +1350,7 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
     // Rewrite to the generic keyed store stub.
     if (FLAG_use_ic) {
       DCHECK(UseVector());
-      ConfigureVectorState(MEGAMORPHIC);
+      ConfigureVectorState(MEGAMORPHIC, name);
       TRACE_IC("StoreIC", name);
       TRACE_GENERIC_IC(isolate(), "StoreIC", "name as array index");
     }
@@ -1458,7 +1466,7 @@ void StoreIC::UpdateCaches(LookupIterator* lookup, Handle<Object> value,
   if (state() == UNINITIALIZED) {
     // This is the first time we execute this inline cache. Set the target to
     // the pre monomorphic stub to delay setting the monomorphic state.
-    ConfigureVectorState(PREMONOMORPHIC);
+    ConfigureVectorState(PREMONOMORPHIC, Handle<Object>());
     TRACE_IC("StoreIC", lookup->name());
     return;
   }
@@ -1878,7 +1886,7 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
                        JSReceiver::MAY_BE_STORE_FROM_KEYED),
         Object);
     if (!is_vector_set()) {
-      ConfigureVectorState(MEGAMORPHIC);
+      ConfigureVectorState(MEGAMORPHIC, key);
       TRACE_GENERIC_IC(isolate(), "KeyedStoreIC",
                        "unhandled internalized string key");
       TRACE_IC("StoreIC", key);
@@ -1951,7 +1959,7 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
   }
 
   if (!is_vector_set()) {
-    ConfigureVectorState(MEGAMORPHIC);
+    ConfigureVectorState(MEGAMORPHIC, key);
     TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "set generic");
   }
   TRACE_IC("StoreIC", key);

src/ic/ic.h

@@ -106,7 +106,7 @@ class IC {
   }
 
   // Configure for most states.
-  void ConfigureVectorState(IC::State new_state);
+  void ConfigureVectorState(IC::State new_state, Handle<Object> key);
   // Configure the vector for MONOMORPHIC.
   void ConfigureVectorState(Handle<Name> name, Handle<Map> map,
                             Handle<Code> handler);
@@ -267,10 +267,6 @@ class CallIC : public IC {
 
 class LoadIC : public IC {
  public:
-  static ExtraICState ComputeExtraICState(TypeofMode typeof_mode) {
-    return LoadICState(typeof_mode).GetExtraICState();
-  }
-
   TypeofMode typeof_mode() const {
     return LoadICState::GetTypeofMode(extra_ic_state());
   }
@@ -320,20 +316,6 @@ class LoadIC : public IC {
 
 class KeyedLoadIC : public LoadIC {
  public:
-  // ExtraICState bits (building on IC)
-  class IcCheckTypeField
-      : public BitField<IcCheckType, LoadICState::kNextBitFieldOffset, 1> {};
-
-  static ExtraICState ComputeExtraICState(TypeofMode typeof_mode,
-                                          IcCheckType key_type) {
-    return LoadICState(typeof_mode).GetExtraICState() |
-           IcCheckTypeField::encode(key_type);
-  }
-
-  static IcCheckType GetKeyType(ExtraICState extra_state) {
-    return IcCheckTypeField::decode(extra_state);
-  }
-
   KeyedLoadIC(FrameDepth depth, Isolate* isolate,
               KeyedLoadICNexus* nexus = NULL)
       : LoadIC(depth, isolate, nexus) {
@@ -366,10 +348,6 @@ class KeyedLoadIC : public LoadIC {
 
 class StoreIC : public IC {
  public:
-  static ExtraICState ComputeExtraICState(LanguageMode flag) {
-    return StoreICState(flag).GetExtraICState();
-  }
-
   StoreIC(FrameDepth depth, Isolate* isolate, FeedbackNexus* nexus = NULL)
       : IC(depth, isolate, nexus) {
     DCHECK(IsStoreStub());
@@ -424,22 +402,6 @@ enum KeyedStoreIncrementLength { kDontIncrementLength, kIncrementLength };
 
 class KeyedStoreIC : public StoreIC {
  public:
-  // ExtraICState bits (building on IC)
-  // ExtraICState bits
-  // When more language modes are added, these BitFields need to move too.
-  STATIC_ASSERT(i::LANGUAGE_END == 3);
-  class ExtraICStateKeyedAccessStoreMode
-      : public BitField<KeyedAccessStoreMode, 3, 3> {};  // NOLINT
-
-  class IcCheckTypeField : public BitField<IcCheckType, 6, 1> {};
-
-  static ExtraICState ComputeExtraICState(LanguageMode flag,
-                                          KeyedAccessStoreMode mode) {
-    return StoreICState(flag).GetExtraICState() |
-           ExtraICStateKeyedAccessStoreMode::encode(mode) |
-           IcCheckTypeField::encode(ELEMENT);
-  }
-
   KeyedAccessStoreMode GetKeyedAccessStoreMode() {
     return casted_nexus<KeyedStoreICNexus>()->GetKeyedAccessStoreMode();
   }

src/type-feedback-vector.cc

@@ -340,6 +340,10 @@ void FeedbackNexus::ConfigurePremonomorphic() {
 
 
 void FeedbackNexus::ConfigureMegamorphic() {
+  // Keyed ICs must use ConfigureMegamorphicKeyed.
+  DCHECK_NE(FeedbackVectorSlotKind::KEYED_LOAD_IC, vector()->GetKind(slot()));
+  DCHECK_NE(FeedbackVectorSlotKind::KEYED_STORE_IC, vector()->GetKind(slot()));
+
   Isolate* isolate = GetIsolate();
   SetFeedback(*TypeFeedbackVector::MegamorphicSentinel(isolate),
               SKIP_WRITE_BARRIER);
@@ -347,6 +351,21 @@ void FeedbackNexus::ConfigureMegamorphic() {
                    SKIP_WRITE_BARRIER);
 }
 
+void KeyedLoadICNexus::ConfigureMegamorphicKeyed(IcCheckType property_type) {
+  Isolate* isolate = GetIsolate();
+  SetFeedback(*TypeFeedbackVector::MegamorphicSentinel(isolate),
+              SKIP_WRITE_BARRIER);
+  SetFeedbackExtra(Smi::FromInt(static_cast<int>(property_type)),
+                   SKIP_WRITE_BARRIER);
+}
+
+void KeyedStoreICNexus::ConfigureMegamorphicKeyed(IcCheckType property_type) {
+  Isolate* isolate = GetIsolate();
+  SetFeedback(*TypeFeedbackVector::MegamorphicSentinel(isolate),
+              SKIP_WRITE_BARRIER);
+  SetFeedbackExtra(Smi::FromInt(static_cast<int>(property_type)),
+                   SKIP_WRITE_BARRIER);
+}
 
 InlineCacheState LoadICNexus::StateFromFeedback() const {
   Isolate* isolate = GetIsolate();
@@ -824,10 +843,20 @@ KeyedAccessStoreMode KeyedStoreICNexus::GetKeyedAccessStoreMode() const {
   return mode;
 }
 
+IcCheckType KeyedLoadICNexus::GetKeyType() const {
+  Object* feedback = GetFeedback();
+  if (feedback == *TypeFeedbackVector::MegamorphicSentinel(GetIsolate())) {
+    return static_cast<IcCheckType>(Smi::cast(GetFeedbackExtra())->value());
+  }
+  return IsPropertyNameFeedback(feedback) ? PROPERTY : ELEMENT;
+}
 
 IcCheckType KeyedStoreICNexus::GetKeyType() const {
-  // The structure of the vector slots tells us the type.
-  return GetFeedback()->IsName() ? PROPERTY : ELEMENT;
+  Object* feedback = GetFeedback();
+  if (feedback == *TypeFeedbackVector::MegamorphicSentinel(GetIsolate())) {
+    return static_cast<IcCheckType>(Smi::cast(GetFeedbackExtra())->value());
+  }
+  return IsPropertyNameFeedback(feedback) ? PROPERTY : ELEMENT;
 }
 }  // namespace internal
 }  // namespace v8

src/type-feedback-vector.h

@@ -475,6 +475,9 @@ class KeyedLoadICNexus : public FeedbackNexus {
   void ConfigurePolymorphic(Handle<Name> name, MapHandleList* maps,
                             CodeHandleList* handlers);
 
+  void ConfigureMegamorphicKeyed(IcCheckType property_type);
+
+  IcCheckType GetKeyType() const;
   InlineCacheState StateFromFeedback() const override;
   Name* FindFirstName() const override;
 };
@@ -531,6 +534,7 @@ class KeyedStoreICNexus : public FeedbackNexus {
   void ConfigurePolymorphic(MapHandleList* maps,
                             MapHandleList* transitioned_maps,
                             CodeHandleList* handlers);
+  void ConfigureMegamorphicKeyed(IcCheckType property_type);
 
   KeyedAccessStoreMode GetKeyedAccessStoreMode() const;
   IcCheckType GetKeyType() const;

src/type-info.cc

@@ -297,7 +297,7 @@ void TypeFeedbackOracle::KeyedPropertyReceiverTypes(
     KeyedLoadICNexus nexus(feedback_vector_, slot);
     CollectReceiverTypes(&nexus, receiver_types);
     *is_string = HasOnlyStringMaps(receiver_types);
-    *key_type = nexus.FindFirstName() != NULL ? PROPERTY : ELEMENT;
+    *key_type = nexus.GetKeyType();
   }
 }
 

test/mjsunit/regress/regress-crbug-594183.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+var global = {}
+
+var fish = [
+  {'name': 'foo'},
+  {'name': 'bar'},
+];
+
+for (var i = 0; i < fish.length; i++) {
+  global[fish[i].name] = 1;
+}
+
+function load() {
+  var sum = 0;
+  for (var i = 0; i < fish.length; i++) {
+    var name = fish[i].name;
+    sum += global[name];
+  }
+  return sum;
+}
+
+load();
+load();
+%OptimizeFunctionOnNextCall(load);
+load();
+assertOptimized(load);
+
+function store() {
+  for (var i = 0; i < fish.length; i++) {
+    var name = fish[i].name;
+    global[name] = 1;
+  }
+}
+
+store();
+store();
+%OptimizeFunctionOnNextCall(store);
+store();
+assertOptimized(store);
+
+// Regression test for KeyedStoreIC bug: would use PROPERTY mode erroneously.
+
+function store_element(obj, key) {
+  obj[key] = 0;
+}
+
+var o1 = new Array(3);
+var o2 = new Array(3);
+o2.o2 = "o2";
+var o3 = new Array(3);
+o3.o3 = "o3";
+var o4 = new Array(3);
+o4.o4 = "o4";
+var o5 = new Array(3);
+o5.o5 = "o5";
+// Make the KeyedStoreIC megamorphic.
+store_element(o1, 0);  // Premonomorphic
+store_element(o1, 0);  // Monomorphic
+store_element(o2, 0);  // 2-way polymorphic.
+store_element(o3, 0);  // 3-way polymorphic.
+store_element(o4, 0);  // 4-way polymorphic.
+store_element(o5, 0);  // Megamorphic.
+
+function inferrable_store(key) {
+  store_element(o5, key);
+}
+
+inferrable_store(0);
+inferrable_store(0);
+%OptimizeFunctionOnNextCall(inferrable_store);
+inferrable_store(0);
+assertOptimized(inferrable_store);
+// If |inferrable_store| emitted a generic keyed store, it won't deopt upon
+// seeing a property name key. It should have inferred a receiver map and
+// emitted an elements store, however.
+inferrable_store("deopt");
+assertUnoptimized(inferrable_store);