Revert "[ic] Migrate StoreGlobal to data handler"

This reverts commit ea4346be.

Reason for revert: This causes a crash in a Node.js test (parallel/test-repl-options). I can reproduce the crash locally. At a first glance, it doesn't look like we're doing anything terribly wrong in Node that explains the crash. I'll have a closer lock tomorrow. Feel free to reland if needed, we can always deactivate the test.  

Original change's description:
> [ic] Migrate StoreGlobal to data handler
> 
> BUG=v8:5561
> 
> Change-Id: If4c679c97af199ce1c90d055627186123bc88574
> Reviewed-on: https://chromium-review.googlesource.com/456698
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Commit-Queue: Toon Verwaest <verwaest@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#43944}

TBR=ishell@chromium.org,verwaest@chromium.org,v8-reviews@googlegroups.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5561

Change-Id: I790ff9ab45016749fe2f3982045f497a995e282e
Reviewed-on: https://chromium-review.googlesource.com/456505Reviewed-by: Franziska Hinkelmann <franzih@chromium.org>
Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43951}

src/code-stub-assembler.cc

@@ -2960,7 +2960,7 @@ Node* CodeStubAssembler::IsJSArray(Node* object) {
 }
 
 Node* CodeStubAssembler::IsWeakCell(Node* object) {
-  return IsWeakCellMap(LoadMap(object));
+  return HasInstanceType(object, WEAK_CELL_TYPE);
 }
 
 Node* CodeStubAssembler::IsBoolean(Node* object) {

src/code-stub-assembler.h

@@ -48,8 +48,7 @@ enum class PrimitiveType { kBoolean, kNumber, kString, kSymbol };
   V(TrueValue, True)                                  \
   V(Tuple2Map, Tuple2Map)                             \
   V(Tuple3Map, Tuple3Map)                             \
-  V(UndefinedValue, Undefined)                        \
-  V(WeakCellMap, WeakCellMap)
+  V(UndefinedValue, Undefined)
 
 // Provides JavaScript-specific "macro-assembler" functionality on top of the
 // CodeAssembler. By factoring the JavaScript-isms out of the CodeAssembler,

src/code-stubs.cc

@@ -1490,6 +1490,88 @@ 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<int>(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;

src/code-stubs.h

@@ -90,6 +90,7 @@ class Node;
   V(StringAdd)                                \
   V(GetProperty)                              \
   V(StoreFastElement)                         \
+  V(StoreGlobal)                              \
   V(StoreInterceptor)                         \
   V(LoadIndexedInterceptor)                   \
   V(GrowArrayElements)
@@ -894,6 +895,49 @@ class KeyedStoreSloppyArgumentsStub : public TurboFanCodeStub {
   DEFINE_TURBOFAN_CODE_STUB(KeyedStoreSloppyArguments, TurboFanCodeStub);
 };
 
+class StoreGlobalStub : public TurboFanCodeStub {
+ public:
+  StoreGlobalStub(Isolate* isolate, PropertyCellType type,
+                  Maybe<PropertyCellConstantType> 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<HeapObject> property_cell_placeholder(Isolate* isolate) {
+    return isolate->factory()->uninitialized_value();
+  }
+
+  Handle<Code> GetCodeCopyFromTemplate(Handle<PropertyCell> 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<PropertyCellType, 0, 2> {};
+  class ConstantTypeBits
+      : public BitField<PropertyCellConstantType, CellTypeBits::kNext, 2> {};
+
+  DEFINE_CALL_INTERFACE_DESCRIPTOR(StoreWithVector);
+  DEFINE_TURBOFAN_CODE_STUB(StoreGlobal, TurboFanCodeStub);
+};
+
 class CallApiCallbackStub : public PlatformCodeStub {
  public:
   static const int kArgBits = 3;

src/counters.h

@@ -814,8 +814,8 @@ class RuntimeCallTimer final {
   V(StoreIC_StoreField)                          \
   V(StoreIC_StoreFieldDH)                        \
   V(StoreIC_StoreFieldStub)                      \
-  V(StoreIC_StoreGlobalDH)                       \
-  V(StoreIC_StoreGlobalTransitionDH)             \
+  V(StoreIC_StoreGlobal)                         \
+  V(StoreIC_StoreGlobalTransition)               \
   V(StoreIC_StoreInterceptorStub)                \
   V(StoreIC_StoreNormalDH)                       \
   V(StoreIC_StoreScriptContextFieldStub)         \

src/ic/accessor-assembler.cc

@@ -617,8 +617,7 @@ 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),
-      store_global(this);
+  Label if_proto_handler(this), if_element_handler(this), call_handler(this);
 
   Branch(TaggedIsSmi(handler), &if_smi_handler, &if_nonsmi_handler);
 
@@ -666,7 +665,6 @@ 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);
   }
 
@@ -687,66 +685,6 @@ 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<PropertyDetails::PropertyCellTypeField>(details);
-
-    Label constant(this), store(this), not_smi(this);
-
-    GotoIf(
-        Word32Equal(
-            type, Int32Constant(static_cast<int>(PropertyCellType::kConstant))),
-        &constant);
-
-    GotoIf(IsTheHole(cell_contents), miss);
-
-    GotoIf(
-        Word32Equal(
-            type, Int32Constant(static_cast<int>(PropertyCellType::kMutable))),
-        &store);
-    CSA_ASSERT(this,
-               Word32Or(Word32Equal(type,
-                                    Int32Constant(static_cast<int>(
-                                        PropertyCellType::kConstantType))),
-                        Word32Equal(type,
-                                    Int32Constant(static_cast<int>(
-                                        PropertyCellType::kUndefined)))));
-
-    GotoIfNot(TaggedIsSmi(cell_contents), &not_smi);
-    GotoIfNot(TaggedIsSmi(p->value), miss);
-    Goto(&store);
-
-    Bind(&not_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(

src/ic/handler-configuration-inl.h

@@ -92,15 +92,15 @@ Handle<Smi> LoadHandler::LoadElement(Isolate* isolate,
   return handle(Smi::FromInt(config), isolate);
 }
 
-Handle<Smi> StoreHandler::StoreNormal(Isolate* isolate) {
+Handle<Object> StoreHandler::StoreNormal(Isolate* isolate) {
   int config = KindBits::encode(kStoreNormal);
   return handle(Smi::FromInt(config), isolate);
 }
 
-Handle<Smi> StoreHandler::StoreField(Isolate* isolate, Kind kind,
-                                     int descriptor, FieldIndex field_index,
-                                     Representation representation,
-                                     bool extend_storage) {
+Handle<Object> 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<Smi> StoreHandler::StoreField(Isolate* isolate, Kind kind,
       break;
     default:
       UNREACHABLE();
-      return Handle<Smi>::null();
+      return Handle<Object>::null();
   }
 
   DCHECK(kind == kStoreField || kind == kTransitionToField ||
@@ -134,26 +134,26 @@ Handle<Smi> StoreHandler::StoreField(Isolate* isolate, Kind kind,
   return handle(Smi::FromInt(config), isolate);
 }
 
-Handle<Smi> StoreHandler::StoreField(Isolate* isolate, int descriptor,
-                                     FieldIndex field_index,
-                                     PropertyConstness constness,
-                                     Representation representation) {
+Handle<Object> 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<Smi> StoreHandler::TransitionToField(Isolate* isolate, int descriptor,
-                                            FieldIndex field_index,
-                                            Representation representation,
-                                            bool extend_storage) {
+Handle<Object> 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<Smi> StoreHandler::TransitionToConstant(Isolate* isolate,
-                                               int descriptor) {
+Handle<Object> StoreHandler::TransitionToConstant(Isolate* isolate,
+                                                  int descriptor) {
   DCHECK(!FLAG_track_constant_fields);
   int config =
       StoreHandler::KindBits::encode(StoreHandler::kTransitionToConstant) |

src/ic/handler-configuration.h

@@ -191,30 +191,32 @@ class StoreHandler {
   static const int kFirstPrototypeIndex = 3;
 
   // Creates a Smi-handler for storing a field to fast object.
-  static inline Handle<Smi> StoreField(Isolate* isolate, int descriptor,
-                                       FieldIndex field_index,
-                                       PropertyConstness constness,
-                                       Representation representation);
+  static inline Handle<Object> 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<Smi> StoreNormal(Isolate* isolate);
+  static inline Handle<Object> StoreNormal(Isolate* isolate);
 
   // Creates a Smi-handler for transitioning store to a field.
-  static inline Handle<Smi> TransitionToField(Isolate* isolate, int descriptor,
-                                              FieldIndex field_index,
-                                              Representation representation,
-                                              bool extend_storage);
+  static inline Handle<Object> 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<Smi> TransitionToConstant(Isolate* isolate,
-                                                 int descriptor);
+  static inline Handle<Object> TransitionToConstant(Isolate* isolate,
+                                                    int descriptor);
 
  private:
-  static inline Handle<Smi> StoreField(Isolate* isolate, Kind kind,
-                                       int descriptor, FieldIndex field_index,
-                                       Representation representation,
-                                       bool extend_storage);
+  static inline Handle<Object> StoreField(Isolate* isolate, Kind kind,
+                                          int descriptor,
+                                          FieldIndex field_index,
+                                          Representation representation,
+                                          bool extend_storage);
 };
 
 }  // namespace internal

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->IsWeakCell() ||
+         object->IsTuple3() || object->IsFixedArray() ||
          (object->IsCode() && Code::cast(object)->is_handler());
 }
 

src/ic/ic.cc

@@ -1797,7 +1797,7 @@ Handle<Object> StoreIC::StoreTransition(Handle<Map> receiver_map,
       Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate());
   if (validity_cell.is_null()) {
     DCHECK_EQ(0, checks_count);
-    validity_cell = handle(Smi::kZero, isolate());
+    validity_cell = handle(Smi::FromInt(0), isolate());
   }
 
   Handle<WeakCell> transition_cell = Map::WeakCellForMap(transition);
@@ -1816,14 +1816,22 @@ Handle<Object> StoreIC::StoreTransition(Handle<Map> receiver_map,
   return handler_array;
 }
 
-namespace {
-
-Handle<Object> StoreGlobal(Isolate* isolate, Handle<PropertyCell> cell) {
-  return isolate->factory()->NewWeakCell(cell);
+static Handle<Code> PropertyCellStoreHandler(Isolate* isolate,
+                                             Handle<JSObject> store_target,
+                                             Handle<Name> name,
+                                             Handle<PropertyCell> cell,
+                                             PropertyCellType type) {
+  auto constant_type = Nothing<PropertyCellConstantType>();
+  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<Object> StoreIC::GetMapIndependentHandler(LookupIterator* lookup) {
   DCHECK_NE(LookupIterator::JSPROXY, lookup->state());
 
@@ -1836,8 +1844,7 @@ Handle<Object> StoreIC::GetMapIndependentHandler(LookupIterator* lookup) {
     case LookupIterator::TRANSITION: {
       auto store_target = lookup->GetStoreTarget();
       if (store_target->IsJSGlobalObject()) {
-        TRACE_HANDLER_STATS(isolate(), StoreIC_StoreGlobalTransitionDH);
-        return StoreGlobal(isolate(), lookup->transition_cell());
+        break;  // Custom-compiled handler.
       }
       // Currently not handled by CompileStoreTransition.
       if (!holder->HasFastProperties()) {
@@ -1914,8 +1921,7 @@ Handle<Object> StoreIC::GetMapIndependentHandler(LookupIterator* lookup) {
       DCHECK_EQ(kData, lookup->property_details().kind());
       if (lookup->is_dictionary_holder()) {
         if (holder->IsJSGlobalObject()) {
-          TRACE_HANDLER_STATS(isolate(), StoreIC_StoreGlobalDH);
-          return StoreGlobal(isolate(), lookup->GetPropertyCell());
+          break;  // Custom-compiled handler.
         }
         TRACE_HANDLER_STATS(isolate(), StoreIC_StoreNormalDH);
         DCHECK(holder.is_identical_to(receiver));
@@ -1963,6 +1969,24 @@ Handle<Object> 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<PropertyCell> cell = lookup->transition_cell();
+        cell->set_value(*value);
+        Handle<Code> 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<Object> accessors = lookup->GetAccessors();
@@ -2008,9 +2032,20 @@ Handle<Object> StoreIC::CompileHandler(LookupIterator* lookup,
       }
     }
 
-    case LookupIterator::DATA:
-    case LookupIterator::TRANSITION:
-    case LookupIterator::INTERCEPTOR:
+    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::INTEGER_INDEXED_EXOTIC:
     case LookupIterator::ACCESS_CHECK:
     case LookupIterator::JSPROXY: