Hydrogen array constructor cleanup and improvements

src/code-stubs-hydrogen.cc

Index: src/code-stubs-hydrogen.cc
diff --git a/src/code-stubs-hydrogen.cc b/src/code-stubs-hydrogen.cc
index 11cd307451cc0017429d0ea84be0bbdc410982a5..08f5d6b68a0bcb18400cafaccb9017986543b826 100644
--- a/src/code-stubs-hydrogen.cc
+++ b/src/code-stubs-hydrogen.cc
@@ -106,8 +106,13 @@ class CodeStubGraphBuilderBase : public HGraphBuilder {
   };
 
   HValue* BuildArrayConstructor(ElementsKind kind,
+                                bool ensure_context,

[danno, 2013/06/27 08:53:04]

Turn this boolean  parameter into an enum, it makes it much easier to understand
at the caller site what's going on.

[mvstanton, 2013/06/27 15:33:29]

Done.

                                 bool disable_allocation_sites,

[danno, 2013/06/27 08:53:04]

Turn this boolean  parameter into an enum, it makes it much easier to understand
at the caller site what's going on. In this case, you may want to make it part
of AllocationSite, e.g. AllocationSite::MementoState with values
AllocationSite::MEMENTOS_ENABLED and AllocationSite::MEMENTOS_DISABLED

[mvstanton, 2013/06/27 15:33:29]

Done.

                                 ArgumentClass argument_class);
+  HValue* BuildArrayConstructorImpl(ElementsKind kind,
+                                    HValue* constructor,
+                                    bool disable_allocation_sites,

[danno, 2013/06/27 08:53:04]

Same as above

[mvstanton, 2013/06/27 15:33:29]

Done.

+                                    ArgumentClass argument_class);
   HValue* BuildInternalArrayConstructor(ElementsKind kind,
                                         ArgumentClass argument_class);
 
@@ -534,14 +539,30 @@ Handle<Code> TransitionElementsKindStub::GenerateCode() {
 }
 
 HValue* CodeStubGraphBuilderBase::BuildArrayConstructor(
-    ElementsKind kind, bool disable_allocation_sites,
+    ElementsKind kind,
+    bool ensure_context,
+    bool disable_allocation_sites,
     ArgumentClass argument_class) {
   HValue* constructor = GetParameter(ArrayConstructorStubBase::kConstructor);
-  HValue* property_cell = GetParameter(ArrayConstructorStubBase::kPropertyCell);
-  HInstruction* array_function = BuildGetArrayFunction(context());
+  if (ensure_context) {
+    HInstruction* array_function = BuildGetArrayFunction(context());
+    ArrayContextChecker checker(this, constructor, array_function);
+    return BuildArrayConstructorImpl(kind, constructor,

[danno, 2013/06/27 08:53:04]

the return clause is common code, how about factoring it out and putting it
after the if

[mvstanton, 2013/06/27 15:33:29]

Thanks, I initially tried that and ran into some IfBuilder problems, but now it
works...I could eliminate BuildArrayConstructorImpl too.

+                                     disable_allocation_sites, argument_class);
+  } else {
+    return BuildArrayConstructorImpl(kind, constructor,
+                                     disable_allocation_sites, argument_class);
+  }
+}
 
-  ArrayContextChecker(this, constructor, array_function);
-  JSArrayBuilder array_builder(this, kind, property_cell,
+
+HValue* CodeStubGraphBuilderBase::BuildArrayConstructorImpl(
+    ElementsKind kind,
+    HValue* constructor,
+    bool disable_allocation_sites,
+    ArgumentClass argument_class) {
+  HValue* property_cell = GetParameter(ArrayConstructorStubBase::kPropertyCell);
+  JSArrayBuilder array_builder(this, kind, property_cell, constructor,
                                disable_allocation_sites);
   HValue* result = NULL;
   switch (argument_class) {
@@ -555,6 +576,7 @@ HValue* CodeStubGraphBuilderBase::BuildArrayConstructor(
       result = BuildArrayNArgumentsConstructor(&array_builder, kind);
       break;
   }
+

[danno, 2013/06/27 08:53:04]

Not sure this additional line makes a big difference :-)

[mvstanton, 2013/06/27 15:33:29]

Done.

   return result;
 }
 
@@ -652,8 +674,10 @@ HValue* CodeStubGraphBuilderBase::BuildArrayNArgumentsConstructor(
 template <>
 HValue* CodeStubGraphBuilder<ArrayNoArgumentConstructorStub>::BuildCodeStub() {
   ElementsKind kind = casted_stub()->elements_kind();
+  bool ensure_context = casted_stub()->ensure_context();
   bool disable_allocation_sites = casted_stub()->disable_allocation_sites();
-  return BuildArrayConstructor(kind, disable_allocation_sites, NONE);
+  return BuildArrayConstructor(kind, ensure_context, disable_allocation_sites,
+                               NONE);
 }
 
 
@@ -666,8 +690,10 @@ template <>
 HValue* CodeStubGraphBuilder<ArraySingleArgumentConstructorStub>::
     BuildCodeStub() {
   ElementsKind kind = casted_stub()->elements_kind();
+  bool ensure_context = casted_stub()->ensure_context();
   bool disable_allocation_sites = casted_stub()->disable_allocation_sites();
-  return BuildArrayConstructor(kind, disable_allocation_sites, SINGLE);
+  return BuildArrayConstructor(kind, ensure_context, disable_allocation_sites,
+                               SINGLE);
 }
 
 
@@ -679,8 +705,10 @@ Handle<Code> ArraySingleArgumentConstructorStub::GenerateCode() {
 template <>
 HValue* CodeStubGraphBuilder<ArrayNArgumentsConstructorStub>::BuildCodeStub() {
   ElementsKind kind = casted_stub()->elements_kind();
+  bool ensure_context = casted_stub()->ensure_context();
   bool disable_allocation_sites = casted_stub()->disable_allocation_sites();
-  return BuildArrayConstructor(kind, disable_allocation_sites, MULTIPLE);
+  return BuildArrayConstructor(kind, ensure_context, disable_allocation_sites,
+                               MULTIPLE);
 }