[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

src/builtins/builtins-array.cc

Index: src/builtins/builtins-array.cc
diff --git a/src/builtins/builtins-array.cc b/src/builtins/builtins-array.cc
index 19cab23bbfbc7c0a8750c1bda28e46774039465c..f3ffa516c64c03a472ca669d095de6658ed1d626 100644
--- a/src/builtins/builtins-array.cc
+++ b/src/builtins/builtins-array.cc
@@ -150,8 +150,9 @@ MUST_USE_RESULT static Object* CallJsIntrinsic(Isolate* isolate,
       isolate,
       Execution::Call(isolate, function, args.receiver(), argc, argv.start()));
 }
+}  // namespace
 
-Object* DoArrayPush(Isolate* isolate, BuiltinArguments args) {
+BUILTIN(ArrayPush) {
   HandleScope scope(isolate);
   Handle<Object> receiver = args.receiver();
   if (!EnsureJSArrayWithWritableFastElements(isolate, receiver, &args, 1)) {
@@ -174,19 +175,168 @@ Object* DoArrayPush(Isolate* isolate, BuiltinArguments args) {
   int new_length = accessor->Push(array, &args, to_add);
   return Smi::FromInt(new_length);
 }
-}  // namespace
 
-BUILTIN(ArrayPush) { return DoArrayPush(isolate, args); }
-
-// TODO(verwaest): This is a temporary helper until the FastArrayPush stub can
-// tailcall to the builtin directly.
-RUNTIME_FUNCTION(Runtime_ArrayPush) {
-  DCHECK_EQ(2, args.length());
-  Arguments* incoming = reinterpret_cast<Arguments*>(args[0]);
-  // Rewrap the arguments as builtins arguments.
-  int argc = incoming->length() + BuiltinArguments::kNumExtraArgsWithReceiver;
-  BuiltinArguments caller_args(argc, incoming->arguments() + 1);
-  return DoArrayPush(isolate, caller_args);
+void Builtins::Generate_FastArrayPush(compiler::CodeAssemblerState* state) {
+  typedef compiler::Node Node;
+  typedef CodeStubAssembler::Label Label;
+  typedef CodeStubAssembler::Variable Variable;
+  CodeStubAssembler assembler(state);
+  Label runtime(&assembler, Label::kDeferred);
+
+  Node* argc = assembler.Parameter(1);
+  Node* context = assembler.Parameter(2);
+  Node* new_target = assembler.Parameter(0);
+
+  CodeStubArguments args(&assembler, argc);
+  Node* receiver = args.GetReceiver();
+
+  Label fast(&assembler);
+  assembler.BranchIfFastJSArray(
+      receiver, context, CodeStubAssembler::FastJSArrayAccessMode::ANY_ACCESS,
+      &fast, &runtime);
+
+  assembler.Bind(&fast);
+  {

[Jakob Kummerow, 2016/11/23 17:17:05]

nit: this identation level doesn't add anything (except whitespace). I'd just
drop it; if you want to keep it, then at least you should pull the
"Bind(&runtime)" section out of it.

[danno, 2016/11/29 14:39:59]

Done.

+    // Disallow pushing onto prototypes. It might be the JSArray prototype.
+    // Disallow pushing onto non-extensible objects.
+    assembler.Comment("Disallow pushing onto prototypes");
+    Node* map = assembler.LoadMap(receiver);
+    Node* bit_field2 = assembler.LoadObjectField(map, Map::kBitField2Offset,

[Jakob Kummerow, 2016/11/23 17:17:05]

... = assembler.LoadMapBitField2(map);

[danno, 2016/11/29 14:39:59]

Done.

+                                                 MachineType::Uint8());
+    int mask = static_cast<int>(Map::IsPrototypeMapBits::kMask) |
+               (1 << Map::kIsExtensible);
+    Node* test = assembler.Word32And(bit_field2, assembler.Int32Constant(mask));
+    assembler.GotoIf(
+        assembler.Word32NotEqual(
+            test, assembler.Int32Constant(1 << Map::kIsExtensible)),
+        &runtime);
+
+    // Disallow pushing onto arrays in dictionary named property mode. We need
+    // to figure out whether the length property is still writable.
+    assembler.Comment(
+        "Disallow pushing onto arrays in dictionary named property mode");
+    Node* bit_field3 = assembler.LoadObjectField(map, Map::kBitField3Offset,

[Jakob Kummerow, 2016/11/23 17:17:05]

... = assembler.LoadMapBitField3(map);

[danno, 2016/11/29 14:39:59]

Done.

+                                                 MachineType::Uint32());
+    mask = static_cast<int>(Map::DictionaryMap::kMask);
+    Node* mask_node = assembler.Int32Constant(mask);
+    test = assembler.Word32And(bit_field3, mask_node);
+    assembler.GotoIf(assembler.Word32Equal(test, mask_node), &runtime);

[Jakob Kummerow, 2016/11/23 17:17:05]

assembler.GotoIf(assembler.IsSetWord32<Map::DictionaryMap>(bit_field3),
&runtime);

+
+    // Check whether the length property is writable. The length property is the

[Jakob Kummerow, 2016/11/23 17:17:05]

Most excellent point. The KeyedStoreGeneric stub has never done this. I've filed
https://bugs.chromium.org/p/v8/issues/detail?id=5669.

[danno, 2016/11/29 14:39:59]

Done.

+    // only default named property on arrays. It's nonconfigurable, hence is
+    // guaranteed to stay the first property.
+    Node* descriptors = assembler.LoadObjectField(map, Map::kDescriptorsOffset);

[Jakob Kummerow, 2016/11/23 17:17:05]

... = assembler.LoadMapDescriptors(map);

[danno, 2016/11/29 14:39:59]

Done.

+    Node* details = assembler.LoadFixedArrayElement(
+        descriptors,
+        assembler.Int32Constant(DescriptorArray::ToDetailsIndex(0)));
+    mask = READ_ONLY << PropertyDetails::AttributesField::kShift;
+    mask_node = assembler.SmiConstant(Smi::FromInt(mask));

[Jakob Kummerow, 2016/11/23 17:17:05]

nit: s/Smi::FromInt(mask)/mask/

[danno, 2016/11/29 14:39:59]

Done.

+    test = assembler.WordAnd(details, mask_node);
+    assembler.GotoIf(assembler.WordEqual(test, mask_node), &runtime);
+
+    Variable arg_index(&assembler, MachineType::PointerRepresentation());
+    arg_index.Bind(assembler.IntPtrConstant(0));
+    Node* kind = assembler.DecodeWord32<Map::ElementsKindBits>(bit_field2);
+    CodeStubAssembler::VariableList vars({&arg_index}, assembler.zone());
+    Label default_label(&assembler, vars);

[Jakob Kummerow, 2016/11/23 17:17:05]

nit: why not just s/vars/&arg_index/ here and once more in line 244?

[danno, 2016/11/29 14:39:59]

Done.

+    Label smi_transition(&assembler);
+    Label object_push_pre(&assembler);
+    Label object_push(&assembler, vars);
+    Label double_push(&assembler);
+    Label double_transition(&assembler);
+
+    assembler.GotoIf(
+        assembler.IntPtrGreaterThan(
+            kind, assembler.IntPtrConstant(FAST_HOLEY_SMI_ELEMENTS)),
+        &object_push_pre);
+
+    {
+      Node* new_length =
+          assembler.BuildAppendJSArray(FAST_SMI_ELEMENTS, context, receiver,
+                                       args, arg_index, &smi_transition);
+      args.PopAndReturn(new_length);
+    }
+
+    // If the argument is not a smi, then use a heavyweight SetProperty to
+    // transition the array for only the single next element. If the argument is
+    // a smi, the failure is due to some other reason and we should fall back on
+    // the most generic implementation for the rest of the array.
+    assembler.Bind(&smi_transition);
+    {
+      Node* arg = args.AtIndex(arg_index.value());
+      assembler.GotoIf(assembler.TaggedIsSmi(arg), &default_label);
+      Node* length = assembler.LoadJSArrayLength(receiver);
+      // TODO(danno): Use the KeyedStoreGeneric stub here when possible,
+      // calling into the runtime to do the elements transition is overkill.
+      assembler.CallRuntime(Runtime::kSetProperty, context, receiver, length,
+                            arg, assembler.SmiConstant(Smi::FromInt(STRICT)));

[Jakob Kummerow, 2016/11/23 17:17:05]

nit: s/Smi::FromInt(STRICT)/STRICT/

[danno, 2016/11/29 14:39:59]

Done.

+      assembler.Increment(arg_index);
+      assembler.GotoIfNotNumber(arg, &object_push);
+      assembler.Goto(&double_push);
+    }
+
+    assembler.Bind(&object_push_pre);
+    assembler.Branch(assembler.IntPtrGreaterThan(
+                         kind, assembler.IntPtrConstant(FAST_HOLEY_ELEMENTS)),
+                     &double_push, &object_push);
+
+    assembler.Bind(&object_push);
+    {
+      Node* new_length = assembler.BuildAppendJSArray(
+          FAST_ELEMENTS, context, receiver, args, arg_index, &default_label);
+      args.PopAndReturn(new_length);
+    }
+
+    assembler.Bind(&double_push);
+    {
+      Node* new_length =
+          assembler.BuildAppendJSArray(FAST_DOUBLE_ELEMENTS, context, receiver,
+                                       args, arg_index, &double_transition);
+      args.PopAndReturn(new_length);
+    }
+
+    // If the argument is not a double, then use a heavyweight SetProperty to
+    // transition the array for only the single next element. If the argument is
+    // a double, the failure is due to some other reason and we should fall back
+    // on the most generic implementation for the rest of the array.
+    assembler.Bind(&double_transition);
+    {
+      Node* arg = args.AtIndex(arg_index.value());
+      assembler.GotoIfNumber(arg, &default_label);
+      Node* length = assembler.LoadJSArrayLength(receiver);
+      // TODO(danno): Use the KeyedStoreGeneric stub here when possible,
+      // calling into the runtime to do the elements transition is overkill.
+      assembler.CallRuntime(Runtime::kSetProperty, context, receiver, length,
+                            arg, assembler.SmiConstant(Smi::FromInt(STRICT)));

[Jakob Kummerow, 2016/11/23 17:17:05]

nit: s/Smi::FromInt(STRICT)/STRICT/

[danno, 2016/11/29 14:39:59]

Done.

+      assembler.Increment(arg_index);
+      assembler.Goto(&object_push);
+    }
+
+    // Fallback that stores un-processed arguments using the full, heavyweight
+    // SetProperty machinery.
+    assembler.Bind(&default_label);
+    {
+      args.ForEach(
+          [receiver, context, &arg_index](CodeStubAssembler* assembler,
+                                          Node* arg) {
+            Node* length = assembler->LoadJSArrayLength(receiver);
+            assembler->CallRuntime(
+                Runtime::kSetProperty, context, receiver, length, arg,
+                assembler->SmiConstant(Smi::FromInt(STRICT)));

[Jakob Kummerow, 2016/11/23 17:17:05]

nit: s/Smi::FromInt(STRICT)/STRICT/

[danno, 2016/11/29 14:39:59]

Done.

+          },
+          arg_index.value());
+      args.PopAndReturn(assembler.LoadJSArrayLength(receiver));
+    }
+
+    assembler.Bind(&runtime);
+    {
+      Node* target =
+          assembler.LoadFromFrame(StandardFrameConstants::kFunctionOffset,
+                                  MachineType::TaggedPointer());
+      assembler.TailCallStub(CodeFactory::ArrayPush(assembler.isolate()),
+                             context, target, new_target, argc);
+    }
+  }
 }
 
 BUILTIN(ArrayPop) {
@@ -1293,7 +1443,9 @@ void Builtins::Generate_ArrayIncludes(compiler::CodeAssemblerState* state) {
 
   // Take slow path if not a JSArray, if retrieving elements requires
   // traversing prototype, or if access checks are required.
-  assembler.BranchIfFastJSArray(array, context, &init_len, &call_runtime);
+  assembler.BranchIfFastJSArray(
+      array, context, CodeStubAssembler::FastJSArrayAccessMode::INBOUNDS_READ,
+      &init_len, &call_runtime);
 
   assembler.Bind(&init_len);
   {
@@ -1734,7 +1886,9 @@ void Builtins::Generate_ArrayIndexOf(compiler::CodeAssemblerState* state) {
 
   // Take slow path if not a JSArray, if retrieving elements requires
   // traversing prototype, or if access checks are required.
-  assembler.BranchIfFastJSArray(array, context, &init_len, &call_runtime);
+  assembler.BranchIfFastJSArray(
+      array, context, CodeStubAssembler::FastJSArrayAccessMode::INBOUNDS_READ,
+      &init_len, &call_runtime);
 
   assembler.Bind(&init_len);
   {