[turbofan] Fix and simplify optimization of access on global proxy

We used to have two special cases for named accesses on the global
proxy, one based on seeing the global proxy constant in the graph and
on based on seeing the global proxy map either in the feedback or in
the graph. A change I made a while ago accidentally disabled the second
one. This CL restores that.

Moreover, given how things are set up now (this might have been
different before), the first optimization is subsumed by the second
one, so this CL also removes the first one.

Finally, this CL records an accumulator hint in the case of a load,
which improves precision of the serializer for concurrent inlining.

Tbr: tebbi@chromium.org
Bug: v8:7790
Change-Id: I255afc6c79e5c5c900b3ccfcd8459d836d21e42b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1801954
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63806}

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

@@ -790,6 +790,9 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
                           : NoChange();
 }
 
+// TODO(neis): Try to merge this with ReduceNamedAccess by introducing a new
+// PropertyAccessInfo kind for global accesses and using the existing mechanism
+// for building loads/stores.
 Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
     Node* node, Node* receiver, Node* value, NameRef const& name,
     AccessMode access_mode, Node* key, PropertyCellRef const& property_cell) {
@@ -838,15 +841,16 @@ Reduction JSNativeContextSpecialization::ReduceGlobalAccess(
     effect = BuildCheckEqualsName(name, key, effect, control);
   }
 
-  // Check if we have a {receiver} to validate. If so, we need to check that
-  // the {receiver} is actually the JSGlobalProxy for the native context that
-  // we are specializing to.
+  // If we have a {receiver} to validate, we do so by checking that its map is
+  // the (target) global proxy's map. This guarantees that in fact the receiver
+  // is the global proxy.
   if (receiver != nullptr) {
-    Node* check = graph()->NewNode(simplified()->ReferenceEqual(), receiver,
-                                   jsgraph()->HeapConstant(global_proxy()));
     effect = graph()->NewNode(
-        simplified()->CheckIf(DeoptimizeReason::kReceiverNotAGlobalProxy),
-        check, effect, control);
+        simplified()->CheckMaps(
+            CheckMapsFlag::kNone,
+            ZoneHandleSet<Map>(
+                HeapObjectRef(broker(), global_proxy()).map().object())),
+        receiver, effect, control);
   }
 
   if (access_mode == AccessMode::kLoad || access_mode == AccessMode::kHas) {
@@ -1050,28 +1054,6 @@ Reduction JSNativeContextSpecialization::ReduceJSStoreGlobal(Node* node) {
   }
 }
 
-void JSNativeContextSpecialization::FilterMapsAndGetPropertyAccessInfos(
-    NamedAccessFeedback const& feedback, AccessMode access_mode, Node* receiver,
-    Node* effect, ZoneVector<PropertyAccessInfo>* access_infos) {
-  ZoneVector<Handle<Map>> receiver_maps(zone());
-
-  // Either infer maps from the graph or use the feedback.
-  if (!InferReceiverMaps(receiver, effect, &receiver_maps)) {
-    receiver_maps = feedback.maps();
-  }
-  RemoveImpossibleReceiverMaps(receiver, &receiver_maps);
-
-  for (Handle<Map> map_handle : receiver_maps) {
-    MapRef map(broker(), map_handle);
-    if (map.is_deprecated()) continue;
-    PropertyAccessInfo access_info = broker()->GetPropertyAccessInfo(
-        map, feedback.name(), access_mode, dependencies(),
-        FLAG_concurrent_inlining ? SerializationPolicy::kAssumeSerialized
-                                 : SerializationPolicy::kSerializeIfNeeded);
-    access_infos->push_back(access_info);
-  }
-}
-
 Reduction JSNativeContextSpecialization::ReduceNamedAccess(
     Node* node, Node* value, NamedAccessFeedback const& feedback,
     AccessMode access_mode, Node* key) {
@@ -1088,28 +1070,45 @@ Reduction JSNativeContextSpecialization::ReduceNamedAccess(
   Node* effect = NodeProperties::GetEffectInput(node);
   Node* control = NodeProperties::GetControlInput(node);
 
-  ZoneVector<PropertyAccessInfo> access_infos_for_feedback(zone());
-  ZoneVector<PropertyAccessInfo> access_infos(zone());
-  FilterMapsAndGetPropertyAccessInfos(feedback, access_mode, receiver, effect,
-                                      &access_infos_for_feedback);
-  AccessInfoFactory access_info_factory(broker(), dependencies(),
-                                        graph()->zone());
-  if (!access_info_factory.FinalizePropertyAccessInfos(
-          access_infos_for_feedback, access_mode, &access_infos)) {
-    return NoChange();
+  // Either infer maps from the graph or use the feedback.
+  ZoneVector<Handle<Map>> receiver_maps(zone());
+  if (!InferReceiverMaps(receiver, effect, &receiver_maps)) {
+    receiver_maps = feedback.maps();
   }
+  RemoveImpossibleReceiverMaps(receiver, &receiver_maps);
 
-  // Check if we have an access o.x or o.x=v where o is the current
-  // native contexts' global proxy, and turn that into a direct access
-  // to the current native context's global object instead.
-  if (access_infos.size() == 1 && access_infos[0].receiver_maps().size() == 1) {
-    MapRef receiver_map(broker(), access_infos[0].receiver_maps()[0]);
+  // Check if we have an access o.x or o.x=v where o is the target native
+  // contexts' global proxy, and turn that into a direct access to the
+  // corresponding global object instead.
+  if (receiver_maps.size() == 1) {
+    MapRef receiver_map(broker(), receiver_maps[0]);
     if (receiver_map.IsMapOfTargetGlobalProxy()) {
       return ReduceGlobalAccess(node, receiver, value, feedback.name(),
                                 access_mode, key);
     }
   }
 
+  ZoneVector<PropertyAccessInfo> access_infos(zone());
+  {
+    ZoneVector<PropertyAccessInfo> access_infos_for_feedback(zone());
+    for (Handle<Map> map_handle : receiver_maps) {
+      MapRef map(broker(), map_handle);
+      if (map.is_deprecated()) continue;
+      PropertyAccessInfo access_info = broker()->GetPropertyAccessInfo(
+          map, feedback.name(), access_mode, dependencies(),
+          FLAG_concurrent_inlining ? SerializationPolicy::kAssumeSerialized
+                                   : SerializationPolicy::kSerializeIfNeeded);
+      access_infos_for_feedback.push_back(access_info);
+    }
+
+    AccessInfoFactory access_info_factory(broker(), dependencies(),
+                                          graph()->zone());
+    if (!access_info_factory.FinalizePropertyAccessInfos(
+            access_infos_for_feedback, access_mode, &access_infos)) {
+      return NoChange();
+    }
+  }
+
   // Ensure that {key} matches the specified name (if {key} is given).
   if (key != nullptr) {
     effect = BuildCheckEqualsName(feedback.name(), key, effect, control);
@@ -1331,24 +1330,6 @@ Reduction JSNativeContextSpecialization::ReduceNamedAccess(
   return Replace(value);
 }
 
-Reduction JSNativeContextSpecialization::ReduceNamedAccessFromNexus(
-    Node* node, Node* value, FeedbackSource const& source, NameRef const& name,
-    AccessMode access_mode) {
-  DCHECK(node->opcode() == IrOpcode::kJSLoadNamed ||
-         node->opcode() == IrOpcode::kJSStoreNamed ||
-         node->opcode() == IrOpcode::kJSStoreNamedOwn);
-  Node* const receiver = NodeProperties::GetValueInput(node, 0);
-
-  // Optimize accesses to the current native context's global proxy.
-  HeapObjectMatcher m(receiver);
-  if (m.HasValue() &&
-      m.Ref(broker()).equals(native_context().global_proxy_object())) {
-    return ReduceGlobalAccess(node, nullptr, value, name, access_mode);
-  }
-
-  return ReducePropertyAccess(node, nullptr, name, value, source, access_mode);
-}
-
 Reduction JSNativeContextSpecialization::ReduceJSLoadNamed(Node* node) {
   DCHECK_EQ(IrOpcode::kJSLoadNamed, node->opcode());
   NamedAccess const& p = NamedAccessOf(node->op());
@@ -1387,9 +1368,8 @@ Reduction JSNativeContextSpecialization::ReduceJSLoadNamed(Node* node) {
   }
 
   if (!p.feedback().IsValid()) return NoChange();
-  return ReduceNamedAccessFromNexus(node, jsgraph()->Dead(),
-                                    FeedbackSource(p.feedback()), name,
-                                    AccessMode::kLoad);
+  return ReducePropertyAccess(node, nullptr, name, jsgraph()->Dead(),
+                              FeedbackSource(p.feedback()), AccessMode::kLoad);
 }
 
 Reduction JSNativeContextSpecialization::ReduceJSGetIterator(Node* node) {
@@ -1524,9 +1504,8 @@ Reduction JSNativeContextSpecialization::ReduceJSStoreNamed(Node* node) {
   Node* const value = NodeProperties::GetValueInput(node, 1);
 
   if (!p.feedback().IsValid()) return NoChange();
-  return ReduceNamedAccessFromNexus(node, value, FeedbackSource(p.feedback()),
-                                    NameRef(broker(), p.name()),
-                                    AccessMode::kStore);
+  return ReducePropertyAccess(node, nullptr, NameRef(broker(), p.name()), value,
+                              FeedbackSource(p.feedback()), AccessMode::kStore);
 }
 
 Reduction JSNativeContextSpecialization::ReduceJSStoreNamedOwn(Node* node) {
@@ -1535,9 +1514,9 @@ Reduction JSNativeContextSpecialization::ReduceJSStoreNamedOwn(Node* node) {
   Node* const value = NodeProperties::GetValueInput(node, 1);
 
   if (!p.feedback().IsValid()) return NoChange();
-  return ReduceNamedAccessFromNexus(node, value, FeedbackSource(p.feedback()),
-                                    NameRef(broker(), p.name()),
-                                    AccessMode::kStoreInLiteral);
+  return ReducePropertyAccess(node, nullptr, NameRef(broker(), p.name()), value,
+                              FeedbackSource(p.feedback()),
+                              AccessMode::kStoreInLiteral);
 }
 
 Reduction JSNativeContextSpecialization::ReduceElementAccessOnString(

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

@@ -101,10 +101,6 @@ class V8_EXPORT_PRIVATE JSNativeContextSpecialization final
                                  base::Optional<NameRef> static_name,
                                  Node* value, FeedbackSource const& source,
                                  AccessMode access_mode);
-  Reduction ReduceNamedAccessFromNexus(Node* node, Node* value,
-                                       FeedbackSource const& source,
-                                       NameRef const& name,
-                                       AccessMode access_mode);
   Reduction ReduceNamedAccess(Node* node, Node* value,
                               NamedAccessFeedback const& processed,
                               AccessMode access_mode, Node* key = nullptr);
@@ -225,11 +221,6 @@ class V8_EXPORT_PRIVATE JSNativeContextSpecialization final
       ElementAccessFeedback const& feedback, Node* receiver,
       Node* effect) const;
 
-  void FilterMapsAndGetPropertyAccessInfos(
-      NamedAccessFeedback const& feedback, AccessMode access_mode,
-      Node* receiver, Node* effect,
-      ZoneVector<PropertyAccessInfo>* access_infos);
-
   // Try to infer maps for the given {receiver} at the current {effect}.
   bool InferReceiverMaps(Node* receiver, Node* effect,
                          ZoneVector<Handle<Map>>* receiver_maps) const;

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

@@ -2499,8 +2499,13 @@ SerializerForBackgroundCompilation::ProcessMapForNamedPropertyAccess(
 
   // For JSNativeContextSpecialization::ReduceNamedAccess.
   if (receiver_map.IsMapOfTargetGlobalProxy()) {
-    broker()->target_native_context().global_proxy_object().GetPropertyCell(
+    JSGlobalProxyRef global_proxy =
+        broker()->target_native_context().global_proxy_object();
+    base::Optional<PropertyCellRef> cell = global_proxy.GetPropertyCell(
         name, SerializationPolicy::kSerializeIfNeeded);
+    if (access_mode == AccessMode::kLoad && cell.has_value()) {
+      new_accumulator_hints->AddConstant(cell->value().object());
+    }
   }
 
   PropertyAccessInfo access_info = broker()->GetPropertyAccessInfo(
@@ -2657,8 +2662,6 @@ void SerializerForBackgroundCompilation::ProcessNamedAccess(
                                      base::nullopt, new_accumulator_hints);
   }
 
-  JSGlobalProxyRef global_proxy =
-      broker()->target_native_context().global_proxy_object();
   for (Handle<Object> hint : receiver.constants()) {
     ObjectRef object(broker(), hint);
     if (access_mode == AccessMode::kLoad && object.IsJSObject()) {
@@ -2667,13 +2670,6 @@ void SerializerForBackgroundCompilation::ProcessNamedAccess(
                                        object.AsJSObject(),
                                        new_accumulator_hints);
     }
-    // For JSNativeContextSpecialization::ReduceNamedAccessFromNexus.
-    if (object.equals(global_proxy)) {
-      // TODO(neis): Record accumulator hint? Also for string.length and maybe
-      // more.
-      global_proxy.GetPropertyCell(feedback.name(),
-                                   SerializationPolicy::kSerializeIfNeeded);
-    }
     // For JSNativeContextSpecialization::ReduceJSLoadNamed.
     if (access_mode == AccessMode::kLoad && object.IsJSFunction() &&
         feedback.name().equals(ObjectRef(
@@ -2684,6 +2680,8 @@ void SerializerForBackgroundCompilation::ProcessNamedAccess(
         new_accumulator_hints->AddConstant(function.prototype().object());
       }
     }
+    // TODO(neis): Also record accumulator hint for string.length and maybe
+    // more?
   }
 }
 

src/deoptimizer/deoptimize-reason.h

@@ -48,7 +48,6 @@ namespace internal {
   V(NotASymbol, "not a Symbol")                                                \
   V(OutOfBounds, "out of bounds")                                              \
   V(Overflow, "overflow")                                                      \
-  V(ReceiverNotAGlobalProxy, "receiver was not a global proxy")                \
   V(Smi, "Smi")                                                                \
   V(Unknown, "(unknown)")                                                      \
   V(ValueMismatch, "value mismatch")                                           \

test/mjsunit/compiler/concurrent-inlining-1.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
+
+// This test ensures that we manage to serialize the global.gaga function for
+// compilation and therefore are able to inline it. Since the call feedback in
+// bar is megamorphic, this relies on recording the correct accumulator hint for
+// the named load of obj.gaga while serializing bar (in turn while serializing
+// foo).
+
+const global = this;
+global.gaga = function gaga() { return true; };
+
+function bar(obj) { return obj.gaga(); };
+function foo() { return %TurbofanStaticAssert(bar(global)); }
+
+%PrepareFunctionForOptimization(foo);
+%PrepareFunctionForOptimization(bar);
+%PrepareFunctionForOptimization(global.gaga);
+
+bar({gaga() {}});
+foo();
+%OptimizeFunctionOnNextCall(foo);
+foo();

test/mjsunit/compiler/concurrent-inlining-2.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
+
+// This test ensures that we manage to serialize the global.gaga function for
+// compilation and therefore are able to inline it. Since the call feedback in
+// bar is megamorphic, this relies on recording the correct accumulator hint for
+// the named load of obj.gaga while serializing bar (in turn while serializing
+// foo).
+
+const global = this;
+global.gaga = function gaga() { return true; };
+
+function bar(obj) { return obj.gaga(); }
+function foo(obj) { obj.gaga; %TurbofanStaticAssert(bar(obj)); }
+
+%PrepareFunctionForOptimization(foo);
+%PrepareFunctionForOptimization(bar);
+%PrepareFunctionForOptimization(global.gaga);
+
+bar({gaga() {}});
+foo(global);
+%OptimizeFunctionOnNextCall(foo);
+foo(global);

test/mjsunit/mjsunit.status

@@ -1123,6 +1123,8 @@
 
   # Static asserts for optimizations don't hold due to removed optimization
   # phases.
+  'compiler/concurrent-inlining-1': [SKIP],
+  'compiler/concurrent-inlining-2': [SKIP],
   'compiler/diamond-followedby-branch': [SKIP],
   'compiler/load-elimination-const-field': [SKIP],
 }],  # variant == turboprop