Reland "[ic] In-place Double -> Tagged transitions"

This is a reland of 0736599a69.
This is a reland of 7e1fbe8f34.

Original change description:
> [ic] In-place Double -> Tagged transitions
>
> With no more MutableHeapNumber, we can make Double -> Tagged transitions
> in-place, at the cost of an extra map check when accessing double fields
> to make sure they are still doubles.
>
> Bug: v8:9606
> Change-Id: I74ff39ed6fba62ee223cd37dfe761f7d73020e1c
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1743973
> Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#63374}

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

Bug: v8:9606
Change-Id: I2d1b7416064d743582f4983fb868316b7e8a4cf2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1777661
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63499}

src/codegen/code-stub-assembler.h

@@ -2730,6 +2730,16 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
     return Word32Equal(Word32And(word32, const_mask), const_mask);
   }
 
+  // Returns true if the bit field |BitField| in |word32| is equal to a given.
+  // constant |value|. Avoids a shift compared to using DecodeWord32.
+  template <typename BitField>
+  TNode<BoolT> IsEqualInWord32(TNode<Word32T> word32,
+                               typename BitField::FieldType value) {
+    TNode<Word32T> masked_word32 =
+        Word32And(word32, Int32Constant(BitField::kMask));
+    return Word32Equal(masked_word32, Int32Constant(BitField::encode(value)));
+  }
+
   // Returns true if any of the |T|'s bits in given |word| are set.
   template <typename T>
   TNode<BoolT> IsSetWord(SloppyTNode<WordT> word) {

src/compiler/access-info.cc

@@ -351,6 +351,11 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
                                                                   descriptor));
   } else if (details_representation.IsDouble()) {
     field_type = type_cache_->kFloat64;
+    if (!FLAG_unbox_double_fields) {
+      unrecorded_dependencies.push_back(
+          dependencies()->FieldRepresentationDependencyOffTheRecord(
+              map_ref, descriptor));
+    }
   } else if (details_representation.IsHeapObject()) {
     // Extract the field type from the property details (make sure its
     // representation is TaggedPointer to reflect the heap object case).
@@ -789,6 +794,12 @@ PropertyAccessInfo AccessInfoFactory::LookupTransition(
             transition_map_ref, number));
   } else if (details_representation.IsDouble()) {
     field_type = type_cache_->kFloat64;
+    if (!FLAG_unbox_double_fields) {
+      transition_map_ref.SerializeOwnDescriptor(number);
+      unrecorded_dependencies.push_back(
+          dependencies()->FieldRepresentationDependencyOffTheRecord(
+              transition_map_ref, number));
+    }
   } else if (details_representation.IsHeapObject()) {
     // Extract the field type from the property details (make sure its
     // representation is TaggedPointer to reflect the heap object case).

src/ic/accessor-assembler.cc

@@ -16,6 +16,7 @@
 #include "src/objects/heap-number.h"
 #include "src/objects/module.h"
 #include "src/objects/objects-inl.h"
+#include "src/objects/property-details.h"
 #include "src/objects/smi.h"
 
 namespace v8 {
@@ -239,7 +240,7 @@ void AccessorAssembler::HandleLoadAccessor(
 
 void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,
                                         Variable* var_double_value,
-                                        Label* rebox_double,
+                                        Label* rebox_double, Label* miss,
                                         ExitPoint* exit_point) {
   Comment("field_load");
   TNode<IntPtrT> index =
@@ -261,8 +262,13 @@ void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,
       var_double_value->Bind(
           LoadObjectField(holder, offset, MachineType::Float64()));
     } else {
-      TNode<HeapNumber> heap_number = CAST(LoadObjectField(holder, offset));
-      var_double_value->Bind(LoadHeapNumberValue(heap_number));
+      TNode<Object> heap_number = LoadObjectField(holder, offset);
+      // This is not an "old" Smi value from before a Smi->Double transition.
+      // Rather, it's possible that since the last update of this IC, the Double
+      // field transitioned to a Tagged field, and was then assigned a Smi.
+      GotoIf(TaggedIsSmi(heap_number), miss);
+      GotoIfNot(IsHeapNumber(CAST(heap_number)), miss);
+      var_double_value->Bind(LoadHeapNumberValue(CAST(heap_number)));
     }
     Goto(rebox_double);
   }
@@ -276,6 +282,13 @@ void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,
     exit_point->Return(value);
 
     BIND(&is_double);
+    if (!FLAG_unbox_double_fields) {
+      // This is not an "old" Smi value from before a Smi->Double transition.
+      // Rather, it's possible that since the last update of this IC, the Double
+      // field transitioned to a Tagged field, and was then assigned a Smi.
+      GotoIf(TaggedIsSmi(value), miss);
+      GotoIfNot(IsHeapNumber(CAST(value)), miss);
+    }
     var_double_value->Bind(LoadHeapNumberValue(CAST(value)));
     Goto(rebox_double);
   }
@@ -465,7 +478,7 @@ void AccessorAssembler::HandleLoadICSmiHandlerLoadNamedCase(
          &module_export, &interceptor);
 
   BIND(&field);
-  HandleLoadField(holder, handler_word, var_double_value, rebox_double,
+  HandleLoadField(holder, handler_word, var_double_value, rebox_double, miss,
                   exit_point);
 
   BIND(&nonexistent);
@@ -1683,6 +1696,24 @@ Node* AccessorAssembler::PrepareValueForStore(Node* handler_word, Node* holder,
   if (representation.IsDouble()) {
     value = TryTaggedToFloat64(value, bailout);
 
+    // We have to check that the representation is still Double. Checking the
+    // value is nor enough, as we could have transitioned to Tagged but still
+    // be holding a HeapNumber, which would no longer be allowed to be mutable.
+
+    // TODO(leszeks): We could skip the representation check in favor of a
+    // constant value check in StoreNamedField here, but then StoreNamedField
+    // would need an IsHeapNumber check in case both the representation changed
+    // and the value is no longer a HeapNumber.
+    TNode<IntPtrT> descriptor_entry =
+        Signed(DecodeWord<StoreHandler::DescriptorBits>(handler_word));
+    TNode<DescriptorArray> descriptors = LoadMapDescriptors(LoadMap(holder));
+    TNode<Uint32T> details =
+        LoadDetailsByDescriptorEntry(descriptors, descriptor_entry);
+
+    GotoIfNot(IsEqualInWord32<PropertyDetails::RepresentationField>(
+                  details, Representation::kDouble),
+              bailout);
+
   } else if (representation.IsHeapObject()) {
     GotoIf(TaggedIsSmi(value), bailout);
 

src/ic/accessor-assembler.h

@@ -275,7 +275,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
 
   void HandleLoadField(Node* holder, Node* handler_word,
                        Variable* var_double_value, Label* rebox_double,
-                       ExitPoint* exit_point);
+                       Label* miss, ExitPoint* exit_point);
 
   void EmitAccessCheck(TNode<Context> expected_native_context,
                        TNode<Context> context, Node* receiver,

src/objects/map.cc

@@ -791,7 +791,8 @@ void Map::GeneralizeField(Isolate* isolate, Handle<Map> map, int modify_index,
     map->PrintGeneralization(
         isolate, stdout, "field type generalization", modify_index,
         map->NumberOfOwnDescriptors(), map->NumberOfOwnDescriptors(), false,
-        details.representation(), details.representation(), old_constness,
+        details.representation(),
+        descriptors->GetDetails(modify_index).representation(), old_constness,
         new_constness, old_field_type, MaybeHandle<Object>(), new_field_type,
         MaybeHandle<Object>());
   }

src/objects/property-details.h

@@ -112,7 +112,9 @@ class Representation {
     // smi and tagged values. Doubles, however, would require a box allocation.
     if (IsNone()) return !other.IsDouble();
     if (!FLAG_modify_field_representation_inplace) return false;
-    return (IsSmi() || IsHeapObject()) && other.IsTagged();
+    return (IsSmi() || (!FLAG_unbox_double_fields && IsDouble()) ||
+            IsHeapObject()) &&
+           other.IsTagged();
   }
 
   bool is_more_general_than(const Representation& other) const {

test/cctest/test-field-type-tracking.cc

@@ -641,7 +641,7 @@ void TestGeneralizeField(int detach_property_at_index, int property_index,
                                       from.representation, from.type);
     } else {
       map = expectations.AddDataField(map, NONE, PropertyConstness::kConst,
-                                      Representation::Double(), any_type);
+                                      Representation::Smi(), any_type);
       if (i == detach_property_at_index) {
         detach_point_map = map;
       }
@@ -654,10 +654,10 @@ void TestGeneralizeField(int detach_property_at_index, int property_index,
   if (is_detached_map) {
     detach_point_map = Map::ReconfigureProperty(
         isolate, detach_point_map, detach_property_at_index, kData, NONE,
-        Representation::Tagged(), any_type);
+        Representation::Double(), any_type);
     expectations.SetDataField(detach_property_at_index,
                               PropertyConstness::kConst,
-                              Representation::Tagged(), any_type);
+                              Representation::Double(), any_type);
     CHECK(map->is_deprecated());
     CHECK(expectations.Check(*detach_point_map,
                              detach_point_map->NumberOfOwnDescriptors()));
@@ -814,7 +814,9 @@ TEST(GeneralizeDoubleFieldToTagged) {
   TestGeneralizeField(
       {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type});
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
+      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace,
+      !FLAG_unbox_double_fields && FLAG_modify_field_representation_inplace);
 }
 
 TEST(GeneralizeHeapObjectFieldToTagged) {
@@ -2297,7 +2299,7 @@ TEST(ElementsKindTransitionFromMapOwningDescriptor) {
         {PropertyConstness::kMutable, Representation::Double(), any_type},
         {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
         {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-        true);
+        FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
   }
 }
 
@@ -2365,7 +2367,7 @@ TEST(ElementsKindTransitionFromMapNotOwningDescriptor) {
         {PropertyConstness::kMutable, Representation::Double(), any_type},
         {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
         {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-        true);
+        FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
   }
 }
 
@@ -2408,7 +2410,8 @@ TEST(PrototypeTransitionFromMapOwningDescriptor) {
   TestGeneralizeFieldWithSpecialTransition(
       config, {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, true);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
+      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
 }
 
 TEST(PrototypeTransitionFromMapNotOwningDescriptor) {
@@ -2461,7 +2464,8 @@ TEST(PrototypeTransitionFromMapNotOwningDescriptor) {
   TestGeneralizeFieldWithSpecialTransition(
       config, {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, true);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
+      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

test/mjsunit/regress/regress-997485.js

@@ -0,0 +1,127 @@
+// Copyright 2019 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
+
+(function doubleToTaggedWithTaggedValueStoresCorrectly() {
+
+  function setX_Double(o) { o.x = 4.2; }
+
+  function foo() {
+    // o.x starts off as Double
+    const o = { x: 0.1 };
+
+    // Write to it a few times with setX_Double, to make sure setX_Double has
+    // Double feedback.
+    setX_Double(o);
+    setX_Double(o);
+
+    // Transition o.x to Tagged.
+    o.x = {};
+
+    // setX_Double will still have Double feedback, so make sure it works with
+    // the new Tagged representation o.x.
+    setX_Double(o);
+
+    assertEquals(o.x, 4.2);
+  }
+
+  %EnsureFeedbackVectorForFunction(setX_Double);
+  foo();
+
+})();
+
+(function doubleToTaggedWithDoubleValueDoesNotMutate() {
+
+  function setX_Double(o) { o.x = 4.2; }
+
+  function foo() {
+    // o.x starts off as Double
+    const o = { x: 0.1 };
+
+    // Write to it a few times with setX_Double, to make sure setX_Double has
+    // Double feedback.
+    setX_Double(o);
+    setX_Double(o);
+
+    // Transition o.x to Tagged.
+    o.x = {};
+
+    // Write the HeapNumber val to o.x.
+    const val = 1.25;
+    o.x = val;
+
+    // setX_Double will still have Double feedback, which expects to be able to
+    // mutate o.x's HeapNumber, so make sure it does not mutate val.
+    setX_Double(o);
+
+    assertEquals(o.x, 4.2);
+    assertNotEquals(val, 4.2);
+  }
+
+  %EnsureFeedbackVectorForFunction(setX_Double);
+  foo();
+
+})();
+
+(function doubleToTaggedWithTaggedValueStoresSmiCorrectly() {
+
+  function setX_Smi(o) { o.x = 42; }
+
+  function foo() {
+    // o.x starts off as Double
+    const o = { x: 0.1 };
+
+    // Write to it a few times with setX_Smi, to make sure setX_Smi has
+    // Double feedback.
+    setX_Smi(o);
+    setX_Smi(o);
+
+    // Transition o.x to Tagged.
+    o.x = {};
+
+    // setX_Smi will still have Double feedback, so make sure it works with
+    // the new Tagged representation o.x.
+    setX_Smi(o);
+
+    assertEquals(o.x, 42);
+  }
+
+  %EnsureFeedbackVectorForFunction(setX_Smi);
+  foo();
+
+})();
+
+(function doubleToTaggedWithSmiValueDoesNotMutate() {
+
+  function setX_Smi(o) { o.x = 42; }
+
+  function foo() {
+    // o.x starts off as Double
+    const o = { x: 0.1 };
+
+    // Write to it a few times with setX_Smi, to make sure setX_Smi has
+    // Double feedback.
+    setX_Smi(o);
+    setX_Smi(o);
+
+    // Transition o.x to Tagged.
+    o.x = {};
+
+    // Write the HeapNumber val to o.x.
+    const val = 1.25;
+    o.x = val;
+
+    // setX_Smi will still have Double feedback, which expects to be able to
+    // mutate o.x's HeapNumber, so make sure it does not mutate val.
+    setX_Smi(o);
+
+    assertEquals(o.x, 42);
+    assertNotEquals(val, 42);
+  }
+
+  %EnsureFeedbackVectorForFunction(setX_Smi);
+  foo();
+
+})();