[ic] Remove SameValue optimisation for constant fields

We would previously try to preserve field constness if field assignment
was assigning the same value. It's unexpected that real-life code would
be assigning the same value multiple times to an intentionally constant
field, so this was additional bookkeeping with unclear value.

Replace this with not doing it, and considering any write to a constant
field to convert it to mutable. In particular, this means that stores to
existing constant fields in TurboFan become unconditional deopts, rather
than emitting additional code to check whether the value is the same.

Locally, this deopt doesn't fire on our peak-performance benchmarks.

Bug: v8:5495
Change-Id: I12216c5f10a00f42be32c64ca3afe7cf59b4e7f3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3976516
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83955}

src/compiler/js-native-context-specialization.cc

@@ -2881,6 +2881,20 @@ JSNativeContextSpecialization::BuildPropertyStore(
     DCHECK(access_mode == AccessMode::kStore ||
            access_mode == AccessMode::kStoreInLiteral ||
            access_mode == AccessMode::kDefine);
+
+    const bool store_to_existing_constant_field =
+        access_info.IsFastDataConstant() && access_mode == AccessMode::kStore &&
+        !access_info.HasTransitionMap();
+    if (store_to_existing_constant_field) {
+      Node* deoptimize = graph()->NewNode(
+          common()->Deoptimize(DeoptimizeReason::kConstantFieldWrite, {}),
+          frame_state, effect, control);
+      NodeProperties::MergeControlToEnd(graph(), common(), deoptimize);
+      Revisit(graph()->end());
+      return ValueEffectControl(jsgraph()->Dead(), jsgraph()->Dead(),
+                                jsgraph()->Dead());
+    }
+
     FieldIndex const field_index = access_info.field_index();
     Type const field_type = access_info.field_type();
     MachineRepresentation const field_representation =
@@ -2893,9 +2907,7 @@ JSNativeContextSpecialization::BuildPropertyStore(
               AccessBuilder::ForJSObjectPropertiesOrHashKnownPointer()),
           storage, effect, control);
     }
-    bool store_to_existing_constant_field = access_info.IsFastDataConstant() &&
+
-                                            access_mode == AccessMode::kStore &&
-                                            !access_info.HasTransitionMap();
     FieldAccess field_access = {
         kTaggedBase,
         field_index.offset(),
@@ -2948,40 +2960,11 @@ JSNativeContextSpecialization::BuildPropertyStore(
           field_access.name = MaybeHandle<Name>();
           field_access.machine_type = MachineType::Float64();
         }
-        if (store_to_existing_constant_field) {
-          DCHECK(!access_info.HasTransitionMap());
-          // If the field is constant check that the value we are going
-          // to store matches current value.
-          Node* current_value = effect = graph()->NewNode(
-              simplified()->LoadField(field_access), storage, effect, control);
-
-          Node* check =
-              graph()->NewNode(simplified()->SameValue(), current_value, value);
-          effect = graph()->NewNode(
-              simplified()->CheckIf(DeoptimizeReason::kWrongValue), check,
-              effect, control);
-          return ValueEffectControl(value, effect, control);
-        }
         break;
       }
       case MachineRepresentation::kTaggedSigned:
       case MachineRepresentation::kTaggedPointer:
       case MachineRepresentation::kTagged:
-        if (store_to_existing_constant_field) {
-          DCHECK(!access_info.HasTransitionMap());
-          // If the field is constant check that the value we are going
-          // to store matches current value.
-          Node* current_value = effect = graph()->NewNode(
-              simplified()->LoadField(field_access), storage, effect, control);
-
-          Node* check = graph()->NewNode(simplified()->SameValueNumbersOnly(),
-                                         current_value, value);
-          effect = graph()->NewNode(
-              simplified()->CheckIf(DeoptimizeReason::kWrongValue), check,
-              effect, control);
-          return ValueEffectControl(value, effect, control);
-        }
-
         if (field_representation == MachineRepresentation::kTaggedSigned) {
           value = effect = graph()->NewNode(
               simplified()->CheckSmi(FeedbackSource()), value, effect, control);

src/deoptimizer/deoptimize-reason.h

@@ -13,6 +13,7 @@ namespace internal {
 #define DEOPTIMIZE_REASON_LIST(V)                                              \
   V(ArrayBufferWasDetached, "array buffer was detached")                       \
   V(BigIntTooBig, "BigInt too big")                                            \
+  V(ConstantFieldWrite, "write to constant field")                             \
   V(CowArrayElementsChanged, "copy-on-write array's elements changed")         \
   V(CouldNotGrowElements, "failed to grow elements store")                     \
   V(PrepareForOnStackReplacement, "prepare for on stack replacement (OSR)")    \

src/ic/accessor-assembler.cc

@@ -1305,25 +1305,12 @@ void AccessorAssembler::HandleStoreICHandlerCase(
         GotoIf(IsSetWord32(details, kTypeAndReadOnlyMask), miss);
 
         if (V8_DICT_PROPERTY_CONST_TRACKING_BOOL) {
-          GotoIf(IsPropertyDetailsConst(details), &if_constant);
+          GotoIf(IsPropertyDetailsConst(details), miss);
         }
 
         StoreValueByKeyIndex<PropertyDictionary>(
             properties, var_name_index.value(), p->value());
         Return(p->value());
-
-        if (V8_DICT_PROPERTY_CONST_TRACKING_BOOL) {
-          BIND(&if_constant);
-          {
-            TNode<Object> prev_value =
-                LoadValueByKeyIndex(properties, var_name_index.value());
-            BranchIfSameValue(prev_value, p->value(), &done, miss,
-                              SameValueMode::kNumbersOnly);
-          }
-
-          BIND(&done);
-          Return(p->value());
-        }
       }
     }
     BIND(&if_fast_smi);
@@ -1616,12 +1603,7 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
         } else {
           TNode<HeapNumber> heap_number =
               CAST(LoadObjectField(object, field_offset));
-          Label store_value(this);
+          GotoIf(IsPropertyDetailsConst(details), slow);
-          GotoIfNot(IsPropertyDetailsConst(details), &store_value);
-          TNode<Float64T> current_value = LoadHeapNumberValue(heap_number);
-          BranchIfSameNumberValue(current_value, double_value, &store_value,
-                                  slow);
-          BIND(&store_value);
           StoreHeapNumberValue(heap_number, double_value);
         }
         Goto(&done);
@@ -1632,12 +1614,7 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
         if (do_transitioning_store) {
           StoreMap(object, object_map);
         } else {
-          Label if_mutable(this);
+          GotoIf(IsPropertyDetailsConst(details), slow);
-          GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
-          TNode<Object> current_value = LoadObjectField(object, field_offset);
-          BranchIfSameValue(current_value, value, &done, slow,
-                            SameValueMode::kNumbersOnly);
-          BIND(&if_mutable);
         }
         StoreObjectField(object, field_offset, value);
         Goto(&done);
@@ -1690,24 +1667,14 @@ void AccessorAssembler::OverwriteExistingFastDataProperty(
           TNode<Float64T> double_value = ChangeNumberToFloat64(CAST(value));
 
           Label if_mutable(this);
-          GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
+          GotoIf(IsPropertyDetailsConst(details), slow);
-          TNode<Float64T> current_value = LoadHeapNumberValue(heap_number);
-          BranchIfSameNumberValue(current_value, double_value, &done, slow);
-
-          BIND(&if_mutable);
           StoreHeapNumberValue(heap_number, double_value);
           Goto(&done);
         }
         BIND(&tagged_rep);
         {
           Label if_mutable(this);
-          GotoIfNot(IsPropertyDetailsConst(details), &if_mutable);
+          GotoIf(IsPropertyDetailsConst(details), slow);
-          TNode<Object> current_value =
-              LoadPropertyArrayElement(properties, backing_store_index);
-          BranchIfSameValue(current_value, value, &done, slow,
-                            SameValueMode::kNumbersOnly);
-
-          BIND(&if_mutable);
           StorePropertyArrayElement(properties, backing_store_index, value);
           Goto(&done);
         }

src/objects/js-objects.cc

@@ -1527,13 +1527,14 @@ Maybe<bool> JSReceiver::ValidateAndApplyPropertyDescriptor(
     return Just(true);
   }
   // 3. If every field in Desc is absent, return true. (This also has a shortcut
-  // not in the spec: if every field value matches the current value, return.)
+  // not in the spec: if every field value matches the current value, return.
+  // This shortcut doesn't apply to the `value` field itself, because of
+  // constness).
   if ((!desc->has_enumerable() ||
        desc->enumerable() == current->enumerable()) &&
       (!desc->has_configurable() ||
        desc->configurable() == current->configurable()) &&
-      (!desc->has_value() ||
+      !desc->has_value() &&
-       (current->has_value() && current->value()->SameValue(*desc->value()))) &&
       (!desc->has_writable() ||
        (current->has_writable() && current->writable() == desc->writable())) &&
       (!desc->has_get() ||

src/objects/lookup.cc

@@ -375,11 +375,11 @@ void LookupIterator::PrepareForDataProperty(Handle<Object> value) {
     // Check that current value matches new value otherwise we should make
     // the property mutable.
     if (holder->HasFastProperties(isolate_)) {
-      if (!IsConstFieldValueEqualTo(*value)) {
+      if (!IsConstFieldUninitialized(*value)) {
         new_constness = PropertyConstness::kMutable;
       }
     } else if (V8_DICT_PROPERTY_CONST_TRACKING_BOOL) {
-      if (!IsConstDictValueEqualTo(*value)) {
+      if (!IsConstDictValueUninitialized(*value)) {
         property_details_ =
             property_details_.CopyWithConstness(PropertyConstness::kMutable);
 
@@ -902,7 +902,7 @@ Handle<Object> LookupIterator::FetchValue(
   return handle(result, isolate_);
 }
 
-bool LookupIterator::IsConstFieldValueEqualTo(Object value) const {
+bool LookupIterator::IsConstFieldUninitialized(Object value) const {
   DCHECK(!IsElement(*holder_));
   DCHECK(holder_->HasFastProperties(isolate_));
   DCHECK_EQ(PropertyLocation::kField, property_details_.location());
@@ -921,29 +921,21 @@ bool LookupIterator::IsConstFieldValueEqualTo(Object value) const {
     uint64_t bits;
     Object current_value = holder->RawFastPropertyAt(isolate_, field_index);
     DCHECK(current_value.IsHeapNumber(isolate_));
-    bits = HeapNumber::cast(current_value).value_as_bits(kRelaxedLoad);
     // Use bit representation of double to check for hole double, since
     // manipulating the signaling NaN used for the hole in C++, e.g. with
     // base::bit_cast or value(), will change its value on ia32 (the x87
     // stack is used to return values and stores to the stack silently clear the
     // signalling bit).
-    if (bits == kHoleNanInt64) {
+    bits = HeapNumber::cast(current_value).value_as_bits(kRelaxedLoad);
-      // Uninitialized double field.
+    // Uninitialized double field.
-      return true;
+    return bits == kHoleNanInt64;
-    }
-    return Object::SameNumberValue(base::bit_cast<double>(bits),
-                                   value.Number());
   } else {
     Object current_value = holder->RawFastPropertyAt(isolate_, field_index);
-    if (current_value.IsUninitialized(isolate()) || current_value == value) {
+    return current_value.IsUninitialized(isolate());
-      return true;
-    }
-    return current_value.IsNumber(isolate_) && value.IsNumber(isolate_) &&
-           Object::SameNumberValue(current_value.Number(), value.Number());
   }
 }
 
-bool LookupIterator::IsConstDictValueEqualTo(Object value) const {
+bool LookupIterator::IsConstDictValueUninitialized(Object value) const {
   DCHECK(!IsElement(*holder_));
   DCHECK(!holder_->HasFastProperties(isolate_));
   DCHECK(!holder_->IsJSGlobalObject());
@@ -968,11 +960,7 @@ bool LookupIterator::IsConstDictValueEqualTo(Object value) const {
     current_value = dict.ValueAt(dictionary_entry());
   }
 
-  if (current_value.IsUninitialized(isolate()) || current_value == value) {
+  return current_value.IsUninitialized(isolate());
-    return true;
-  }
-  return current_value.IsNumber(isolate_) && value.IsNumber(isolate_) &&
-         Object::SameNumberValue(current_value.Number(), value.Number());
 }
 
 int LookupIterator::GetFieldDescriptorIndex() const {
@@ -1059,7 +1047,7 @@ void LookupIterator::WriteDataValue(Handle<Object> value,
       // equal to |value|.
       DCHECK_IMPLIES(!initializing_store && property_details_.constness() ==
                                                 PropertyConstness::kConst,
-                     IsConstFieldValueEqualTo(*value));
+                     IsConstFieldUninitialized(*value));
       JSObject::cast(*holder).WriteToField(descriptor_number(),
                                            property_details_, *value);
     } else {
@@ -1083,7 +1071,7 @@ void LookupIterator::WriteDataValue(Handle<Object> value,
     DCHECK_IMPLIES(
         V8_DICT_PROPERTY_CONST_TRACKING_BOOL && !initializing_store &&
             property_details_.constness() == PropertyConstness::kConst,
-        holder->IsJSProxy(isolate_) || IsConstDictValueEqualTo(*value));
+        holder->IsJSProxy(isolate_) || IsConstDictValueUninitialized(*value));
 
     if (V8_ENABLE_SWISS_NAME_DICTIONARY_BOOL) {
       SwissNameDictionary dictionary =

src/objects/lookup.h

@@ -257,8 +257,8 @@ class V8_EXPORT_PRIVATE LookupIterator final {
   void RestartInternal(InterceptorState interceptor_state);
   Handle<Object> FetchValue(AllocationPolicy allocation_policy =
                                 AllocationPolicy::kAllocationAllowed) const;
-  bool IsConstFieldValueEqualTo(Object value) const;
+  bool IsConstFieldUninitialized(Object value) const;
-  bool IsConstDictValueEqualTo(Object value) const;
+  bool IsConstDictValueUninitialized(Object value) const;
 
   template <bool is_element>
   void ReloadPropertyInformation();

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

@@ -2812,9 +2812,7 @@ MaybeHandle<Object> Call(Isolate* isolate, Handle<JSFunction> function,
 
 void TestStoreToConstantField(const char* store_func_source,
                               Handle<Object> value1, Handle<Object> value2,
-                              Representation expected_rep,
+                              Representation expected_rep) {
-                              PropertyConstness expected_constness,
-                              int store_repetitions) {
   Isolate* isolate = CcTest::i_isolate();
   CompileRun(store_func_source);
 
@@ -2825,9 +2823,7 @@ void TestStoreToConstantField(const char* store_func_source,
   // Store value1 to obj1 and check that it got property with expected
   // representation and constness.
   Handle<JSObject> obj1 = isolate->factory()->NewJSObjectFromMap(initial_map);
-  for (int i = 0; i < store_repetitions; i++) {
+  Call(isolate, store_func, obj1, value1).Check();
-    Call(isolate, store_func, obj1, value1).Check();
-  }
 
   Handle<Map> map(obj1->map(), isolate);
   CHECK(!map->is_dictionary_map());
@@ -2870,28 +2866,22 @@ void TestStoreToConstantField(const char* store_func_source,
             .GetDetails(first)
             .representation()
             .Equals(expected_rep));
-  CHECK_EQ(expected_constness,
+  CHECK_EQ(PropertyConstness::kMutable,
            map->instance_descriptors(isolate).GetDetails(first).constness());
 }
 
-void TestStoreToConstantField_PlusMinusZero(const char* store_func_source,
+void TestStoreToConstantField_PlusMinusZero(const char* store_func_source) {
-                                            int store_repetitions) {
   Isolate* isolate = CcTest::i_isolate();
   CompileRun(store_func_source);
 
   Handle<Object> minus_zero = isolate->factory()->NewNumber(-0.0);
   Handle<Object> plus_zero = isolate->factory()->NewNumber(0.0);
 
-  // +0 and -0 are treated as not equal upon stores.
-  const PropertyConstness kExpectedFieldConstness = PropertyConstness::kMutable;
-
   TestStoreToConstantField(store_func_source, minus_zero, plus_zero,
-                           Representation::Double(), kExpectedFieldConstness,
+                           Representation::Double());
-                           store_repetitions);
 }
 
-void TestStoreToConstantField_NaN(const char* store_func_source,
+void TestStoreToConstantField_NaN(const char* store_func_source) {
-                                  int store_repetitions) {
   Isolate* isolate = CcTest::i_isolate();
   CompileRun(store_func_source);
 
@@ -2907,10 +2897,8 @@ void TestStoreToConstantField_NaN(const char* store_func_source,
   Handle<Object> nan1 = isolate->factory()->NewNumber(nan_double1);
   Handle<Object> nan2 = isolate->factory()->NewNumber(nan_double2);
 
-  // NaNs with different bit patters are treated as equal upon stores.
   TestStoreToConstantField(store_func_source, nan1, nan2,
-                           Representation::Double(), PropertyConstness::kConst,
+                           Representation::Double());
-                           store_repetitions);
 }
 
 }  // namespace
@@ -2925,11 +2913,9 @@ TEST(StoreToConstantField_PlusMinusZero) {
       "  %SetNamedProperty(o, 'v', v);"
       "}";
 
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 1);
+  TestStoreToConstantField_PlusMinusZero(store_func_source);
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 3);
 
-  TestStoreToConstantField_NaN(store_func_source, 1);
+  TestStoreToConstantField_NaN(store_func_source);
-  TestStoreToConstantField_NaN(store_func_source, 2);
 }
 
 TEST(StoreToConstantField_ObjectDefineProperty) {
@@ -2945,11 +2931,9 @@ TEST(StoreToConstantField_ObjectDefineProperty) {
       "                         enumerable: true});"
       "}";
 
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 1);
+  TestStoreToConstantField_PlusMinusZero(store_func_source);
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 3);
 
-  TestStoreToConstantField_NaN(store_func_source, 1);
+  TestStoreToConstantField_NaN(store_func_source);
-  TestStoreToConstantField_NaN(store_func_source, 2);
 }
 
 TEST(StoreToConstantField_ReflectSet) {
@@ -2961,11 +2945,9 @@ TEST(StoreToConstantField_ReflectSet) {
       "  Reflect.set(o, 'v', v);"
       "}";
 
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 1);
+  TestStoreToConstantField_PlusMinusZero(store_func_source);
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 3);
 
-  TestStoreToConstantField_NaN(store_func_source, 1);
+  TestStoreToConstantField_NaN(store_func_source);
-  TestStoreToConstantField_NaN(store_func_source, 2);
 }
 
 TEST(StoreToConstantField_StoreIC) {
@@ -2977,11 +2959,9 @@ TEST(StoreToConstantField_StoreIC) {
       "  o.v = v;"
       "}";
 
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 1);
+  TestStoreToConstantField_PlusMinusZero(store_func_source);
-  TestStoreToConstantField_PlusMinusZero(store_func_source, 3);
 
-  TestStoreToConstantField_NaN(store_func_source, 1);
+  TestStoreToConstantField_NaN(store_func_source);
-  TestStoreToConstantField_NaN(store_func_source, 2);
 }
 
 TEST(NormalizeToMigrationTarget) {

test/mjsunit/const-dict-tracking.js

@@ -42,14 +42,6 @@ function MakeFunctionWithUniqueSFI(...args) {
 
   // Allocate feedback vector, but we don't want to optimize the function.
   %PrepareFunctionForOptimization(update_z);
-  for (var i = 0; i < 4; i++) {
-    // Overwriting with same value maintains const-ness.
-    update_z(1);
-  }
-
-
-  assertTrue(%HasOwnConstDataProperty(proto, "z"));
-
   update_z(2);
 
   assertFalse(%HasOwnConstDataProperty(proto, "z"));

test/mjsunit/const-field-tracking.js

@@ -148,88 +148,3 @@ function TestLoadFromConstantFieldOfAPrototype(the_value, other_value) {
   var other_value = new V();
   TestLoadFromConstantFieldOfAPrototype(the_value, other_value);
 })();
-
-
-//
-// Store to constant field of a constant object.
-//
-function TestStoreToConstantFieldOfConstantObject(the_value, other_value) {
-  function A(v) { this.v = v; }
-  function O() { this.a = new A(the_value); }
-  var the_object = new O();
-
-  // Ensure that {the_object.a}'s map is not stable to complicate compiler's
-  // life.
-  new A(the_value).blah = 0;
-
-  // Ensure that constant tracking is enabled for {contant_object}.
-  delete global.constant_object;
-  global.constant_object = the_object;
-  assertEquals(the_object, constant_object);
-
-  assertTrue(%HasFastProperties(the_object));
-
-  // {constant_object} is known to the compiler via global property cell
-  // tracking.
-  var store = MakeFunctionWithUniqueSFI("v", "constant_object.a.v = v;");
-  %PrepareFunctionForOptimization(store);
-  store(the_value);
-  store(the_value);
-  %OptimizeFunctionOnNextCall(store);
-  store(the_value);
-  assertEquals(the_value, constant_object.a.v);
-  assertOptimized(store);
-  // Storing of the same value does not deoptimize.
-  store(the_value);
-  assertEquals(the_value, constant_object.a.v);
-  assertOptimized(store);
-
-  var a = new A(other_value);
-
-  if (typeof the_value == "function" || typeof the_value == "object") {
-    // For heap object fields "field-owner" dependency is installed for
-    // any access of the field, therefore making constant field mutable by
-    // assigning other value to some other instance of A should already
-    // trigger deoptimization.
-    assertTrue(%HaveSameMap(a, the_object.a));
-    new A(the_value).v = other_value;
-    assertTrue(%HaveSameMap(a, new A(the_value)));
-    assertTrue(%HaveSameMap(a, the_object.a));
-    assertUnoptimized(store);
-  } else {
-    assertOptimized(store);
-  }
-  // Storing other value deoptimizes because of failed value check.
-  store(other_value);
-  assertUnoptimized(store);
-  assertEquals(other_value, constant_object.a.v);
-}
-
-// Test constant tracking with Smi values.
-(function() {
-  var the_value = 42;
-  var other_value = 153;
-  TestStoreToConstantFieldOfConstantObject(the_value, other_value);
-})();
-
-// Test constant tracking with double values.
-(function() {
-  var the_value = 0.9;
-  var other_value = 0.42
-  TestStoreToConstantFieldOfConstantObject(the_value, other_value);
-})();
-
-// Test constant tracking with function values.
-(function() {
-  var the_value = function V() {};
-  var other_value = function W() {};
-  TestStoreToConstantFieldOfConstantObject(the_value, other_value);
-})();
-
-// Test constant tracking with heap object values.
-(function() {
-  function V() {}
-  var the_value = new V();
-  var other_value = new V();
-  TestStoreToConstantFieldOfConstantObject(the_value, other_value);
-})();