Merged: [compiler] Fix typing JSLoadNamed of private brands

Private method loads are compiled to a named load of a private brand,
which always loads a BlockContext. This BlockContext holds the private
methods common to all instances of a class. TurboFan currently considers
JSLoadNamed to be of Type::NonInternal(). Private methods break this
assumption, since BlockContext is of Type::OtherInternal().

This CL changes the typing of JSLoadNamed of private brands to be
Type::OtherInternal().

Bug: v8:12500, chromium:1287475

(cherry picked from commit d19a707d148ca9b9ab9dd8c294fe4c49e99f31d4)

Change-Id: I91f39747bf9422bd419d299f44152f567d8be8db
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3399225
Reviewed-by: Maya Lekova <[email protected]>
Commit-Queue: Shu-yu Guo <[email protected]>
Cr-Commit-Position: refs/branch-heads/9.7@{#38}
Cr-Branched-From: 49162da-refs/heads/9.7.106@{#1}
Cr-Branched-From: a7e9b8f-refs/heads/main@{#77674}

src/compiler/access-info.cc

@@ -431,8 +431,9 @@ bool AccessInfoFactory::ComputeElementAccessInfos(
 }
 
 PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
-    MapRef receiver_map, MapRef map, base::Optional<JSObjectRef> holder,
-    InternalIndex descriptor, AccessMode access_mode) const {
+    MapRef receiver_map, MapRef map, NameRef name,
+    base::Optional<JSObjectRef> holder, InternalIndex descriptor,
+    AccessMode access_mode) const {
   DCHECK(descriptor.is_found());
   // TODO(jgruber,v8:7790): Use DescriptorArrayRef instead.
   Handle<DescriptorArray> descriptors = map.instance_descriptors().object();
@@ -449,7 +450,10 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
   }
   FieldIndex field_index = FieldIndex::ForPropertyIndex(*map.object(), index,
                                                         details_representation);
-  Type field_type = Type::NonInternal();
+  // Private brands are used when loading private methods, which are stored in a
+  // BlockContext, an internal object.
+  Type field_type = name.object()->IsPrivateBrand() ? Type::OtherInternal()
+                                                    : Type::NonInternal();
   base::Optional<MapRef> field_map;
 
   ZoneVector<CompilationDependency const*> unrecorded_dependencies(zone());
@@ -842,8 +846,8 @@ PropertyAccessInfo AccessInfoFactory::ComputePropertyAccessInfo(
       }
       if (details.location() == PropertyLocation::kField) {
         if (details.kind() == kData) {
-          return ComputeDataFieldAccessInfo(receiver_map, map, holder, index,
-                                            access_mode);
+          return ComputeDataFieldAccessInfo(receiver_map, map, name, holder,
+                                            index, access_mode);
         } else {
           DCHECK_EQ(kAccessor, details.kind());
           // TODO(turbofan): Add support for general accessors?

src/compiler/access-info.h

@@ -289,8 +289,9 @@ class AccessInfoFactory final {
                                       base::Optional<JSObjectRef> holder,
                                       PropertyAttributes attrs) const;
   PropertyAccessInfo ComputeDataFieldAccessInfo(
-      MapRef receiver_map, MapRef map, base::Optional<JSObjectRef> holder,
-      InternalIndex descriptor, AccessMode access_mode) const;
+      MapRef receiver_map, MapRef map, NameRef name,
+      base::Optional<JSObjectRef> holder, InternalIndex descriptor,
+      AccessMode access_mode) const;
   PropertyAccessInfo ComputeAccessorDescriptorAccessInfo(
       MapRef receiver_map, NameRef name, MapRef map,
       base::Optional<JSObjectRef> holder, InternalIndex descriptor,

src/compiler/typer.cc

@@ -1325,7 +1325,18 @@ Type Typer::Visitor::TypeJSLoadProperty(Node* node) {
   return Type::NonInternal();
 }
 
-Type Typer::Visitor::TypeJSLoadNamed(Node* node) { return Type::NonInternal(); }
+Type Typer::Visitor::TypeJSLoadNamed(Node* node) {
+#ifdef DEBUG
+  // Loading of private methods is compiled to a named load of a BlockContext
+  // via a private brand, which is an internal object. However, native context
+  // specialization should always apply for those cases, so assert that the name
+  // is not a private brand here. Otherwise Type::NonInternal() is wrong.
+  JSLoadNamedNode n(node);
+  NamedAccess const& p = n.Parameters();
+  DCHECK(!p.name(typer_->broker()).object()->IsPrivateBrand());
+#endif
+  return Type::NonInternal();
+}
 
 Type Typer::Visitor::TypeJSLoadNamedFromSuper(Node* node) {
   return Type::NonInternal();

test/mjsunit/compiler/inline-private-method.js

@@ -0,0 +1,22 @@
+// Copyright 2021 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
+
+class A {
+  a() { this.#b() }
+  #b() {}
+}
+
+function InlinePrivateMethod() {
+  for (let i = 0; i < 10; i++) {
+    new A().a();
+  }
+}
+
+%PrepareFunctionForOptimization(A);
+%PrepareFunctionForOptimization(InlinePrivateMethod);
+InlinePrivateMethod();
+%OptimizeFunctionOnNextCall(InlinePrivateMethod);
+InlinePrivateMethod();