[stubs] KeyedStoreGeneric: overwrite existing fast properties directly

src/ic/keyed-store-generic.cc

Index: src/ic/keyed-store-generic.cc
diff --git a/src/ic/keyed-store-generic.cc b/src/ic/keyed-store-generic.cc
index 00d2d48a6fd550dd75788d37037207d98ef39b7d..f05f326550feadecfbfcffe8ed7dcf86325ec69b 100644
--- a/src/ic/keyed-store-generic.cc
+++ b/src/ic/keyed-store-generic.cc
@@ -72,6 +72,13 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
                                       Variable* var_accessor_pair,
                                       Variable* var_accessor_holder,
                                       Label* readonly, Label* bailout);
+
+  void CheckFieldType(Node* descriptors, Node* name_index, Node* representation,
+                      Node* value, Label* bailout);
+  void OverwriteExistingFastProperty(Node* object, Node* object_map,
+                                     Node* properties, Node* descriptors,
+                                     Node* descriptor_name_index, Node* details,
+                                     Node* value, Label* slow);
 };
 
 void KeyedStoreGenericGenerator::Generate(compiler::CodeAssemblerState* state,
@@ -544,6 +551,7 @@ void KeyedStoreGenericAssembler::LookupPropertyOnPrototypeChain(
         JumpIfDataProperty(details, &ok_to_write, readonly);
 
         // Accessor case.
+        // TODO(jkummerow): Implement a trimmed-down LoadAccessorFromFastObject.
         Variable var_details(this, MachineRepresentation::kWord32);
         LoadPropertyFromFastObject(holder, holder_map, descriptors, name_index,
                                    &var_details, var_accessor_pair);
@@ -599,6 +607,149 @@ void KeyedStoreGenericAssembler::LookupPropertyOnPrototypeChain(
   Bind(&ok_to_write);
 }
 
+void KeyedStoreGenericAssembler::CheckFieldType(Node* descriptors,
+                                                Node* name_index,
+                                                Node* representation,
+                                                Node* value, Label* bailout) {
+  Label r_smi(this), r_double(this), r_heapobject(this), all_fine(this);
+  if (FLAG_track_fields) {
+    GotoIf(Word32Equal(representation, Int32Constant(Representation::kSmi)),
+           &r_smi);
+  }
+  if (FLAG_track_double_fields) {
+    GotoIf(Word32Equal(representation, Int32Constant(Representation::kDouble)),
+           &r_double);
+  }
+  if (FLAG_track_heap_object_fields) {
+    GotoIf(
+        Word32Equal(representation, Int32Constant(Representation::kHeapObject)),
+        &r_heapobject);
+  }
+  if (FLAG_track_fields) {
+    GotoIf(Word32Equal(representation, Int32Constant(Representation::kNone)),
+           bailout);
+  }

[Igor Sheludko, 2017/02/16 16:04:37]

CSA_ASSERT(representation == kTagged)?

[Jakob Kummerow, 2017/02/16 18:24:27]

Done.

+  Goto(&all_fine);
+
+  Bind(&r_smi);

[Igor Sheludko, 2017/02/16 16:04:37]

1) "if (FLAG_track_fields) {" or we will have an unreachable block.
2) I think FLAG_track_fields must be made readonly or we could end up using stub
which is not in sync with the runtime code.

[Jakob Kummerow, 2017/02/16 18:24:27]

I agree, but as discussed, that unfortunately doesn't work, because the snapshot
intentionally turns off FLAG_track_double_fields. To solve that issue, let's
just always emit all code here, regardless of flags.

+  { Branch(TaggedIsSmi(value), &all_fine, bailout); }
+
+  Bind(&r_double);

[Igor Sheludko, 2017/02/16 16:04:37]

Same here.

+  {
+    GotoIf(TaggedIsSmi(value), &all_fine);
+    Node* value_map = LoadMap(value);
+    // While supporting mutable HeapNumbers would be straightforward, such
+    // objects should not end up here anyway.
+    CSA_ASSERT(this,
+               WordNotEqual(value_map,
+                            LoadRoot(Heap::kMutableHeapNumberMapRootIndex)));
+    Branch(IsHeapNumberMap(value_map), &all_fine, bailout);
+  }
+
+  Bind(&r_heapobject);

[Igor Sheludko, 2017/02/16 16:04:37]

Same here.

+  {
+    GotoIf(TaggedIsSmi(value), bailout);
+    Node* field_type =
+        LoadValueByKeyIndex<DescriptorArray>(descriptors, name_index);
+    intptr_t kNoneType = reinterpret_cast<intptr_t>(FieldType::None());
+    intptr_t kAnyType = reinterpret_cast<intptr_t>(FieldType::Any());
+    // FieldType::None can't hold any value.
+    GotoIf(WordEqual(field_type, IntPtrConstant(kNoneType)), bailout);
+    // FieldType::Any can hold any value.
+    GotoIf(WordEqual(field_type, IntPtrConstant(kAnyType)), &all_fine);
+    CSA_ASSERT(this, IsWeakCell(field_type));
+    // Cleared WeakCells count as FieldType::None, which can't hold any value.
+    field_type = LoadWeakCellValue(field_type, bailout);
+    // FieldType::Class(...) performs a map check.
+    CSA_ASSERT(this, IsMap(field_type));
+    Branch(WordEqual(LoadMap(value), field_type), &all_fine, bailout);
+  }
+
+  Bind(&all_fine);
+}
+
+void KeyedStoreGenericAssembler::OverwriteExistingFastProperty(
+    Node* object, Node* object_map, Node* properties, Node* descriptors,
+    Node* descriptor_name_index, Node* details, Node* value, Label* slow) {
+  // Properties in descriptors can't be overwritten without map transition.
+  GotoIf(Word32NotEqual(DecodeWord32<PropertyDetails::LocationField>(details),
+                        Int32Constant(kField)),
+         slow);
+
+  if (FLAG_track_constant_fields) {
+    // TODO(ishell): Taking the slow path is not necessary if new and old
+    // values are identical.
+    GotoIf(Word32Equal(DecodeWord32<PropertyDetails::ConstnessField>(details),
+                       Int32Constant(kConst)),
+           slow);
+  }
+
+  Label done(this);
+  Node* representation =
+      DecodeWord32<PropertyDetails::RepresentationField>(details);
+
+  CheckFieldType(descriptors, descriptor_name_index, representation, value,
+                 slow);
+  Node* field_index =
+      DecodeWordFromWord32<PropertyDetails::FieldIndexField>(details);
+  Node* inobject_properties = LoadMapInobjectProperties(object_map);
+
+  Label inobject(this), backing_store(this);
+  Branch(UintPtrLessThan(field_index, inobject_properties), &inobject,
+         &backing_store);
+
+  Bind(&inobject);
+  {
+    Node* field_offset =
+        IntPtrMul(IntPtrSub(LoadMapInstanceSize(object_map),
+                            IntPtrSub(inobject_properties, field_index)),
+                  IntPtrConstant(kPointerSize));
+    Label tagged_rep(this), double_rep(this);
+    Branch(Word32Equal(representation, Int32Constant(Representation::kDouble)),
+           &double_rep, &tagged_rep);
+    Bind(&double_rep);
+    {
+      Node* double_value = ChangeNumberToFloat64(value);
+      if (FLAG_unbox_double_fields) {
+        StoreObjectFieldNoWriteBarrier(object, field_offset, double_value,
+                                       MachineRepresentation::kFloat64);
+      } else {
+        Node* mutable_heap_number = LoadObjectField(object, field_offset);
+        StoreHeapNumberValue(mutable_heap_number, double_value);
+      }
+      Goto(&done);
+    }
+
+    Bind(&tagged_rep);
+    {
+      StoreObjectField(object, field_offset, value);
+      Goto(&done);
+    }
+  }
+
+  Bind(&backing_store);
+  {
+    Node* backing_store_index = IntPtrSub(field_index, inobject_properties);
+    Label tagged_rep(this), double_rep(this);
+    Branch(Word32Equal(representation, Int32Constant(Representation::kDouble)),
+           &double_rep, &tagged_rep);
+    Bind(&double_rep);
+    {
+      Node* double_value = ChangeNumberToFloat64(value);
+      Node* mutable_heap_number =
+          LoadFixedArrayElement(properties, backing_store_index);
+      StoreHeapNumberValue(mutable_heap_number, double_value);
+      Goto(&done);
+    }
+    Bind(&tagged_rep);
+    {
+      StoreFixedArrayElement(properties, backing_store_index, value);
+      Goto(&done);
+    }
+  }
+  Bind(&done);
+}
+
 void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
     Node* receiver, Node* receiver_map, const StoreICParameters* p, Label* slow,
     LanguageMode language_mode) {
@@ -613,10 +764,40 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
 
   Bind(&fast_properties);
   {
-    // TODO(jkummerow): Does it make sense to support some cases here inline?
-    // Maybe overwrite existing writable properties?
-    // Maybe support map transitions?
-    Goto(&stub_cache);
+    Comment("fast property store");
+    Node* bitfield3 = LoadMapBitField3(receiver_map);
+    Node* descriptors = LoadMapDescriptors(receiver_map);
+    Label descriptor_found(this);
+    Variable var_name_index(this, MachineType::PointerRepresentation());
+    // TODO(jkummerow): Maybe look for existing map transitions?
+    Label* notfound = &stub_cache;
+    DescriptorLookup(p->name, descriptors, bitfield3, &descriptor_found,
+                     &var_name_index, notfound);
+
+    Bind(&descriptor_found);
+    {
+      Node* name_index = var_name_index.value();
+      Node* details =
+          LoadDetailsByKeyIndex<DescriptorArray>(descriptors, name_index);
+      Label data_property(this);
+      JumpIfDataProperty(details, &data_property, &readonly);
+
+      // Accessor case.
+      // TODO(jkummerow): Implement a trimmed-down LoadAccessorFromFastObject.
+      Variable var_details(this, MachineRepresentation::kWord32);
+      LoadPropertyFromFastObject(receiver, receiver_map, descriptors,
+                                 name_index, &var_details, &var_accessor_pair);
+      var_accessor_holder.Bind(receiver);
+      Goto(&accessor);
+
+      Bind(&data_property);
+      {
+        OverwriteExistingFastProperty(receiver, receiver_map, properties,
+                                      descriptors, name_index, details,
+                                      p->value, slow);
+        Return(p->value);
+      }
+    }
   }
 
   Bind(&dictionary_properties);