Revert "[ic] Don't transition to premonomorphic state"

This reverts commit 159df248.

Reason for revert: Breaks large-classes-properties test (https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket.appspot.com/8906338563361079200/+/steps/Bisect_159df248/0/steps/Retry_-_isolates/0/logs/large-classes-properties/0)

Original change's description:
> [ic] Don't transition to premonomorphic state
> 
> We used to use premonomorphic state to delay initializing the ICs.
> This optimization was to avoid the cost of setting up handlers if the
> code executed only once. With lazy feedback allocation we no longer
> need this.
> 
> This cl also renames LoadIC_Uninitialized to LoadIC_Nofeedback and
> StoreIC_Uninitialized to StoreIC_Nofeedback since we now miss to
> runtime in the uninitialized state and use the builtin when there
> is no feedback.
> 
> 
> Change-Id: I1633e61ea74664da51348e362c34c47a017a264a
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1683525
> Commit-Queue: Mythri Alle <mythria@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#63020}

TBR=mythria@chromium.org,verwaest@chromium.org

Change-Id: I4fad4e8b881d4a3f8d12149e1797b217a317eaee
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1730995Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63023}

src/builtins/builtins-definitions.h

@@ -222,9 +222,9 @@ namespace internal {
   TFH(LoadIC_Slow, LoadWithVector)                                             \
   TFH(LoadIC_StringLength, LoadWithVector)                                     \
   TFH(LoadIC_StringWrapperLength, LoadWithVector)                              \
-  TFH(LoadIC_Nofeedback, Load)                                                 \
+  TFH(LoadIC_Uninitialized, LoadWithVector)                                    \
   TFH(StoreGlobalIC_Slow, StoreWithVector)                                     \
-  TFH(StoreIC_Nofeedback, Store)                                               \
+  TFH(StoreIC_Uninitialized, StoreWithVector)                                  \
   TFH(StoreInArrayLiteralIC_Slow, StoreWithVector)                             \
   TFH(KeyedLoadIC_SloppyArguments, LoadWithVector)                             \
   TFH(LoadIndexedInterceptorIC, LoadWithVector)                                \

src/builtins/builtins-handler-gen.cc

@@ -66,9 +66,9 @@ void Builtins::Generate_KeyedStoreIC_Megamorphic(
   KeyedStoreGenericGenerator::Generate(state);
 }
 
-void Builtins::Generate_StoreIC_Nofeedback(
+void Builtins::Generate_StoreIC_Uninitialized(
     compiler::CodeAssemblerState* state) {
-  StoreICNofeedbackGenerator::Generate(state);
+  StoreICUninitializedGenerator::Generate(state);
 }
 
 // TODO(mythria): Check if we can remove feedback vector and slot parameters in

src/builtins/builtins-ic-gen.cc

@@ -23,7 +23,7 @@ namespace internal {
 IC_BUILTIN(LoadIC)
 IC_BUILTIN(LoadIC_Megamorphic)
 IC_BUILTIN(LoadIC_Noninlined)
-IC_BUILTIN(LoadIC_Nofeedback)
+IC_BUILTIN(LoadIC_Uninitialized)
 IC_BUILTIN(LoadICTrampoline)
 IC_BUILTIN(LoadICTrampoline_Megamorphic)
 IC_BUILTIN(KeyedLoadIC)

src/ic/accessor-assembler.cc

@@ -2524,8 +2524,8 @@ void AccessorAssembler::LoadIC_BytecodeHandler(const LazyLoadICParameters* p,
     Comment("LoadIC_BytecodeHandler_nofeedback");
     // Call into the stub that implements the non-inlined parts of LoadIC.
     exit_point->ReturnCallStub(
-        Builtins::CallableFor(isolate(), Builtins::kLoadIC_Nofeedback),
-        p->context(), p->receiver(), p->name(), p->slot());
+        Builtins::CallableFor(isolate(), Builtins::kLoadIC_Uninitialized),
+        p->context(), p->receiver(), p->name(), p->slot(), p->vector());
   }
 
   BIND(&miss);
@@ -2589,6 +2589,8 @@ void AccessorAssembler::LoadIC_Noninlined(const LoadICParameters* p,
                                           TVariable<MaybeObject>* var_handler,
                                           Label* if_handler, Label* miss,
                                           ExitPoint* exit_point) {
+  Label try_uninitialized(this, Label::kDeferred);
+
   // Neither deprecated map nor monomorphic. These cases are handled in the
   // bytecode handler.
   CSA_ASSERT(this, Word32BinaryNot(IsDeprecatedMap(receiver_map)));
@@ -2599,20 +2601,41 @@ void AccessorAssembler::LoadIC_Noninlined(const LoadICParameters* p,
   {
     // Check megamorphic case.
     GotoIfNot(WordEqual(feedback, LoadRoot(RootIndex::kmegamorphic_symbol)),
-              miss);
+              &try_uninitialized);
 
     TryProbeStubCache(isolate()->load_stub_cache(), p->receiver(), p->name(),
                       if_handler, var_handler, miss);
   }
+
+  BIND(&try_uninitialized);
+  {
+    // Check uninitialized case.
+    GotoIfNot(WordEqual(feedback, LoadRoot(RootIndex::kuninitialized_symbol)),
+              miss);
+    exit_point->ReturnCallStub(
+        Builtins::CallableFor(isolate(), Builtins::kLoadIC_Uninitialized),
+        p->context(), p->receiver(), p->name(), p->slot(), p->vector());
+  }
 }
 
-void AccessorAssembler::LoadIC_Nofeedback(const LoadICParameters* p) {
-  Label miss(this, Label::kDeferred);
+void AccessorAssembler::LoadIC_Uninitialized(const LoadICParameters* p) {
+  Label miss(this, Label::kDeferred),
+      check_function_prototype(this);
   Node* receiver = p->receiver();
   GotoIf(TaggedIsSmi(receiver), &miss);
   Node* receiver_map = LoadMap(receiver);
   Node* instance_type = LoadMapInstanceType(receiver_map);
 
+  GotoIf(IsUndefined(p->vector()), &check_function_prototype);
+  // Optimistically write the state transition to the vector.
+  StoreFeedbackVectorSlot(p->vector(), p->slot(),
+                          LoadRoot(RootIndex::kpremonomorphic_symbol),
+                          SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
+  StoreWeakReferenceInFeedbackVector(p->vector(), p->slot(), receiver_map,
+                                     kTaggedSize, SMI_PARAMETERS);
+  Goto(&check_function_prototype);
+
+  BIND(&check_function_prototype);
   {
     // Special case for Function.prototype load, because it's very common
     // for ICs that are only executed once (MyFunc.prototype.foo = ...).
@@ -2632,6 +2655,15 @@ void AccessorAssembler::LoadIC_Nofeedback(const LoadICParameters* p) {
 
   BIND(&miss);
   {
+    Label call_runtime(this, Label::kDeferred);
+    GotoIf(IsUndefined(p->vector()), &call_runtime);
+    // Undo the optimistic state transition.
+    StoreFeedbackVectorSlot(p->vector(), p->slot(),
+                            LoadRoot(RootIndex::kuninitialized_symbol),
+                            SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
+    Goto(&call_runtime);
+
+    BIND(&call_runtime);
     TailCallRuntime(Runtime::kLoadIC_Miss, p->context(), p->receiver(),
                     p->name(), p->slot(), p->vector());
   }
@@ -2740,7 +2772,6 @@ void AccessorAssembler::KeyedLoadIC(const LoadICParameters* p,
   TVARIABLE(MaybeObject, var_handler);
   Label if_handler(this, &var_handler), try_polymorphic(this, Label::kDeferred),
       try_megamorphic(this, Label::kDeferred),
-      try_uninitialized(this, Label::kDeferred),
       try_polymorphic_name(this, Label::kDeferred),
       miss(this, Label::kDeferred), generic(this, Label::kDeferred);
 
@@ -2777,7 +2808,7 @@ void AccessorAssembler::KeyedLoadIC(const LoadICParameters* p,
     // Check megamorphic case.
     Comment("KeyedLoadIC_try_megamorphic");
     Branch(WordEqual(strong_feedback, LoadRoot(RootIndex::kmegamorphic_symbol)),
-           &generic, &try_uninitialized);
+           &generic, &try_polymorphic_name);
   }
 
   BIND(&generic);
@@ -2790,15 +2821,6 @@ void AccessorAssembler::KeyedLoadIC(const LoadICParameters* p,
                     p->vector());
   }
 
-  BIND(&try_uninitialized);
-  {
-    // Check uninitialized case.
-    Comment("KeyedLoadIC_try_uninitialized");
-    Branch(
-        WordEqual(strong_feedback, LoadRoot(RootIndex::kuninitialized_symbol)),
-        &miss, &try_polymorphic_name);
-  }
-
   BIND(&try_polymorphic_name);
   {
     // We might have a name in feedback, and a weak fixed array in the next
@@ -2992,7 +3014,8 @@ void AccessorAssembler::StoreIC(const StoreICParameters* p) {
   Label if_handler(this, &var_handler),
       if_handler_from_stub_cache(this, &var_handler, Label::kDeferred),
       try_polymorphic(this, Label::kDeferred),
-      try_megamorphic(this, Label::kDeferred), miss(this, Label::kDeferred),
+      try_megamorphic(this, Label::kDeferred),
+      try_uninitialized(this, Label::kDeferred), miss(this, Label::kDeferred),
       no_feedback(this, Label::kDeferred);
 
   Node* receiver_map = LoadReceiverMap(p->receiver());
@@ -3026,16 +3049,24 @@ void AccessorAssembler::StoreIC(const StoreICParameters* p) {
     // Check megamorphic case.
     GotoIfNot(
         WordEqual(strong_feedback, LoadRoot(RootIndex::kmegamorphic_symbol)),
-        &miss);
+        &try_uninitialized);
 
     TryProbeStubCache(isolate()->store_stub_cache(), p->receiver(), p->name(),
                       &if_handler, &var_handler, &miss);
   }
+  BIND(&try_uninitialized);
+  {
+    // Check uninitialized case.
+    Branch(
+        WordEqual(strong_feedback, LoadRoot(RootIndex::kuninitialized_symbol)),
+        &no_feedback, &miss);
+  }
 
   BIND(&no_feedback);
   {
-    TailCallBuiltin(Builtins::kStoreIC_Nofeedback, p->context(), p->receiver(),
-                    p->name(), p->value(), p->slot());
+    TailCallBuiltin(Builtins::kStoreIC_Uninitialized, p->context(),
+                    p->receiver(), p->name(), p->value(), p->slot(),
+                    p->vector());
   }
 
   BIND(&miss);
@@ -3054,10 +3085,6 @@ void AccessorAssembler::StoreGlobalIC(const StoreICParameters* pp) {
   BIND(&if_heapobject);
   {
     Label try_handler(this), miss(this, Label::kDeferred);
-    // We use pre-monomorphic state for global stores that run into
-    // interceptors because the property doesn't exist yet. Using
-    // pre-monomorphic state gives it a chance to find more information the
-    // second time.
     GotoIf(
         WordEqual(maybe_weak_ref, LoadRoot(RootIndex::kpremonomorphic_symbol)),
         &miss);
@@ -3405,16 +3432,17 @@ void AccessorAssembler::GenerateLoadIC_Noninlined() {
                                 slot, vector);
 }
 
-void AccessorAssembler::GenerateLoadIC_Nofeedback() {
-  using Descriptor = LoadDescriptor;
+void AccessorAssembler::GenerateLoadIC_Uninitialized() {
+  using Descriptor = LoadWithVectorDescriptor;
 
   Node* receiver = Parameter(Descriptor::kReceiver);
   Node* name = Parameter(Descriptor::kName);
   Node* slot = Parameter(Descriptor::kSlot);
+  Node* vector = Parameter(Descriptor::kVector);
   TNode<Context> context = CAST(Parameter(Descriptor::kContext));
 
-  LoadICParameters p(context, receiver, name, slot, UndefinedConstant());
-  LoadIC_Nofeedback(&p);
+  LoadICParameters p(context, receiver, name, slot, vector);
+  LoadIC_Uninitialized(&p);
 }
 
 void AccessorAssembler::GenerateLoadICTrampoline() {

src/ic/accessor-assembler.h

@@ -30,7 +30,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
   void GenerateLoadIC();
   void GenerateLoadIC_Megamorphic();
   void GenerateLoadIC_Noninlined();
-  void GenerateLoadIC_Nofeedback();
+  void GenerateLoadIC_Uninitialized();
   void GenerateLoadICTrampoline();
   void GenerateLoadICTrampoline_Megamorphic();
   void GenerateKeyedLoadIC();
@@ -214,7 +214,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
   TNode<MaybeObject> LoadDescriptorValueOrFieldType(
       TNode<Map> map, TNode<IntPtrT> descriptor_entry);
 
-  void LoadIC_Nofeedback(const LoadICParameters* p);
+  void LoadIC_Uninitialized(const LoadICParameters* p);
 
   void KeyedLoadIC(const LoadICParameters* p, LoadAccessMode access_mode);
   void KeyedLoadICGeneric(const LoadICParameters* p);

src/ic/ic.cc

@@ -400,8 +400,10 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
 
   if (MigrateDeprecated(isolate(), object)) use_ic = false;
 
-  JSObject::MakePrototypesFast(object, kStartAtReceiver, isolate());
-  update_receiver_map(object);
+  if (state() != UNINITIALIZED) {
+    JSObject::MakePrototypesFast(object, kStartAtReceiver, isolate());
+    update_receiver_map(object);
+  }
 
   LookupIterator it(isolate(), object, name);
 
@@ -646,6 +648,15 @@ void IC::PatchCache(Handle<Name> name, const MaybeObjectHandle& handler) {
 }
 
 void LoadIC::UpdateCaches(LookupIterator* lookup) {
+  if (state() == UNINITIALIZED && !IsLoadGlobalIC()) {
+    // This is the first time we execute this inline cache. Set the target to
+    // the pre monomorphic stub to delay setting the monomorphic state.
+    TRACE_HANDLER_STATS(isolate(), LoadIC_Premonomorphic);
+    ConfigureVectorState(receiver_map());
+    TraceIC("LoadIC", lookup->name());
+    return;
+  }
+
   Handle<Object> code;
   if (lookup->state() == LookupIterator::ACCESS_CHECK) {
     code = slow_stub();
@@ -1404,7 +1415,9 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
     return TypeError(MessageTemplate::kNonObjectPropertyStore, object, name);
   }
 
-  JSObject::MakePrototypesFast(object, kStartAtPrototype, isolate());
+  if (state() != UNINITIALIZED) {
+    JSObject::MakePrototypesFast(object, kStartAtPrototype, isolate());
+  }
   LookupIterator it(isolate(), object, name);
 
   if (name->IsPrivate()) {
@@ -1429,6 +1442,15 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
 
 void StoreIC::UpdateCaches(LookupIterator* lookup, Handle<Object> value,
                            StoreOrigin store_origin) {
+  if (state() == UNINITIALIZED && !IsStoreGlobalIC()) {
+    // This is the first time we execute this inline cache. Transition
+    // to premonomorphic state to delay setting the monomorphic state.
+    TRACE_HANDLER_STATS(isolate(), StoreIC_Premonomorphic);
+    ConfigureVectorState(receiver_map());
+    TraceIC("StoreIC", lookup->name());
+    return;
+  }
+
   MaybeObjectHandle handler;
   if (LookupForWrite(lookup, value, store_origin)) {
     if (IsStoreGlobalIC()) {
@@ -1788,8 +1810,10 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
   handlers.reserve(target_receiver_maps.size());
   StoreElementPolymorphicHandlers(&target_receiver_maps, &handlers, store_mode);
   if (target_receiver_maps.size() == 0) {
-    Handle<Object> handler = StoreElementHandler(receiver_map, store_mode);
-    ConfigureVectorState(Handle<Name>(), receiver_map, handler);
+    // Transition to PREMONOMORPHIC state here and remember a weak-reference
+    // to the {receiver_map} in case TurboFan sees this function before the
+    // IC can transition further.
+    ConfigureVectorState(receiver_map);
   } else if (target_receiver_maps.size() == 1) {
     ConfigureVectorState(Handle<Name>(), target_receiver_maps[0], handlers[0]);
   } else {

src/ic/keyed-store-generic.cc

@@ -30,7 +30,7 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
 
   void KeyedStoreGeneric();
 
-  void StoreIC_Nofeedback();
+  void StoreIC_Uninitialized();
 
   // Generates code for [[Set]] operation, the |unique_name| is supposed to be
   // unique otherwise this code will always go to runtime.
@@ -138,9 +138,10 @@ void KeyedStoreGenericGenerator::Generate(compiler::CodeAssemblerState* state) {
   assembler.KeyedStoreGeneric();
 }
 
-void StoreICNofeedbackGenerator::Generate(compiler::CodeAssemblerState* state) {
+void StoreICUninitializedGenerator::Generate(
+    compiler::CodeAssemblerState* state) {
   KeyedStoreGenericAssembler assembler(state, StoreMode::kOrdinary);
-  assembler.StoreIC_Nofeedback();
+  assembler.StoreIC_Uninitialized();
 }
 
 void KeyedStoreGenericGenerator::SetProperty(
@@ -1042,13 +1043,14 @@ void KeyedStoreGenericAssembler::SetProperty(TNode<Context> context,
   KeyedStoreGeneric(context, receiver, key, value, Just(language_mode));
 }
 
-void KeyedStoreGenericAssembler::StoreIC_Nofeedback() {
-  using Descriptor = StoreDescriptor;
+void KeyedStoreGenericAssembler::StoreIC_Uninitialized() {
+  using Descriptor = StoreWithVectorDescriptor;
 
   Node* receiver = Parameter(Descriptor::kReceiver);
   Node* name = Parameter(Descriptor::kName);
   Node* value = Parameter(Descriptor::kValue);
   Node* slot = Parameter(Descriptor::kSlot);
+  Node* vector = Parameter(Descriptor::kVector);
   Node* context = Parameter(Descriptor::kContext);
 
   Label miss(this, Label::kDeferred), store_property(this);
@@ -1059,16 +1061,33 @@ void KeyedStoreGenericAssembler::StoreIC_Nofeedback() {
   // Receivers requiring non-standard element accesses (interceptors, access
   // checks, strings and string wrappers, proxies) are handled in the runtime.
   GotoIf(IsSpecialReceiverInstanceType(instance_type), &miss);
+
+  // Optimistically write the state transition to the vector.
+  GotoIf(IsUndefined(vector), &store_property);
+  StoreFeedbackVectorSlot(vector, slot,
+                          LoadRoot(RootIndex::kpremonomorphic_symbol),
+                          SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
+  Goto(&store_property);
+
+  BIND(&store_property);
   {
-    StoreICParameters p(CAST(context), receiver, name, value, slot,
-                        UndefinedConstant());
+    StoreICParameters p(CAST(context), receiver, name, value, slot, vector);
     EmitGenericPropertyStore(receiver, receiver_map, &p, &miss);
   }
 
   BIND(&miss);
   {
-    TailCallRuntime(Runtime::kStoreIC_Miss, context, value, slot,
-                    UndefinedConstant(), receiver, name);
+    Label call_runtime(this);
+    // Undo the optimistic state transition.
+    GotoIf(IsUndefined(vector), &call_runtime);
+    StoreFeedbackVectorSlot(vector, slot,
+                            LoadRoot(RootIndex::kuninitialized_symbol),
+                            SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
+    Goto(&call_runtime);
+
+    BIND(&call_runtime);
+    TailCallRuntime(Runtime::kStoreIC_Miss, context, value, slot, vector,
+                    receiver, name);
   }
 }
 

src/ic/keyed-store-generic.h

@@ -37,7 +37,7 @@ class KeyedStoreGenericGenerator {
                                    TNode<Object> value);
 };
 
-class StoreICNofeedbackGenerator {
+class StoreICUninitializedGenerator {
  public:
   static void Generate(compiler::CodeAssemblerState* state);
 };

test/cctest/test-feedback-vector.cc

@@ -438,7 +438,9 @@ TEST(VectorLoadICStates) {
       Handle<FeedbackVector>(f->feedback_vector(), isolate);
   FeedbackSlot slot(0);
   FeedbackNexus nexus(feedback_vector, slot);
+  CHECK_EQ(PREMONOMORPHIC, nexus.ic_state());
 
+  CompileRun("f(o)");
   CHECK_EQ(MONOMORPHIC, nexus.ic_state());
   // Verify that the monomorphic map is the one we expect.
   v8::MaybeLocal<v8::Value> v8_o =
@@ -524,13 +526,16 @@ TEST(VectorLoadICOnSmi) {
   CompileRun(
       "var o = { foo: 3 };"
       "%EnsureFeedbackVectorForFunction(f);"
-      "function f(a) { return a.foo; } f(34);");
+      "function f(a) { return a.foo; } f(o);");
   Handle<JSFunction> f = GetFunction("f");
   // There should be one IC.
   Handle<FeedbackVector> feedback_vector =
       Handle<FeedbackVector>(f->feedback_vector(), isolate);
   FeedbackSlot slot(0);
   FeedbackNexus nexus(feedback_vector, slot);
+  CHECK_EQ(PREMONOMORPHIC, nexus.ic_state());
+
+  CompileRun("f(34)");
   CHECK_EQ(MONOMORPHIC, nexus.ic_state());
   // Verify that the monomorphic map is the one we expect.
   Map number_map = ReadOnlyRoots(heap).heap_number_map();

test/mjsunit/es6/large-classes-properties.js

@@ -8,14 +8,14 @@
   // This is to test for dictionary mode when there more than
   // kMaxNumberOfDescriptors (1024) properties.
   const kLimit = 1030;
-  let evalString = "function f(i) { " +
+  let evalString = "(function(i) { " +
       "let clazz = class { " +
       "   constructor(i) { this.value = i;";
   for (let i = 0; i < kLimit ; i++) {
     evalString  += "this.property"+i +" = "+i+"; "
   }
   evalString += "}};" +
-      " return (new clazz(i)); }; f;";
+      " return (new clazz(i)); })";
 
   let fn = eval(evalString);
   %PrepareFunctionForOptimization(fn);