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

This reverts commit 0736599a.
This reverts commit 7e1fbe8f.

Reason for revert: Still some crashes, reverting to unblock dev.

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

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

src/codegen/code-stub-assembler.h

@@ -2730,16 +2730,6 @@ 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,11 +351,6 @@ 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).
@@ -793,12 +788,6 @@ 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,7 +16,6 @@
 #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 {
@@ -240,7 +239,7 @@ void AccessorAssembler::HandleLoadAccessor(
 
 void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,
                                         Variable* var_double_value,
-                                        Label* rebox_double, Label* miss,
+                                        Label* rebox_double,
                                         ExitPoint* exit_point) {
   Comment("field_load");
   TNode<IntPtrT> index =
@@ -262,13 +261,8 @@ void AccessorAssembler::HandleLoadField(Node* holder, Node* handler_word,
       var_double_value->Bind(
           LoadObjectField(holder, offset, MachineType::Float64()));
     } else {
-      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)));
+      TNode<HeapNumber> heap_number = CAST(LoadObjectField(holder, offset));
+      var_double_value->Bind(LoadHeapNumberValue(heap_number));
     }
     Goto(rebox_double);
   }
@@ -468,7 +462,7 @@ void AccessorAssembler::HandleLoadICSmiHandlerLoadNamedCase(
          &module_export, &interceptor);
 
   BIND(&field);
-  HandleLoadField(holder, handler_word, var_double_value, rebox_double, miss,
+  HandleLoadField(holder, handler_word, var_double_value, rebox_double,
                   exit_point);
 
   BIND(&nonexistent);
@@ -1672,24 +1666,6 @@ 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,
-                       Label* miss, ExitPoint* exit_point);
+                       ExitPoint* exit_point);
 
   void EmitAccessCheck(TNode<Context> expected_native_context,
                        TNode<Context> context, Node* receiver,

src/objects/map.cc

@@ -787,8 +787,7 @@ 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(),
-        descriptors->GetDetails(modify_index).representation(), old_constness,
+        details.representation(), details.representation(), old_constness,
         new_constness, old_field_type, MaybeHandle<Object>(), new_field_type,
         MaybeHandle<Object>());
   }

src/objects/property-details.h

@@ -112,9 +112,7 @@ 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() || (!FLAG_unbox_double_fields && IsDouble()) ||
-            IsHeapObject()) &&
-           other.IsTagged();
+    return (IsSmi() || 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::Smi(), any_type);
+                                      Representation::Double(), 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::Double(), any_type);
+        Representation::Tagged(), any_type);
     expectations.SetDataField(detach_property_at_index,
                               PropertyConstness::kConst,
-                              Representation::Double(), any_type);
+                              Representation::Tagged(), any_type);
     CHECK(map->is_deprecated());
     CHECK(expectations.Check(*detach_point_map,
                              detach_point_map->NumberOfOwnDescriptors()));
@@ -814,9 +814,7 @@ TEST(GeneralizeDoubleFieldToTagged) {
   TestGeneralizeField(
       {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_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);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type});
 }
 
 TEST(GeneralizeHeapObjectFieldToTagged) {
@@ -2299,7 +2297,7 @@ TEST(ElementsKindTransitionFromMapOwningDescriptor) {
         {PropertyConstness::kMutable, Representation::Double(), any_type},
         {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
         {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-        FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
+        true);
   }
 }
 
@@ -2367,7 +2365,7 @@ TEST(ElementsKindTransitionFromMapNotOwningDescriptor) {
         {PropertyConstness::kMutable, Representation::Double(), any_type},
         {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
         {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-        FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
+        true);
   }
 }
 
@@ -2410,8 +2408,7 @@ TEST(PrototypeTransitionFromMapOwningDescriptor) {
   TestGeneralizeFieldWithSpecialTransition(
       config, {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, true);
 }
 
 TEST(PrototypeTransitionFromMapNotOwningDescriptor) {
@@ -2464,8 +2461,7 @@ TEST(PrototypeTransitionFromMapNotOwningDescriptor) {
   TestGeneralizeFieldWithSpecialTransition(
       config, {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, true);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

test/mjsunit/regress/regress-997485.js

-// 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();
-
-})();