[runtime] Remove --modify-field-representation-inplace flag

which was enabled a long ago and is not supposed to be disabled.

In addition this CL adds Representation::MightCauseMapDeprecation()
predicate and ensures it's consistent with the existing
MostGenericInPlaceChange() and CanBeInPlaceChangedTo().

Bug: v8:11104, v8:8865
Change-Id: Ia8046b76822c9b20fe3ce85de6b98570334aad21
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2527088
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71151}

src/flags/flag-definitions.h

@@ -1355,8 +1355,6 @@ DEFINE_GENERIC_IMPLICATION(
                   v8::tracing::TracingCategoryObserver::ENABLED_BY_NATIVE))
 DEFINE_BOOL_READONLY(fast_map_update, false,
                      "enable fast map update by caching the migration target")
-DEFINE_BOOL(modify_field_representation_inplace, true,
-            "enable in-place field representation updates")
 DEFINE_INT(max_valid_polymorphic_map_count, 4,
            "maximum number of valid maps to track in POLYMORPHIC state")
 

src/init/bootstrapper.cc

@@ -3603,10 +3603,11 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
               .ToHandleChecked();
 
     // done
-    // TODO(bmeurer): Once FLAG_modify_field_representation_inplace is always
-    // on, we can say Representation::HeapObject() here and have the inplace
-    // update logic take care of the case where someone ever stores a Smi into
-    // the done field.
+    // TODO(ishell): Consider using Representation::HeapObject() here and rely
+    // on the inplace update logic to take care of the case where someone ever
+    // stores a Smi into the done field. The logic works fine but in --jitless
+    // mode FLAG_track_heap_object_fields is off and the logic doesn't handle
+    // generalizations of HeapObject representation properly.
     map = Map::CopyWithField(isolate(), map, factory->done_string(),
                              FieldType::Any(isolate()), NONE,
                              PropertyConstness::kConst,

src/objects/map-inl.h

@@ -565,11 +565,7 @@ bool Map::is_stable() const {
 bool Map::CanBeDeprecated() const {
   for (InternalIndex i : IterateOwnDescriptors()) {
     PropertyDetails details = instance_descriptors(kRelaxedLoad).GetDetails(i);
-    if (details.representation().IsNone()) return true;
-    if (details.representation().IsSmi()) return true;
-    if (details.representation().IsDouble() && FLAG_unbox_double_fields)
-      return true;
-    if (details.representation().IsHeapObject()) return true;
+    if (details.representation().MightCauseMapDeprecation()) return true;
     if (details.kind() == kData && details.location() == kDescriptor) {
       return true;
     }

src/objects/map-updater.cc

@@ -217,6 +217,14 @@ MapUpdater::State MapUpdater::TryReconfigureToDataFieldInplace() {
 
   PropertyDetails old_details =
       old_descriptors_->GetDetails(modified_descriptor_);
+
+  if (old_details.attributes() != new_attributes_ ||
+      old_details.kind() != new_kind_ ||
+      old_details.location() != new_location_) {
+    // These changes can't be done in-place.
+    return state_;  // Not done yet.
+  }
+
   Representation old_representation = old_details.representation();
   if (!old_representation.CanBeInPlaceChangedTo(new_representation_)) {
     return state_;  // Not done yet.

src/objects/property-details.h

@@ -7,7 +7,6 @@
 
 #include "include/v8.h"
 #include "src/utils/allocation.h"
-// TODO(bmeurer): Remove once FLAG_modify_field_representation_inplace is gone.
 #include "src/base/bit-field.h"
 #include "src/flags/flags.h"
 
@@ -104,24 +103,41 @@ class Representation {
     return Equals(other);
   }
 
+  // Returns true if a change from this representation to a more general one
+  // might cause a map deprecation.
+  bool MightCauseMapDeprecation() const {
+    // HeapObject to tagged representation change can be done in-place.
+    if (IsTagged() || IsHeapObject()) return false;
+    // When double fields unboxing is enabled, there must be a map deprecation.
+    // Boxed double to tagged transition is always done in-place.
+    if (IsDouble()) return FLAG_unbox_double_fields;
+    // None to double and smi to double representation changes require
+    // deprecation, because doubles might require box allocation, see
+    // CanBeInPlaceChangedTo().
+    DCHECK(IsNone() || IsSmi());
+    return true;
+  }
+
   bool CanBeInPlaceChangedTo(const Representation& other) const {
+    if (Equals(other)) return true;
     // If it's just a representation generalization case (i.e. property kind and
     // attributes stays unchanged) it's fine to transition from None to anything
     // but double without any modification to the object, because the default
     // uninitialized value for representation None can be overwritten by both
     // 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();
+    if (!other.IsTagged()) return false;
+    // Everything but unboxed doubles can be in-place changed to Tagged.
+    if (FLAG_unbox_double_fields && IsDouble()) return false;
+    DCHECK(IsSmi() || (!FLAG_unbox_double_fields && IsDouble()) ||
+           IsHeapObject());
+    return true;
   }
 
   // Return the most generic representation that this representation can be
-  // changed to in-place. If in-place representation changes are disabled, then
-  // this will return the current representation.
+  // changed to in-place. If an in-place representation change is not allowed,
+  // then this will return the current representation.
   Representation MostGenericInPlaceChange() const {
-    if (!FLAG_modify_field_representation_inplace) return *this;
     // Everything but unboxed doubles can be in-place changed to Tagged.
     if (FLAG_unbox_double_fields && IsDouble()) return Representation::Double();
     return Representation::Tagged();

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

@@ -3,16 +3,15 @@
 // found in the LICENSE file.
 
 #include <stdlib.h>
-#include <utility>
-
-#include "test/cctest/test-api.h"
 
-#include "src/init/v8.h"
+#include <utility>
 
+#include "src/base/logging.h"
 #include "src/execution/execution.h"
 #include "src/handles/global-handles.h"
 #include "src/heap/factory-inl.h"
 #include "src/ic/stub-cache.h"
+#include "src/init/v8.h"
 #include "src/objects/field-type.h"
 #include "src/objects/heap-number-inl.h"
 #include "src/objects/objects-inl.h"
@@ -20,6 +19,7 @@
 #include "src/objects/struct-inl.h"
 #include "src/objects/transitions.h"
 #include "src/utils/ostreams.h"
+#include "test/cctest/test-api.h"
 
 namespace v8 {
 namespace internal {
@@ -823,9 +823,8 @@ TEST(GeneralizeSmiFieldToTagged) {
   TestGeneralizeField(
       {PropertyConstness::kMutable, Representation::Smi(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace,
-      FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, false,
+      true);
 }
 
 TEST(GeneralizeDoubleFieldToTagged) {
@@ -841,8 +840,7 @@ TEST(GeneralizeDoubleFieldToTagged) {
       {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);
+      FLAG_unbox_double_fields, !FLAG_unbox_double_fields);
 }
 
 TEST(GeneralizeHeapObjectFieldToTagged) {
@@ -857,9 +855,8 @@ TEST(GeneralizeHeapObjectFieldToTagged) {
   TestGeneralizeField(
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
       {PropertyConstness::kMutable, Representation::Smi(), any_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace,
-      FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, false,
+      true);
 }
 
 TEST(GeneralizeHeapObjectFieldToHeapObject) {
@@ -1039,9 +1036,8 @@ namespace {
 //
 // where "p2A" and "p2B" differ only in the attributes.
 //
-void TestReconfigureDataFieldAttribute_GeneralizeField(
-    const CRFTData& from, const CRFTData& to, const CRFTData& expected,
-    bool expected_deprecation) {
+void TestReconfigureDataFieldAttribute_GeneralizeFieldWithDeprecation(
+    const CRFTData& from, const CRFTData& to, const CRFTData& expected) {
   Isolate* isolate = CcTest::i_isolate();
 
   Expectations expectations(isolate);
@@ -1114,25 +1110,20 @@ void TestReconfigureDataFieldAttribute_GeneralizeField(
     expectations.SetDataField(i, expected.constness, expected.representation,
                               expected.type);
   }
-  if (expected_deprecation) {
-    // |map| should be deprecated and |new_map| should match new expectations.
-    CHECK(map->is_deprecated());
-    CHECK(!code_field_type->marked_for_deoptimization());
-    CHECK(!code_field_repr->marked_for_deoptimization());
-    CHECK(!code_field_const->marked_for_deoptimization());
-    CHECK_NE(*map, *new_map);
+  // |map| should be deprecated and |new_map| should match new expectations.
+  CHECK(map->is_deprecated());
+  CHECK(!code_field_type->marked_for_deoptimization());
+  CHECK(!code_field_repr->marked_for_deoptimization());
+  CHECK(!code_field_const->marked_for_deoptimization());
+  CHECK_NE(*map, *new_map);
 
-    CHECK(!new_map->is_deprecated());
-    CHECK(expectations.Check(*new_map));
+  CHECK(!new_map->is_deprecated());
+  CHECK(expectations.Check(*new_map));
 
-    // Update deprecated |map|, it should become |new_map|.
-    Handle<Map> updated_map = Map::Update(isolate, map);
-    CHECK_EQ(*new_map, *updated_map);
-    CheckMigrationTarget(isolate, *map, *updated_map);
-  } else {
-    CHECK(!map->is_deprecated());
-    CHECK(expectations.Check(*map));
-  }
+  // Update deprecated |map|, it should become |new_map|.
+  Handle<Map> updated_map = Map::Update(isolate, map);
+  CHECK_EQ(*new_map, *updated_map);
+  CheckMigrationTarget(isolate, *map, *updated_map);
 }
 
 // This test ensures that trivial field generalization (from HeapObject to
@@ -1145,6 +1136,7 @@ void TestReconfigureDataFieldAttribute_GeneralizeField(
 //
 // where "p2A" and "p2B" differ only in the attributes.
 //
+// TODO(ishell): rename to XXX_GeneralizeFieldInplace here and around.
 void TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
     const CRFTData& from, const CRFTData& to, const CRFTData& expected,
     bool expected_field_owner_dependency = true) {
@@ -1236,6 +1228,24 @@ void TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
   CHECK_EQ(*new_map, *updated_map);
 }
 
+// This is an entry point to either
+// TestReconfigureDataFieldAttribute_GeneralizeFieldWithDeprecation() or
+// TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial() depending on
+// whether the deprecation is expected or not. In the latter case the relevant
+// code dependencies must be triggered.
+void TestReconfigureDataFieldAttribute_GeneralizeField(
+    const CRFTData& from, const CRFTData& to, const CRFTData& expected,
+    bool expected_deprecation) {
+  if (expected_deprecation) {
+    TestReconfigureDataFieldAttribute_GeneralizeFieldWithDeprecation(from, to,
+                                                                     expected);
+  } else {
+    const bool expected_field_owner_dependency = true;
+    TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
+        from, to, expected, expected_field_owner_dependency);
+  }
+}
+
 }  // namespace
 
 TEST(ReconfigureDataFieldAttribute_GeneralizeSmiFieldToDouble) {
@@ -1275,29 +1285,25 @@ TEST(ReconfigureDataFieldAttribute_GeneralizeSmiFieldToTagged) {
   Handle<FieldType> value_type =
       FieldType::Class(Map::Create(isolate, 0), isolate);
 
-  TestReconfigureDataFieldAttribute_GeneralizeField(
+  TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
       {PropertyConstness::kConst, Representation::Smi(), any_type},
       {PropertyConstness::kConst, Representation::HeapObject(), value_type},
-      {PropertyConstness::kConst, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kConst, Representation::Tagged(), any_type});
 
-  TestReconfigureDataFieldAttribute_GeneralizeField(
+  TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
       {PropertyConstness::kConst, Representation::Smi(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type});
 
-  TestReconfigureDataFieldAttribute_GeneralizeField(
+  TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
       {PropertyConstness::kMutable, Representation::Smi(), any_type},
       {PropertyConstness::kConst, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type});
 
-  TestReconfigureDataFieldAttribute_GeneralizeField(
+  TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
       {PropertyConstness::kMutable, Representation::Smi(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type});
 }
 
 TEST(ReconfigureDataFieldAttribute_GeneralizeDoubleFieldToTagged) {
@@ -1313,25 +1319,25 @@ TEST(ReconfigureDataFieldAttribute_GeneralizeDoubleFieldToTagged) {
       {PropertyConstness::kConst, Representation::Double(), any_type},
       {PropertyConstness::kConst, Representation::HeapObject(), value_type},
       {PropertyConstness::kConst, Representation::Tagged(), any_type},
-      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
+      FLAG_unbox_double_fields);
 
   TestReconfigureDataFieldAttribute_GeneralizeField(
       {PropertyConstness::kConst, 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);
 
   TestReconfigureDataFieldAttribute_GeneralizeField(
       {PropertyConstness::kMutable, Representation::Double(), any_type},
       {PropertyConstness::kConst, Representation::HeapObject(), value_type},
       {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      FLAG_unbox_double_fields || !FLAG_modify_field_representation_inplace);
+      FLAG_unbox_double_fields);
 
   TestReconfigureDataFieldAttribute_GeneralizeField(
       {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);
 }
 
 TEST(ReconfigureDataFieldAttribute_GeneralizeHeapObjFieldToHeapObj) {
@@ -1414,11 +1420,10 @@ TEST(ReconfigureDataFieldAttribute_GeneralizeHeapObjectFieldToTagged) {
   Handle<FieldType> value_type =
       FieldType::Class(Map::Create(isolate, 0), isolate);
 
-  TestReconfigureDataFieldAttribute_GeneralizeField(
+  TestReconfigureDataFieldAttribute_GeneralizeFieldTrivial(
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
       {PropertyConstness::kMutable, Representation::Smi(), any_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type});
 }
 
 // Checks that given |map| is deprecated and that it updates to given |new_map|
@@ -2343,13 +2348,14 @@ TEST(ElementsKindTransitionFromMapOwningDescriptor) {
         {PropertyConstness::kMutable, Representation::Smi(), any_type},
         {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
         {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-        !FLAG_modify_field_representation_inplace);
+        false);
+
     TestGeneralizeFieldWithSpecialTransition(
         &configs[i],
         {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);
   }
 }
 
@@ -2410,13 +2416,14 @@ TEST(ElementsKindTransitionFromMapNotOwningDescriptor) {
         {PropertyConstness::kMutable, Representation::Smi(), any_type},
         {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
         {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-        !FLAG_modify_field_representation_inplace);
+        false);
+
     TestGeneralizeFieldWithSpecialTransition(
         &configs[i],
         {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);
   }
 }
 
@@ -2452,14 +2459,14 @@ TEST(PrototypeTransitionFromMapOwningDescriptor) {
   TestGeneralizeFieldWithSpecialTransition(
       &config, {PropertyConstness::kMutable, Representation::Smi(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, false);
+
   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);
+      FLAG_unbox_double_fields);
 }
 
 TEST(PrototypeTransitionFromMapNotOwningDescriptor) {
@@ -2505,14 +2512,14 @@ TEST(PrototypeTransitionFromMapNotOwningDescriptor) {
   TestGeneralizeFieldWithSpecialTransition(
       &config, {PropertyConstness::kMutable, Representation::Smi(), any_type},
       {PropertyConstness::kMutable, Representation::HeapObject(), value_type},
-      {PropertyConstness::kMutable, Representation::Tagged(), any_type},
-      !FLAG_modify_field_representation_inplace);
+      {PropertyConstness::kMutable, Representation::Tagged(), any_type}, false);
+
   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);
+      FLAG_unbox_double_fields);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -2799,15 +2806,8 @@ TEST(TransitionDataConstantToDataField) {
   TransitionToDataFieldOperator transition_op2(
       PropertyConstness::kMutable, Representation::Tagged(), any_type, value2);
 
-  if (FLAG_modify_field_representation_inplace) {
-    SameMapChecker checker;
-    TestTransitionTo(&transition_op1, &transition_op2, &checker);
-  } else {
-    FieldGeneralizationChecker checker(kPropCount - 1,
-                                       PropertyConstness::kMutable,
-                                       Representation::Tagged(), any_type);
-    TestTransitionTo(&transition_op1, &transition_op2, &checker);
-  }
+  SameMapChecker checker;
+  TestTransitionTo(&transition_op1, &transition_op2, &checker);
 }
 
 
@@ -3065,6 +3065,30 @@ TEST(NormalizeToMigrationTarget) {
   CHECK(new_normalized_map->is_migration_target());
 }
 
+TEST(RepresentationPredicatesAreInSync) {
+  STATIC_ASSERT(Representation::kNumRepresentations == 5);
+  static Representation reps[] = {
+      Representation::None(), Representation::Smi(), Representation::Double(),
+      Representation::HeapObject(), Representation::Tagged()};
+
+  for (Representation from : reps) {
+    Representation most_generic_rep = from.MostGenericInPlaceChange();
+    CHECK(from.CanBeInPlaceChangedTo(most_generic_rep));
+
+    bool might_be_deprecated = false;
+
+    for (Representation to : reps) {
+      // Skip representation narrowing cases.
+      if (!from.fits_into(to)) continue;
+
+      if (!from.CanBeInPlaceChangedTo(to)) {
+        might_be_deprecated = true;
+      }
+    }
+    CHECK_EQ(from.MightCauseMapDeprecation(), might_be_deprecated);
+  }
+}
+
 }  // namespace test_field_type_tracking
 }  // namespace compiler
 }  // namespace internal

test/mjsunit/compiler/field-representation-tracking.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --allow-natives-syntax --modify-field-representation-inplace
+// Flags: --allow-natives-syntax
 // Flags: --no-always-opt --opt
 
 // Test that code embedding accesses to a Smi field gets properly

test/mjsunit/field-representation-tracking.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --allow-natives-syntax --modify-field-representation-inplace
+// Flags: --allow-natives-syntax
 
 // Test that s->t field representation changes are done in-place.
 (function() {

test/mjsunit/track-fields.js

@@ -26,7 +26,6 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 // Flags: --track-fields --track-double-fields --allow-natives-syntax
-// Flags: --modify-field-representation-inplace
 
 // Test transitions caused by changes to field representations.