[dict-proto] TF support for constants in dictionary mode protos, pt. 3

This is a reland of
https://chromium-review.googlesource.com/c/v8/v8/+/2720300.
As compared to the original version, it adds
--no-stress-flush-bytecode to the const-dict-tracking.js test

Original description:

This CL is part of a  series that implements Turbofan support for
property accesses satisfying the following conditions:
1. The holder is a dictionary mode object.
2. The holder is a prototype.
3. The access is a load.

This feature will only be enabled if the build flag
v8_dict_property_const_tracking is set.

This particular CL implements support for the case that the property
in question is an accesor, meaning that the given PropertyAccessInfo
has kind kAccessorDictionaryProtoConstant.

Bug: v8:11248
Change-Id: I896e5dc59821f88abdb7a743e21ca3a700af9db2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2782280Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Frank Emrich <emrich@google.com>
Cr-Commit-Position: refs/heads/master@{#73617}

src/compiler/access-info.cc

@@ -135,7 +135,7 @@ PropertyAccessInfo PropertyAccessInfo::FastAccessorConstant(
     Zone* zone, Handle<Map> receiver_map, Handle<Object> constant,
     MaybeHandle<JSObject> holder) {
   return PropertyAccessInfo(zone, kFastAccessorConstant, holder, constant,
-                            {{receiver_map}, zone});
+                            MaybeHandle<Name>(), {{receiver_map}, zone});
 }
 
 // static
@@ -143,7 +143,7 @@ PropertyAccessInfo PropertyAccessInfo::ModuleExport(Zone* zone,
                                                     Handle<Map> receiver_map,
                                                     Handle<Cell> cell) {
   return PropertyAccessInfo(zone, kModuleExport, MaybeHandle<JSObject>(), cell,
-                            {{receiver_map}, zone});
+                            MaybeHandle<Name>{}, {{receiver_map}, zone});
 }
 
 // static
@@ -164,9 +164,9 @@ PropertyAccessInfo PropertyAccessInfo::DictionaryProtoDataConstant(
 // static
 PropertyAccessInfo PropertyAccessInfo::DictionaryProtoAccessorConstant(
     Zone* zone, Handle<Map> receiver_map, MaybeHandle<JSObject> holder,
-    Handle<Object> constant) {
+    Handle<Object> constant, Handle<Name> property_name) {
   return PropertyAccessInfo(zone, kDictionaryProtoAccessorConstant, holder,
-                            constant, {{receiver_map}, zone});
+                            constant, property_name, {{receiver_map}, zone});
 }
 
 // static
@@ -204,7 +204,8 @@ PropertyAccessInfo::PropertyAccessInfo(
 
 PropertyAccessInfo::PropertyAccessInfo(
     Zone* zone, Kind kind, MaybeHandle<JSObject> holder,
-    Handle<Object> constant, ZoneVector<Handle<Map>>&& lookup_start_object_maps)
+    Handle<Object> constant, MaybeHandle<Name> property_name,
+    ZoneVector<Handle<Map>>&& lookup_start_object_maps)
     : kind_(kind),
       lookup_start_object_maps_(lookup_start_object_maps),
       constant_(constant),
@@ -212,7 +213,11 @@ PropertyAccessInfo::PropertyAccessInfo(
       unrecorded_dependencies_(zone),
       field_representation_(Representation::None()),
       field_type_(Type::Any()),
-      dictionary_index_(InternalIndex::NotFound()) {}
+      dictionary_index_(InternalIndex::NotFound()),
+      name_(property_name) {
+  DCHECK_IMPLIES(kind == kDictionaryProtoAccessorConstant,
+                 !property_name.is_null());
+}
 PropertyAccessInfo::PropertyAccessInfo(
     Kind kind, MaybeHandle<JSObject> holder, MaybeHandle<Map> transition_map,
     FieldIndex field_index, Representation field_representation,
@@ -320,6 +325,7 @@ bool PropertyAccessInfo::Merge(PropertyAccessInfo const* that,
       return false;
     }
 
+    case kDictionaryProtoAccessorConstant:
     case kFastAccessorConstant: {
       // Check if we actually access the same constant.
       if (this->constant_.address() == that->constant_.address()) {
@@ -358,10 +364,6 @@ bool PropertyAccessInfo::Merge(PropertyAccessInfo const* that,
     }
     case kModuleExport:
       return false;
-
-    case kDictionaryProtoAccessorConstant:
-      // TODO(v8:11248) Dealt with in follow-up CLs.
-      UNREACHABLE();
   }
 }
 
@@ -515,54 +517,57 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
   UNREACHABLE();
 }
 
-PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
-    Handle<Map> receiver_map, Handle<Name> name, Handle<Map> map,
-    MaybeHandle<JSObject> holder, InternalIndex descriptor,
-    AccessMode access_mode) const {
-  DCHECK(descriptor.is_found());
-  Handle<DescriptorArray> descriptors(map->instance_descriptors(isolate()),
-                                      isolate());
-  SLOW_DCHECK(descriptor == descriptors->Search(*name, *map));
+namespace {
+using AccessorsObjectGetter = std::function<Handle<Object>()>;
+
+PropertyAccessInfo AccessorAccessInfoHelper(
+    Isolate* isolate, Zone* zone, JSHeapBroker* broker,
+    const AccessInfoFactory* ai_factory, Handle<Map> receiver_map,
+    Handle<Name> name, Handle<Map> map, MaybeHandle<JSObject> holder,
+    AccessMode access_mode, AccessorsObjectGetter get_accessors) {
   if (map->instance_type() == JS_MODULE_NAMESPACE_TYPE) {
     DCHECK(map->is_prototype_map());
     Handle<PrototypeInfo> proto_info(PrototypeInfo::cast(map->prototype_info()),
-                                     isolate());
+                                     isolate);
     Handle<JSModuleNamespace> module_namespace(
-        JSModuleNamespace::cast(proto_info->module_namespace()), isolate());
+        JSModuleNamespace::cast(proto_info->module_namespace()), isolate);
     Handle<Cell> cell(Cell::cast(module_namespace->module().exports().Lookup(
-                          isolate(), name, Smi::ToInt(name->GetHash()))),
-                      isolate());
-    if (cell->value().IsTheHole(isolate())) {
+                          isolate, name, Smi::ToInt(name->GetHash()))),
+                      isolate);
+    if (cell->value().IsTheHole(isolate)) {
       // This module has not been fully initialized yet.
-      return PropertyAccessInfo::Invalid(zone());
+      return PropertyAccessInfo::Invalid(zone);
     }
-    return PropertyAccessInfo::ModuleExport(zone(), receiver_map, cell);
+    return PropertyAccessInfo::ModuleExport(zone, receiver_map, cell);
   }
   if (access_mode == AccessMode::kHas) {
+    // kHas is not supported for dictionary mode objects.
+    DCHECK(!map->is_dictionary_map());
+
     // HasProperty checks don't call getter/setters, existence is sufficient.
-    return PropertyAccessInfo::FastAccessorConstant(zone(), receiver_map,
+    return PropertyAccessInfo::FastAccessorConstant(zone, receiver_map,
                                                     Handle<Object>(), holder);
   }
-  Handle<Object> accessors(descriptors->GetStrongValue(descriptor), isolate());
+  Handle<Object> accessors = get_accessors();
   if (!accessors->IsAccessorPair()) {
-    return PropertyAccessInfo::Invalid(zone());
+    return PropertyAccessInfo::Invalid(zone);
   }
   Handle<Object> accessor(access_mode == AccessMode::kLoad
                               ? Handle<AccessorPair>::cast(accessors)->getter()
                               : Handle<AccessorPair>::cast(accessors)->setter(),
-                          isolate());
+                          isolate);
   if (!accessor->IsJSFunction()) {
-    CallOptimization optimization(isolate(), accessor);
+    CallOptimization optimization(isolate, accessor);
     if (!optimization.is_simple_api_call() ||
         optimization.IsCrossContextLazyAccessorPair(
-            *broker()->target_native_context().object(), *map)) {
-      return PropertyAccessInfo::Invalid(zone());
+            *broker->target_native_context().object(), *map)) {
+      return PropertyAccessInfo::Invalid(zone);
     }
 
     CallOptimization::HolderLookup lookup;
     holder = optimization.LookupHolderOfExpectedType(receiver_map, &lookup);
     if (lookup == CallOptimization::kHolderNotFound) {
-      return PropertyAccessInfo::Invalid(zone());
+      return PropertyAccessInfo::Invalid(zone);
     }
     DCHECK_IMPLIES(lookup == CallOptimization::kHolderIsReceiver,
                    holder.is_null());
@@ -570,15 +575,39 @@ PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
   }
   if (access_mode == AccessMode::kLoad) {
     Handle<Name> cached_property_name;
-    if (FunctionTemplateInfo::TryGetCachedPropertyName(isolate(), accessor)
+    if (FunctionTemplateInfo::TryGetCachedPropertyName(isolate, accessor)
             .ToHandle(&cached_property_name)) {
-      PropertyAccessInfo access_info =
-          ComputePropertyAccessInfo(map, cached_property_name, access_mode);
+      PropertyAccessInfo access_info = ai_factory->ComputePropertyAccessInfo(
+          map, cached_property_name, access_mode);
       if (!access_info.IsInvalid()) return access_info;
     }
   }
-  return PropertyAccessInfo::FastAccessorConstant(zone(), receiver_map,
-                                                  accessor, holder);
+  if (map->is_dictionary_map()) {
+    return PropertyAccessInfo::DictionaryProtoAccessorConstant(
+        zone, receiver_map, holder, accessor, name);
+  } else {
+    return PropertyAccessInfo::FastAccessorConstant(zone, receiver_map,
+                                                    accessor, holder);
+  }
+}
+
+}  // namespace
+
+PropertyAccessInfo AccessInfoFactory::ComputeAccessorDescriptorAccessInfo(
+    Handle<Map> receiver_map, Handle<Name> name, Handle<Map> holder_map,
+    MaybeHandle<JSObject> holder, InternalIndex descriptor,
+    AccessMode access_mode) const {
+  DCHECK(descriptor.is_found());
+  Handle<DescriptorArray> descriptors(
+      holder_map->instance_descriptors(kRelaxedLoad), isolate());
+  SLOW_DCHECK(descriptor == descriptors->Search(*name, *holder_map));
+
+  auto get_accessors = [&]() {
+    return handle(descriptors->GetStrongValue(descriptor), isolate());
+  };
+  return AccessorAccessInfoHelper(isolate(), zone(), broker(), this,
+                                  receiver_map, name, holder_map, holder,
+                                  access_mode, get_accessors);
 }
 
 PropertyAccessInfo AccessInfoFactory::ComputeDictionaryProtoAccessInfo(
@@ -599,8 +628,13 @@ PropertyAccessInfo AccessInfoFactory::ComputeDictionaryProtoAccessInfo(
         zone(), receiver_map, holder, dictionary_index, name);
   }
 
-  // TODO(v8:11248) Support for accessors is implemented a in follow-up CL.
-  return PropertyAccessInfo::Invalid(zone());
+  auto get_accessors = [&]() {
+    return JSObject::DictionaryPropertyAt(holder, dictionary_index);
+  };
+  Handle<Map> holder_map = handle(holder->map(), isolate());
+  return AccessorAccessInfoHelper(isolate(), zone(), broker(), this,
+                                  receiver_map, name, holder_map, holder,
+                                  access_mode, get_accessors);
 }
 
 MinimorphicLoadPropertyAccessInfo AccessInfoFactory::ComputePropertyAccessInfo(

src/compiler/access-info.h

@@ -105,7 +105,7 @@ class PropertyAccessInfo final {
       InternalIndex dict_index, Handle<Name> name);
   static PropertyAccessInfo DictionaryProtoAccessorConstant(
       Zone* zone, Handle<Map> receiver_map, MaybeHandle<JSObject> holder,
-      Handle<Object> constant);
+      Handle<Object> constant, Handle<Name> name);
 
   bool Merge(PropertyAccessInfo const* that, AccessMode access_mode,
              Zone* zone) V8_WARN_UNUSED_RESULT;
@@ -183,7 +183,7 @@ class PropertyAccessInfo final {
   PropertyAccessInfo(Zone* zone, Kind kind, MaybeHandle<JSObject> holder,
                      ZoneVector<Handle<Map>>&& lookup_start_object_maps);
   PropertyAccessInfo(Zone* zone, Kind kind, MaybeHandle<JSObject> holder,
-                     Handle<Object> constant,
+                     Handle<Object> constant, MaybeHandle<Name> name,
                      ZoneVector<Handle<Map>>&& lookup_start_object_maps);
   PropertyAccessInfo(Kind kind, MaybeHandle<JSObject> holder,
                      MaybeHandle<Map> transition_map, FieldIndex field_index,

src/compiler/compilation-dependencies.cc

@@ -110,10 +110,11 @@ class ConstantInDictionaryPrototypeChainDependency final
  public:
   explicit ConstantInDictionaryPrototypeChainDependency(
       const MapRef receiver_map, const NameRef property_name,
-      const ObjectRef constant)
+      const ObjectRef constant, PropertyKind kind)
       : receiver_map_(receiver_map),
         property_name_{property_name},
-        constant_{constant} {
+        constant_{constant},
+        kind_{kind} {
     DCHECK(V8_DICT_PROPERTY_CONST_TRACKING_BOOL);
   }
 
@@ -166,7 +167,26 @@ class ConstantInDictionaryPrototypeChainDependency final
         return ValidationResult::kFoundIncorrect;
       }
 
-      Object value = dictionary.ValueAt(entry);
+      Object dictionary_value = dictionary.ValueAt(entry);
+      Object value;
+      // We must be able to detect the case that the property |property_name_|
+      // of |holder_| was originally a plain function |constant_| (when creating
+      // this dependency) and has since become an accessor whose getter is
+      // |constant_|. Therefore, we cannot just look at the property kind of
+      // |details|, because that reflects the current situation, not the one
+      // when creating this dependency.
+      if (details.kind() != kind_) {
+        return ValidationResult::kFoundIncorrect;
+      }
+      if (kind_ == PropertyKind::kAccessor) {
+        // Only supporting loading at the moment, so we only ever want the
+        // getter.
+        CHECK(dictionary_value.IsAccessorPair());
+        value = AccessorPair::cast(dictionary_value)
+                    .get(AccessorComponent::ACCESSOR_GETTER);
+      } else {
+        value = dictionary_value;
+      }
       return value == *constant_.object() ? ValidationResult::kFoundCorrect
                                           : ValidationResult::kFoundIncorrect;
     };
@@ -202,6 +222,7 @@ class ConstantInDictionaryPrototypeChainDependency final
   MapRef receiver_map_;
   NameRef property_name_;
   ObjectRef constant_;
+  PropertyKind kind_;
 };
 
 class TransitionDependency final : public CompilationDependency {
@@ -504,9 +525,9 @@ void CompilationDependencies::DependOnStableMap(const MapRef& map) {
 
 void CompilationDependencies::DependOnConstantInDictionaryPrototypeChain(
     const MapRef& receiver_map, const NameRef& property_name,
-    const ObjectRef& constant) {
+    const ObjectRef& constant, PropertyKind kind) {
   RecordDependency(zone_->New<ConstantInDictionaryPrototypeChainDependency>(
-      receiver_map, property_name, constant));
+      receiver_map, property_name, constant, kind));
 }
 
 AllocationType CompilationDependencies::DependOnPretenureMode(

src/compiler/compilation-dependencies.h

@@ -51,9 +51,13 @@ class V8_EXPORT_PRIVATE CompilationDependencies : public ZoneObject {
   // of the objects between receiver and |holder| on the prototype chain, b) any
   // of the objects on the prototype chain up to |holder| change prototypes, or
   // c) the value of |property_name| in |holder| changes.
+  // If PropertyKind is kData, |constant| is the value of the property in
+  // question. In case of PropertyKind::kAccessor, |constant| is the accessor
+  // function (i.e., getter or setter) itself, not the overall AccessorPair.
   void DependOnConstantInDictionaryPrototypeChain(const MapRef& receiver_map,
                                                   const NameRef& property_name,
-                                                  const ObjectRef& constant);
+                                                  const ObjectRef& constant,
+                                                  PropertyKind kind);
 
   // Return the pretenure mode of {site} and record the assumption that it does
   // not change.

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

@@ -2224,6 +2224,16 @@ Node* JSNativeContextSpecialization::InlinePropertyGetterCall(
     Node* frame_state, Node** effect, Node** control,
     ZoneVector<Node*>* if_exceptions, PropertyAccessInfo const& access_info) {
   ObjectRef constant(broker(), access_info.constant());
+
+  if (access_info.IsDictionaryProtoAccessorConstant()) {
+    // For fast mode holders we recorded dependencies in BuildPropertyLoad.
+    for (const Handle<Map> map : access_info.lookup_start_object_maps()) {
+      dependencies()->DependOnConstantInDictionaryPrototypeChain(
+          MapRef{broker(), map}, NameRef{broker(), access_info.name()},
+          constant, PropertyKind::kAccessor);
+    }
+  }
+
   Node* target = jsgraph()->Constant(constant);
   // Introduce the call to the getter function.
   Node* value;
@@ -2359,7 +2369,8 @@ JSNativeContextSpecialization::BuildPropertyLoad(
   Node* value;
   if (access_info.IsNotFound()) {
     value = jsgraph()->UndefinedConstant();
-  } else if (access_info.IsFastAccessorConstant()) {
+  } else if (access_info.IsFastAccessorConstant() ||
+             access_info.IsDictionaryProtoAccessorConstant()) {
     ConvertReceiverMode receiver_mode =
         receiver == lookup_start_object
             ? ConvertReceiverMode::kNotNullOrUndefined

src/compiler/property-access-builder.cc

@@ -160,7 +160,7 @@ Node* PropertyAccessBuilder::FoldLoadDictPrototypeConstant(
   for (const Handle<Map> map : access_info.lookup_start_object_maps()) {
     dependencies()->DependOnConstantInDictionaryPrototypeChain(
         MapRef{broker(), map}, NameRef{broker(), access_info.name()},
-        value.value());
+        value.value(), PropertyKind::kData);
   }
 
   return jsgraph()->Constant(value.value());

src/compiler/serializer-for-background-compilation.cc

@@ -3012,7 +3012,8 @@ SerializerForBackgroundCompilation::ProcessMapForNamedPropertyAccess(
 
   // For JSNativeContextSpecialization::InlinePropertySetterCall
   // and InlinePropertyGetterCall.
-  if (access_info.IsFastAccessorConstant() &&
+  if ((access_info.IsFastAccessorConstant() ||
+       access_info.IsDictionaryProtoAccessorConstant()) &&
       !access_info.constant().is_null()) {
     if (access_info.constant()->IsJSFunction()) {
       JSFunctionRef function(broker(), access_info.constant());

test/mjsunit/const-dict-tracking.js

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 //
 // Flags: --allow-natives-syntax --opt --no-always-opt
+// Flags: --no-stress-flush-bytecode
 //
 // Tests tracking of constness of properties stored in dictionary
 // mode prototypes.
@@ -372,6 +373,38 @@ function testbench(o, proto, update_proto, check_constness) {
   }
 })();
 
+// Test inlining of accessor.
+(function() {
+  var proto = Object.create(null);
+  proto.x_val = 1;
+  Object.defineProperty(proto, "x", {
+    get : function () {return this.x_val;}
+  });
+
+  var o = Object.create(proto);
+  assertFalse(%HasFastProperties(proto))
+
+  function read_x(arg_o) {
+    return arg_o.x;
+  }
+
+  %PrepareFunctionForOptimization(read_x);
+  assertEquals(1, read_x(o));
+  %OptimizeFunctionOnNextCall(read_x);
+  assertEquals(1, read_x(o));
+  assertOptimized(read_x);
+
+  // Test that we inlined the access:
+  var dummy = {x : 123};
+  read_x(dummy);
+
+  if (%IsDictPropertyConstTrackingEnabled()) {
+    assertTrue(%HasFastProperties(o));
+    assertFalse(%HasFastProperties(proto));
+    assertUnoptimized(read_x);
+  }
+})();
+
 // Invalidation by adding same property to receiver.
 (function() {
   var proto = Object.create(null);