[interpreter] Use fast paths again for object literals with spread cloning

[1] fixes the behavior of StaNamedOwnProperty to no longer do prototype
lookups. This lets us revert [2] and go back to using the fast path in
the clone spread object literal bytecode.

The test case from [2] is kept.

[1] https://chromium-review.googlesource.com/c/v8/v8/+/2795831
[2] https://chromium-review.googlesource.com/c/v8/v8/+/3178969

Bug: v8:9888, chromium:1251366
Change-Id: I9d2cb69b803c403f63365f55d27c4de20ff7dafb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3224666Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Patrick Thier <pthier@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77444}

src/interpreter/bytecode-generator.cc

@@ -3015,88 +3015,79 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
     BuildCreateObjectLiteral(literal, flags, entry);
   }
 
-  // If we used CloneObject for the first element is spread case, we already
-  // copied accessors. Therefore skip the static initialization and treat all
-  // properties after the spread as dynamic.
-  // TOOD(v8:9888): Use new Define ICs instead of Set ICs in the clone object
-  // spread case.
-  if (!clone_object_spread) {
-    // Store computed values into the literal.
-    AccessorTable<ObjectLiteral::Property> accessor_table(zone());
-    for (; property_index < expr->properties()->length(); property_index++) {
-      ObjectLiteral::Property* property =
-          expr->properties()->at(property_index);
-      if (property->is_computed_name()) break;
-      if (property->IsCompileTimeValue()) continue;
+  // Store computed values into the literal.
+  AccessorTable<ObjectLiteral::Property> accessor_table(zone());
+  for (; property_index < expr->properties()->length(); property_index++) {
+    ObjectLiteral::Property* property = expr->properties()->at(property_index);
+    if (property->is_computed_name()) break;
+    if (!clone_object_spread && property->IsCompileTimeValue()) continue;
 
-      RegisterAllocationScope inner_register_scope(this);
-      Literal* key = property->key()->AsLiteral();
-      switch (property->kind()) {
-        case ObjectLiteral::Property::SPREAD:
-        case ObjectLiteral::Property::CONSTANT:
-          UNREACHABLE();
-        case ObjectLiteral::Property::MATERIALIZED_LITERAL:
-          DCHECK(!property->value()->IsCompileTimeValue());
-          V8_FALLTHROUGH;
-        case ObjectLiteral::Property::COMPUTED: {
-          // It is safe to use [[Put]] here because the boilerplate already
-          // contains computed properties with an uninitialized value.
+    RegisterAllocationScope inner_register_scope(this);
+    Literal* key = property->key()->AsLiteral();
+    switch (property->kind()) {
+      case ObjectLiteral::Property::SPREAD:
+        UNREACHABLE();
+      case ObjectLiteral::Property::CONSTANT:
+      case ObjectLiteral::Property::MATERIALIZED_LITERAL:
+        DCHECK(clone_object_spread || !property->value()->IsCompileTimeValue());
+        V8_FALLTHROUGH;
+      case ObjectLiteral::Property::COMPUTED: {
+        // It is safe to use [[Put]] here because the boilerplate already
+        // contains computed properties with an uninitialized value.
+        Register key_reg;
+        if (key->IsStringLiteral()) {
+          DCHECK(key->IsPropertyName());
+        } else {
+          key_reg = register_allocator()->NewRegister();
+          builder()->SetExpressionPosition(property->key());
+          VisitForRegisterValue(property->key(), key_reg);
+        }
+
+        object_literal_context_scope.SetEnteredIf(
+            property->value()->IsConciseMethodDefinition());
+        builder()->SetExpressionPosition(property->value());
+
+        if (property->emit_store()) {
+          VisitForAccumulatorValue(property->value());
           if (key->IsStringLiteral()) {
-            DCHECK(key->IsPropertyName());
-            object_literal_context_scope.SetEnteredIf(
-                property->value()->IsConciseMethodDefinition());
-            if (property->emit_store()) {
-              builder()->SetExpressionPosition(property->value());
-              VisitForAccumulatorValue(property->value());
-              FeedbackSlot slot = feedback_spec()->AddStoreOwnICSlot();
-              builder()->StoreNamedOwnProperty(
-                  literal, key->AsRawPropertyName(), feedback_index(slot));
-            } else {
-              builder()->SetExpressionPosition(property->value());
-              VisitForEffect(property->value());
-            }
+            FeedbackSlot slot = feedback_spec()->AddStoreOwnICSlot();
+            builder()->StoreNamedOwnProperty(literal, key->AsRawPropertyName(),
+                                             feedback_index(slot));
           } else {
-            RegisterList args = register_allocator()->NewRegisterList(3);
-
-            builder()->MoveRegister(literal, args[0]);
-            builder()->SetExpressionPosition(property->key());
-            VisitForRegisterValue(property->key(), args[1]);
-
-            object_literal_context_scope.SetEnteredIf(
-                property->value()->IsConciseMethodDefinition());
-            builder()->SetExpressionPosition(property->value());
-            VisitForRegisterValue(property->value(), args[2]);
-            if (property->emit_store()) {
-              builder()->CallRuntime(Runtime::kSetKeyedProperty, args);
-            }
+            FeedbackSlot slot = feedback_spec()->AddKeyedDefineOwnICSlot();
+            builder()->DefineKeyedProperty(literal, key_reg,
+                                           feedback_index(slot));
           }
-          break;
-        }
-        case ObjectLiteral::Property::PROTOTYPE: {
-          // __proto__:null is handled by CreateObjectLiteral.
-          if (property->IsNullPrototype()) break;
-          DCHECK(property->emit_store());
-          DCHECK(!property->NeedsSetFunctionName());
-          RegisterList args = register_allocator()->NewRegisterList(2);
-          builder()->MoveRegister(literal, args[0]);
-          object_literal_context_scope.SetEnteredIf(false);
-          builder()->SetExpressionPosition(property->value());
-          VisitForRegisterValue(property->value(), args[1]);
-          builder()->CallRuntime(Runtime::kInternalSetPrototype, args);
-          break;
+        } else {
+          VisitForEffect(property->value());
         }
-        case ObjectLiteral::Property::GETTER:
-          if (property->emit_store()) {
-            accessor_table.LookupOrInsert(key)->getter = property;
-          }
-          break;
-        case ObjectLiteral::Property::SETTER:
-          if (property->emit_store()) {
-            accessor_table.LookupOrInsert(key)->setter = property;
-          }
-          break;
+        break;
+      }
+      case ObjectLiteral::Property::PROTOTYPE: {
+        // __proto__:null is handled by CreateObjectLiteral.
+        if (property->IsNullPrototype()) break;
+        DCHECK(property->emit_store());
+        DCHECK(!property->NeedsSetFunctionName());
+        RegisterList args = register_allocator()->NewRegisterList(2);
+        builder()->MoveRegister(literal, args[0]);
+        object_literal_context_scope.SetEnteredIf(false);
+        builder()->SetExpressionPosition(property->value());
+        VisitForRegisterValue(property->value(), args[1]);
+        builder()->CallRuntime(Runtime::kInternalSetPrototype, args);
+        break;
       }
+      case ObjectLiteral::Property::GETTER:
+        if (property->emit_store()) {
+          accessor_table.LookupOrInsert(key)->getter = property;
+        }
+        break;
+      case ObjectLiteral::Property::SETTER:
+        if (property->emit_store()) {
+          accessor_table.LookupOrInsert(key)->setter = property;
+        }
+        break;
     }
+  }
 
     // Define accessors, using only a single call to the runtime for each pair
     // of corresponding getters and setters.
@@ -3113,7 +3104,6 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
           .StoreAccumulatorInRegister(args[4])
           .CallRuntime(Runtime::kDefineAccessorPropertyUnchecked, args);
     }
-  }
 
   // Object literals have two parts. The "static" part on the left contains no
   // computed property names, and so we can compute its map ahead of time; see

test/cctest/interpreter/bytecode_expectations/ObjectLiterals.golden

@@ -231,20 +231,19 @@ handlers: [
 snippet: "
   var a = 1; return { 1: a };
 "
-frame size: 5
+frame size: 3
 parameter count: 1
-bytecode array length: 25
+bytecode array length: 20
 bytecodes: [
   /*   42 S> */ B(LdaSmi), I8(1),
                 B(Star0),
   /*   45 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(Star1),
                 B(LdaSmi), I8(1),
-                B(Star3),
-                B(Mov), R(1), R(2),
-                B(Mov), R(0), R(4),
-  /*   57 E> */ B(CallRuntime), U16(Runtime::kSetKeyedProperty), R(2), U8(3),
-                B(Ldar), R(2),
+                B(Star2),
+                B(Ldar), R(0),
+  /*   57 E> */ B(StaKeyedPropertyAsDefine), R(1), R(2), U8(1),
+                B(Ldar), R(1),
   /*   61 S> */ B(Return),
 ]
 constant pool: [

test/mjsunit/regress/regress-crbug-1251366.js

@@ -6,8 +6,15 @@ Object.defineProperty(Object.prototype, "x", {
     set: function (val) {}
 });
 
+Object.defineProperty(Object.prototype, "0", {
+    set: function (val) {}
+});
+
 let o = {...{}, x: 3};
-assertEquals(o.x, 3);
+assertEquals(3, o.x);
 
 let o2 = {...{x: 1}, x: 3};
-assertEquals(o2.x, 3);
+assertEquals(3, o2.x);
+
+let o3 = {...{}, 0: 42}
+assertEquals(42, o3[0]);