Remove hand-written assembly ArrayPush stubs

src/hydrogen.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 2418 matching lines @@
 }
 
 
 HValue* HGraphBuilder::BuildNewElementsCapacity(HValue* old_capacity) {
   HValue* half_old_capacity = AddUncasted<HShr>(old_capacity,
                                                 graph_->GetConstant1());
 
   HValue* new_capacity = AddUncasted<HAdd>(half_old_capacity, old_capacity);
   new_capacity->ClearFlag(HValue::kCanOverflow);
 
-  HValue* min_growth = Add<HConstant>(16);
+  HValue* min_growth = Add<HConstant>(4);
 
   new_capacity = AddUncasted<HAdd>(new_capacity, min_growth);
   new_capacity->ClearFlag(HValue::kCanOverflow);
 
   return new_capacity;
 }
 
 
 void HGraphBuilder::BuildNewSpaceArrayCheck(HValue* length, ElementsKind kind) {
   int element_size = IsFastDoubleElementsKind(kind) ? kDoubleSize
@@ skipping 5203 matching lines @@
       }
       result = ast_context()->IsEffect() ? graph()->GetConstant0() : Top();
       Add<HSimulate>(expr->id(), REMOVABLE_SIMULATE);
       if (!ast_context()->IsEffect()) Drop(1);
 
       ast_context()->ReturnValue(result);
       return true;
     }
     case kArrayPush: {
       if (receiver_map.is_null()) return false;
-      if (receiver_map->instance_type() != JS_ARRAY_TYPE) return false;

[Toon Verwaest, 2014/04/11 13:06:57]

This is invalid, since .push is spec'ed as reading the length, writing the
element, updating the length. 
1) regular objects don't have a HObjectAccess::ForArrayLength(...) and
2) the length may be observable through accessors.
Additionally, I presume UncheckedMono... doesn't actually do anything to a
"length" field for non-JSArrays, so the fact that the length is updated by .push
is relevant.

Please add tests to this effect.

[danno, 2014/04/16 14:34:38]

Done.

       ElementsKind elements_kind = receiver_map->elements_kind();
       if (!IsFastElementsKind(elements_kind)) return false;
+      if (receiver_map->is_observed()) return false;
+      if (!receiver_map->is_extensible()) return false;

[Toon Verwaest, 2014/04/11 13:06:57]

I think is_extensible isn't necessary cause we guarantee we have dictionary
elements for those cases. We should definitely ASSERT though.

Both these conditions should also hold for ArrayPop. In fact, they are supposed
to hold for all BuildUncheckMonomorphicElementAccess users, so we should ASSERT
them in that method.

[danno, 2014/04/16 14:34:38]

I changed these into ASSERTs and added them to pop, too. Unfortuantely,
BuildUncheckMonomorphicElementAccess doesn't take a map so we can't use it as a
bottleneck.

 
       // If there may be elements accessors in the prototype chain, the fast
       // inlined version can't be used.
       if (receiver_map->DictionaryElementsInPrototypeChainOnly()) return false;
       // If there currently can be no elements accessors on the prototype chain,
       // it doesn't mean that there won't be any later. Install a full prototype
       // chain check to trap element accessors being installed on the prototype
       // chain, which would cause elements to go to dictionary mode and result
       // in a map change.
       Handle<JSObject> prototype(JSObject::cast(receiver_map->prototype()));
       BuildCheckPrototypeMaps(prototype, Handle<JSObject>());
 
-      HValue* op_vals[] = {
-        context(),
-        // Receiver.
-        environment()->ExpressionStackAt(expr->arguments()->length())
-      };
+      const int argc = expr->arguments()->length();
+      if (argc != 1) return false;
 
-      const int argc = expr->arguments()->length();
-      // Includes receiver.
-      PushArgumentsFromEnvironment(argc + 1);
+      HValue* value_to_push = Pop();
+      HValue* array = Pop();
 
-      CallInterfaceDescriptor* descriptor =
-          isolate()->call_descriptor(Isolate::CallHandler);
+      HValue* length = Add<HLoadNamedField>(array, static_cast<HValue*>(NULL),
+          HObjectAccess::ForArrayLength(elements_kind));
 
-      ArrayPushStub stub(receiver_map->elements_kind(), argc);
-      Handle<Code> code = stub.GetCode(isolate());
-      HConstant* code_value = Add<HConstant>(code);
+      {
+        NoObservableSideEffectsScope scope(this);
 
-      ASSERT((sizeof(op_vals) / kPointerSize) ==
-             descriptor->environment_length());
+        bool is_array = receiver_map->instance_type() == JS_ARRAY_TYPE;
+        BuildUncheckedMonomorphicElementAccess(array, length,
+                                               value_to_push, is_array,
+                                               elements_kind, STORE,
+                                               NEVER_RETURN_HOLE,
+                                               STORE_AND_GROW_NO_TRANSITION);
+      }
 
-      HInstruction* call = New<HCallWithDescriptor>(
-          code_value, argc + 1, descriptor,
-          Vector<HValue*>(op_vals, descriptor->environment_length()));
+      HInstruction* new_size = NewUncasted<HAdd>(length, Add<HConstant>(argc));
       Drop(1);  // Drop function.
-      ast_context()->ReturnInstruction(call, expr->id());
+      ast_context()->ReturnInstruction(new_size, expr->id());
       return true;
     }
     default:
       // Not yet supported for inlining.
       break;
   }
   return false;
 }
 
 
@@ skipping 3825 matching lines @@
   if (ShouldProduceTraceOutput()) {
     isolate()->GetHTracer()->TraceHydrogen(name(), graph_);
   }
 
 #ifdef DEBUG
   graph_->Verify(false);  // No full verify.
 #endif
 }
 
 } }  // namespace v8::internal