Add hydrogen support for ArrayPop, and remove the handwritten call 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 7596 matching lines @@
       if (argument_count == 3 && check_type == RECEIVER_MAP_CHECK) {
         AddCheckConstantFunction(expr->holder(), receiver, receiver_map);
         HValue* right = Pop();
         HValue* left = Pop();
         Drop(1);  // Receiver.
         HInstruction* result = HMul::NewImul(zone(), context(), left, right);
         ast_context()->ReturnInstruction(result, expr->id());
         return true;
       }
       break;
+    case kArrayPop: {
+      if (!expr->IsMonomorphic() || expr->check_type() != RECEIVER_MAP_CHECK) {
+        return false;
+      }
+      if (receiver_map->instance_type() != JS_ARRAY_TYPE) return false;
+      ElementsKind elements_kind = receiver_map->elements_kind();
+      if (!IsFastElementsKind(elements_kind)) return false;
+      AddCheckConstantFunction(expr->holder(), receiver, receiver_map);
+
+      Drop(expr->arguments()->length());
+      HValue* result;
+      HValue* checked_object;
+      HValue* reduced_length;
+      HValue* receiver = Pop();
+      { NoObservableSideEffectsScope scope(this);
+        checked_object = AddCheckMap(receiver, receiver_map);
+        HValue* elements = AddLoadElements(checked_object);
+        // Ensure that we aren't popping from a copy-on-write array.
+        if (IsFastSmiOrObjectElementsKind(elements_kind)) {
+          Add<HCheckMaps>(
+              elements, isolate()->factory()->fixed_array_map(), top_info());
+        }
+        HValue* length = Add<HLoadNamedField>(
+            checked_object, HObjectAccess::ForArrayLength(elements_kind));
+        reduced_length = AddUncasted<HSub>(length, graph()->GetConstant1());
+        HValue* bounds_check = Add<HBoundsCheck>(
+            graph()->GetConstant0(), length);
+        result = AddElementAccess(elements, reduced_length, NULL,
+                                  bounds_check, elements_kind, false);

[mvstanton, 2014/01/20 12:45:39]

Can we make the boolean parameter to AddElementAccess be a LOAD/STORE enum? It's
a bit jarring because I expect to see some indication in the line that we are
doing a load here and a store below.

[Toon Verwaest, 2014/01/20 13:28:09]

Will do so.

On 2014/01/20 12:45:39, mvstanton wrote:

+        Factory* factory = isolate()->factory();
+        double nan_double = FixedDoubleArray::hole_nan_as_double();
+        HValue* hole = IsFastSmiOrObjectElementsKind(elements_kind)
+            ? Add<HConstant>(factory->the_hole_value())
+            : Add<HConstant>(nan_double);
+        if (IsFastSmiOrObjectElementsKind(elements_kind)) {
+          elements_kind = FAST_HOLEY_ELEMENTS;

[mvstanton, 2014/01/20 12:45:39]

This line confuses me a bit. A) Why wouldn't the elements_kind just change to be
the holey version of whatever it was? B) Why would double be excluded from this
operation? And if A) proves to be good enough, then you could eliminate the
change to elements kind in the case that it's already holey. 

If you have to do this for a technical reason so that that store works out
correctly, add a comment to explain.

[Toon Verwaest, 2014/01/20 13:28:09]

This is the standard way of storing holes into arrays. NaN-double is a double,
so it can directly be stored into a double array.

A hole (*the* hole), however, is a tagged value. If we'd try to store this into
a smi-array, there would be a tagged->smi conversion on the hole value; which
doesn't work. Hence we need to specifically allow this "tagged store" into a smi
array to let the hole go through.

As I said, this isn't necessary in case of double arrays, since double array
contains doubles and NaN in that case is just a special double.

On 2014/01/20 12:45:39, mvstanton wrote:

+        }
+        AddElementAccess(
+            elements, reduced_length, hole, bounds_check, elements_kind, true);
+      }
+      Add<HStoreNamedField>(
+          checked_object, HObjectAccess::ForArrayLength(elements_kind),
+          reduced_length);
+      Add<HSimulate>(expr->id(), REMOVABLE_SIMULATE);
+      ast_context()->ReturnValue(result);
+      return true;
+    }
     default:
       // Not yet supported for inlining.
       break;
   }
   return false;
 }
 
 
 bool HOptimizedGraphBuilder::TryCallApply(Call* expr) {
   Expression* callee = expr->expression();
@@ skipping 3379 matching lines @@
   if (ShouldProduceTraceOutput()) {
     isolate()->GetHTracer()->TraceHydrogen(name(), graph_);
   }
 
 #ifdef DEBUG
   graph_->Verify(false);  // No full verify.
 #endif
 }
 
 } }  // namespace v8::internal