[turbofan] Serialize for ReduceKeyedLoadFromHeapConstant

Drive-by fix: In ProcessFeedbackForGlobalAccess, we had forgotten to
return the feedback when it already existed.

Bug: v8:7790, v8:9094
Change-Id: Ie4be6cef5755bbdd9d8ed472caaa2e32d243893d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1554680Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60705}

src/compiler/js-heap-broker.h

@@ -124,6 +124,11 @@ class V8_EXPORT_PRIVATE ObjectRef {
   bool BooleanValue() const;
   Maybe<double> OddballToNumber() const;
 
+  // Return the element at key {index} if {index} is known to be an own data
+  // property of the object that is non-writable and non-configurable.
+  base::Optional<ObjectRef> GetOwnConstantElement(uint32_t index,
+                                                  bool serialize = false) const;
+
   Isolate* isolate() const;
 
  protected:
@@ -132,8 +137,12 @@ class V8_EXPORT_PRIVATE ObjectRef {
   ObjectData* data_;  // Should be used only by object() getters.
 
  private:
+  friend class JSArrayData;
   friend class JSGlobalProxyRef;
   friend class JSGlobalProxyData;
+  friend class JSObjectData;
+  friend class StringData;
+
   JSHeapBroker* broker_;
 };
 
@@ -521,6 +530,11 @@ class JSArrayRef : public JSObjectRef {
   Handle<JSArray> object() const;
 
   ObjectRef length() const;
+
+  // Return the element at key {index} if the array has a copy-on-write elements
+  // storage and {index} is known to be an own data property.
+  base::Optional<ObjectRef> GetOwnCowElement(uint32_t index,
+                                             bool serialize = false) const;
 };
 
 class ScopeInfoRef : public HeapObjectRef {

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

@@ -1742,58 +1742,38 @@ Reduction JSNativeContextSpecialization::ReduceKeyedLoadFromHeapConstant(
     return NoChange();
   }
 
-  // Check whether we're accessing a known element on the {receiver}
-  // that is non-configurable, non-writable (e.g. the {receiver} was
-  // frozen using Object.freeze).
+  // Check whether we're accessing a known element on the {receiver} and can
+  // constant-fold the load.
   NumberMatcher mindex(index);
   if (mindex.IsInteger() && mindex.IsInRange(0.0, kMaxUInt32 - 1.0)) {
-    LookupIterator it(isolate(), receiver_ref.object(),
-                      static_cast<uint32_t>(mindex.Value()),
-                      LookupIterator::OWN);
-    if (it.state() == LookupIterator::DATA) {
-      if (it.IsReadOnly() && !it.IsConfigurable()) {
-        // We can safely constant-fold the {index} access to {receiver},
-        // since the element is non-configurable, non-writable and thus
-        // cannot change anymore.
-        Node* value = access_mode == AccessMode::kHas
-                          ? jsgraph()->TrueConstant()
-                          : jsgraph()->Constant(it.GetDataValue());
-        ReplaceWithValue(node, value, effect, control);
-        return Replace(value);
-      }
-
-      // Check if the {receiver} is a known constant with a copy-on-write
-      // backing store, and whether {index} is within the appropriate
-      // bounds. In that case we can constant-fold the access and only
-      // check that the {elements} didn't change. This is sufficient as
-      // the backing store of a copy-on-write JSArray is defensively
-      // copied whenever the length or the elements (might) change.
-      //
-      // What's interesting here is that we don't need to map check the
-      // {receiver}, since JSArray's will always have their elements in
-      // the backing store.
-      if (receiver_ref.IsJSArray()) {
-        Handle<JSArray> array = receiver_ref.AsJSArray().object();
-        if (array->elements()->IsCowArray()) {
-          Node* elements = effect = graph()->NewNode(
-              simplified()->LoadField(AccessBuilder::ForJSObjectElements()),
-              receiver, effect, control);
-          Handle<FixedArray> array_elements(FixedArray::cast(array->elements()),
-                                            isolate());
-          Node* check =
-              graph()->NewNode(simplified()->ReferenceEqual(), elements,
-                               jsgraph()->HeapConstant(array_elements));
-          effect = graph()->NewNode(
-              simplified()->CheckIf(DeoptimizeReason::kCowArrayElementsChanged),
-              check, effect, control);
-          Node* value = access_mode == AccessMode::kHas
-                            ? jsgraph()->TrueConstant()
-                            : jsgraph()->Constant(it.GetDataValue());
-          ReplaceWithValue(node, value, effect, control);
-          return Replace(value);
-        }
+    uint32_t index_value = static_cast<uint32_t>(mindex.Value());
+    base::Optional<ObjectRef> element =
+        receiver_ref.GetOwnConstantElement(index_value);
+    if (!element.has_value() && receiver_ref.IsJSArray()) {
+      // We didn't find a constant element, but if the receiver is a cow-array
+      // we can exploit the fact that any future write to the element will
+      // replace the whole elements storage.
+      element = receiver_ref.AsJSArray().GetOwnCowElement(index_value);
+      if (element.has_value()) {
+        Node* elements = effect = graph()->NewNode(
+            simplified()->LoadField(AccessBuilder::ForJSObjectElements()),
+            receiver, effect, control);
+        FixedArrayRef array_elements =
+            receiver_ref.AsJSArray().elements().AsFixedArray();
+        Node* check = graph()->NewNode(simplified()->ReferenceEqual(), elements,
+                                       jsgraph()->Constant(array_elements));
+        effect = graph()->NewNode(
+            simplified()->CheckIf(DeoptimizeReason::kCowArrayElementsChanged),
+            check, effect, control);
       }
     }
+    if (element.has_value()) {
+      Node* value = access_mode == AccessMode::kHas
+                        ? jsgraph()->TrueConstant()
+                        : jsgraph()->Constant(*element);
+      ReplaceWithValue(node, value, effect, control);
+      return Replace(value);
+    }
   }
 
   // For constant Strings we can eagerly strength-reduce the keyed
@@ -2272,7 +2252,6 @@ JSNativeContextSpecialization::BuildPropertyAccess(
       return BuildPropertyTest(effect, control, access_info);
   }
   UNREACHABLE();
-  return ValueEffectControl();
 }
 
 JSNativeContextSpecialization::ValueEffectControl

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

@@ -717,13 +717,15 @@ SerializerForBackgroundCompilation::ProcessFeedbackForGlobalAccess(
   if (slot.IsInvalid()) return nullptr;
   if (environment()->function().feedback_vector.is_null()) return nullptr;
   FeedbackSource source(environment()->function().feedback_vector, slot);
-  if (!broker()->HasFeedback(source)) {
-    const GlobalAccessFeedback* feedback =
-        broker()->ProcessFeedbackForGlobalAccess(source);
-    broker()->SetFeedback(source, feedback);
-    return feedback;
+
+  if (broker()->HasFeedback(source)) {
+    return broker()->GetGlobalAccessFeedback(source);
   }
-  return nullptr;
+
+  const GlobalAccessFeedback* feedback =
+      broker()->ProcessFeedbackForGlobalAccess(source);
+  broker()->SetFeedback(source, feedback);
+  return feedback;
 }
 
 void SerializerForBackgroundCompilation::VisitLdaGlobal(
@@ -818,6 +820,43 @@ void SerializerForBackgroundCompilation::ProcessFeedbackForKeyedPropertyAccess(
   }
 }
 
+void SerializerForBackgroundCompilation::ProcessKeyedPropertyAccess(
+    Hints const& receiver, Hints const& key, FeedbackSlot slot,
+    AccessMode mode) {
+  ProcessFeedbackForKeyedPropertyAccess(slot, mode);
+
+  for (Handle<Object> hint : receiver.constants()) {
+    ObjectRef receiver_ref(broker(), hint);
+
+    // For JSNativeContextSpecialization::ReduceElementAccess.
+    if (mode == AccessMode::kStore) {
+      if (receiver_ref.IsJSTypedArray()) {
+        receiver_ref.AsJSTypedArray().Serialize();
+      }
+    }
+
+    // For JSNativeContextSpecialization::ReduceKeyedLoadFromHeapConstant.
+    if (mode == AccessMode::kLoad || mode == AccessMode::kHas) {
+      for (Handle<Object> hint : key.constants()) {
+        ObjectRef key_ref(broker(), hint);
+        // TODO(neis): Do this for integer-HeapNumbers too?
+        if (key_ref.IsSmi() && key_ref.AsSmi() >= 0) {
+          base::Optional<ObjectRef> element =
+              receiver_ref.GetOwnConstantElement(key_ref.AsSmi(), true);
+          if (!element.has_value() && receiver_ref.IsJSArray()) {
+            // We didn't find a constant element, but if the receiver is a
+            // cow-array we can exploit the fact that any future write to the
+            // element will replace the whole elements storage.
+            receiver_ref.AsJSArray().GetOwnCowElement(key_ref.AsSmi(), true);
+          }
+        }
+      }
+    }
+  }
+
+  environment()->accumulator_hints().Clear();
+}
+
 void SerializerForBackgroundCompilation::ProcessMapForNamedPropertyAccess(
     MapRef const& map, NameRef const& name) {
   // For JSNativeContextSpecialization::ReduceNamedAccess.
@@ -850,9 +889,11 @@ void SerializerForBackgroundCompilation::ProcessFeedbackForNamedPropertyAccess(
 
 void SerializerForBackgroundCompilation::VisitLdaKeyedProperty(
     BytecodeArrayIterator* iterator) {
+  Hints const& key = environment()->accumulator_hints();
+  Hints const& receiver =
+      environment()->register_hints(iterator->GetRegisterOperand(0));
   FeedbackSlot slot = iterator->GetSlotOperand(1);
-  ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kLoad);
-  environment()->accumulator_hints().Clear();
+  ProcessKeyedPropertyAccess(receiver, key, slot, AccessMode::kLoad);
 }
 
 void SerializerForBackgroundCompilation::ProcessNamedPropertyAccess(
@@ -907,32 +948,31 @@ void SerializerForBackgroundCompilation::VisitStaNamedProperty(
 
 void SerializerForBackgroundCompilation::VisitTestIn(
     BytecodeArrayIterator* iterator) {
+  Hints const& receiver = environment()->accumulator_hints();
+  Hints const& key =
+      environment()->register_hints(iterator->GetRegisterOperand(0));
   FeedbackSlot slot = iterator->GetSlotOperand(1);
-  ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kHas);
-  environment()->accumulator_hints().Clear();
+  ProcessKeyedPropertyAccess(receiver, key, slot, AccessMode::kHas);
 }
 
 void SerializerForBackgroundCompilation::VisitStaKeyedProperty(
     BytecodeArrayIterator* iterator) {
-  const Hints& receiver =
+  Hints const& receiver =
       environment()->register_hints(iterator->GetRegisterOperand(0));
+  Hints const& key =
+      environment()->register_hints(iterator->GetRegisterOperand(1));
   FeedbackSlot slot = iterator->GetSlotOperand(2);
-
-  ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kStore);
-
-  for (Handle<Object> object : receiver.constants()) {
-    // For JSNativeContextSpecialization::ReduceElementAccess.
-    if (object->IsJSTypedArray()) JSTypedArrayRef(broker(), object).Serialize();
-  }
-
-  environment()->accumulator_hints().Clear();
+  ProcessKeyedPropertyAccess(receiver, key, slot, AccessMode::kStore);
 }
 
 void SerializerForBackgroundCompilation::VisitStaInArrayLiteral(
     BytecodeArrayIterator* iterator) {
+  Hints const& receiver =
+      environment()->register_hints(iterator->GetRegisterOperand(0));
+  Hints const& key =
+      environment()->register_hints(iterator->GetRegisterOperand(1));
   FeedbackSlot slot = iterator->GetSlotOperand(2);
-  ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kStoreInLiteral);
-  environment()->accumulator_hints().Clear();
+  ProcessKeyedPropertyAccess(receiver, key, slot, AccessMode::kStoreInLiteral);
 }
 
 #define DEFINE_CLEAR_ENVIRONMENT(name, ...)             \

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

@@ -251,6 +251,8 @@ class SerializerForBackgroundCompilation {
   void ProcessJump(interpreter::BytecodeArrayIterator* iterator);
   void MergeAfterJump(interpreter::BytecodeArrayIterator* iterator);
 
+  void ProcessKeyedPropertyAccess(Hints const& receiver, Hints const& key,
+                                  FeedbackSlot slot, AccessMode mode);
   void ProcessNamedPropertyAccess(interpreter::BytecodeArrayIterator* iterator,
                                   AccessMode mode);
   void ProcessNamedPropertyAccess(Hints const& receiver, NameRef const& name,

src/objects/js-objects.h

@@ -303,16 +303,15 @@ class JSObject : public JSReceiver {
   // corresponds to a set of object representations of elements that
   // have something in common.
   //
-  // In the fast mode elements is a FixedArray and so each element can
-  // be quickly accessed. This fact is used in the generated code. The
-  // elements array can have one of three maps in this mode:
-  // fixed_array_map, sloppy_arguments_elements_map or
-  // fixed_cow_array_map (for copy-on-write arrays). In the latter case
-  // the elements array may be shared by a few objects and so before
-  // writing to any element the array must be copied. Use
+  // In the fast mode elements is a FixedArray and so each element can be
+  // quickly accessed. The elements array can have one of several maps in this
+  // mode: fixed_array_map, fixed_double_array_map,
+  // sloppy_arguments_elements_map or fixed_cow_array_map (for copy-on-write
+  // arrays). In the latter case the elements array may be shared by a few
+  // objects and so before writing to any element the array must be copied. Use
   // EnsureWritableFastElements in this case.
   //
-  // In the slow mode the elements is either a NumberDictionary, a
+  // In the slow mode the elements is either a NumberDictionary or a
   // FixedArray parameter map for a (sloppy) arguments object.
   DECL_ACCESSORS(elements, FixedArrayBase)
   inline void initialize_elements();

test/mjsunit/compiler/keyed-load-on-string.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
+
+var s = "hello";
+
+function foo() {
+  return s[4];
+}
+assertTrue("o" === foo());
+assertTrue("o" === foo());
+%OptimizeFunctionOnNextCall(foo);
+assertTrue("o" === foo());
+
+function bar() {
+  return s[5];
+}
+assertSame(undefined, bar());
+assertSame(undefined, bar());
+%OptimizeFunctionOnNextCall(bar);
+assertSame(undefined, bar());

test/mjsunit/mjsunit.status

@@ -1035,4 +1035,10 @@
   '*': [SKIP],
 }], # variant == jitless and not embedded_builtins
 
+##############################################################################
+['variant == future', {
+  # crbug.com/v8/9094
+  'compiler/constant-fold-cow-array': [SKIP],
+}], # variant == future
+
 ]