Build fast literals in hydrogen.

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 435805fbfbd62eb6e91e4dcb8e1bb67bdaa68ee2..bf839c5fe950f564dd964816acd137f80c8acdd5 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -6040,33 +6040,33 @@ void HOptimizedGraphBuilder::VisitObjectLiteral(ObjectLiteral* expr) {
 
   // Check whether to use fast or slow deep-copying for boilerplate.
   int total_size = 0;
-  int max_properties = HFastLiteral::kMaxLiteralProperties;
+  int max_properties = kMaxLiteralProperties;
   Handle<Object> boilerplate(closure->literals()->get(expr->literal_index()),
                              isolate());
   if (boilerplate->IsJSObject() &&
       IsFastLiteral(Handle<JSObject>::cast(boilerplate),
-                    HFastLiteral::kMaxLiteralDepth,
+                    kMaxLiteralDepth,
                     &max_properties,
                     &total_size)) {
     Handle<JSObject> boilerplate_object = Handle<JSObject>::cast(boilerplate);
-    literal = new(zone()) HFastLiteral(context,
-                                       boilerplate_object,
-                                       total_size,
-                                       expr->literal_index(),
-                                       expr->depth(),
-                                       DONT_TRACK_ALLOCATION_SITE);
+    literal = BuildFastLiteral(context,
+                               boilerplate_object,
+                               total_size,
+                               DONT_TRACK_ALLOCATION_SITE,
+                               environment()->previous_ast_id());
   } else {
-    literal = new(zone()) HObjectLiteral(context,
-                                         expr->constant_properties(),
-                                         expr->fast_elements(),
-                                         expr->literal_index(),
-                                         expr->depth(),
-                                         expr->has_function());
+    literal = AddInstruction(
+                  new(zone()) HObjectLiteral(context,
+                                             expr->constant_properties(),
+                                             expr->fast_elements(),
+                                             expr->literal_index(),
+                                             expr->depth(),
+                                             expr->has_function()));
   }
 
   // The object is expected in the bailout environment during computation
   // of the property values and is the value of the entire expression.
-  PushAndAdd(literal);
+  Push(literal);
 
   expr->CalculateEmitStore(zone());
 
@@ -6175,32 +6175,32 @@ void HOptimizedGraphBuilder::VisitArrayLiteral(ArrayLiteral* expr) {
 
   // Check whether to use fast or slow deep-copying for boilerplate.
   int total_size = 0;
-  int max_properties = HFastLiteral::kMaxLiteralProperties;
+  int max_properties = kMaxLiteralProperties;
   if (IsFastLiteral(boilerplate,
-                    HFastLiteral::kMaxLiteralDepth,
+                    kMaxLiteralDepth,
                     &max_properties,
                     &total_size)) {
     if (mode == TRACK_ALLOCATION_SITE) {
       total_size += AllocationSiteInfo::kSize;
     }
-    literal = new(zone()) HFastLiteral(context,
-                                       boilerplate,
-                                       total_size,
-                                       expr->literal_index(),
-                                       expr->depth(),
-                                       mode);
+    literal = BuildFastLiteral(context,
+                               boilerplate,
+                               total_size,
+                               mode,
+                               environment()->previous_ast_id());
   } else {
-    literal = new(zone()) HArrayLiteral(context,
-                                        boilerplate,
-                                        length,
-                                        expr->literal_index(),
-                                        expr->depth(),
-                                        mode);
+    literal = AddInstruction(
+                  new(zone()) HArrayLiteral(context,
+                                            boilerplate,
+                                            length,
+                                            expr->literal_index(),
+                                            expr->depth(),
+                                            mode));
   }
 
   // The array is expected in the bailout environment during computation
   // of the property values and is the value of the entire expression.
-  PushAndAdd(literal);
+  Push(literal);
 
   HLoadElements* elements = NULL;
 
@@ -9897,6 +9897,201 @@ HInstruction* HOptimizedGraphBuilder::BuildThisFunction() {
 }
 
 
+HInstruction* HOptimizedGraphBuilder::BuildFastLiteral(
+    HValue* context,
+    Handle<JSObject> boilerplate_object,
+    int size,
+    AllocationSiteMode mode,
+    BailoutId id) {
+  Zone* zone = this->zone();
+  Handle<Map> map(boilerplate_object->map());
+  ElementsKind kind = map->elements_kind();
+  bool packed = IsFastPackedElementsKind(kind);
+
+  HInstruction* boilerplate = AddInstruction(new(zone) HConstant(
+      boilerplate_object, Representation::Tagged()));

[danno, 2013/04/02 21:08:47]

Maybe I missed this, but where is the boilerplate copied? I don't mean in the
code, I mean that the copy must be made to make sure that changes in the
original boilerplate don't cause differences in later behavior, including during
lithium code generation.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+  if (CanTransitionToMoreGeneralFastElementsKind(kind, packed)) {

[danno, 2013/04/02 21:08:47]

Can you get rid of this? If you make a copy of the boilerplate before-hand, no
map check is ever needed since the boilerplate never changes.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+    AddInstruction(new(zone) HCheckMaps(boilerplate, map, zone));
+  }
+
+  HValue* size_in_bytes =
+      AddInstruction(new(zone) HConstant(size, Representation::Integer32()));
+  HInstruction* object =
+      AddInstruction(new(zone) HAllocate(context,
+                                         size_in_bytes,
+                                         HType::JSObject(),
+                                         HAllocate::CAN_ALLOCATE_IN_NEW_SPACE));
+  int offset = 0;
+  BuildEmitDeepCopy(boilerplate_object, object, boilerplate, &offset, mode, id);
+  ASSERT_EQ(size, offset);
+  return object;
+}
+
+
+void HOptimizedGraphBuilder::BuildEmitDeepCopy(Handle<JSObject> object,
+                                               HInstruction* result,
+                                               HInstruction* source,

[danno, 2013/04/02 21:08:47]

I am not sure why you need to have both object and source here. From what I can
see, source is currently always the HConstant of object. Later you will want to
pass a different allocation-site object for each sub-object, but for now, do you
need to pass source around, or can you cons it inside this method when you need
it? 

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+                                               int* offset,
+                                               AllocationSiteMode mode,
+                                               BailoutId id) {
+  Zone* zone = this->zone();
+  Factory* factory = isolate()->factory();
+
+  bool create_allocation_site_info = mode == TRACK_ALLOCATION_SITE &&
+      object->map()->CanTrackAllocationSite();
+
+  // Only elements backing stores for non-COW arrays need to be copied.
+  Handle<FixedArrayBase> elements(object->elements());
+  ElementsKind kind = object->map()->elements_kind();
+  bool has_elements = elements->length() > 0 &&
+      elements->map() != isolate()->heap()->fixed_cow_array_map();
+
+  // Increase the offset so that subsequent objects end up right after
+  // this object and its backing store.
+  int object_offset = *offset;
+  int object_size = object->map()->instance_size();
+  int elements_size = has_elements ? elements->Size() : 0;
+  int elements_offset = *offset + object_size;
+  if (create_allocation_site_info) {
+    elements_offset += AllocationSiteInfo::kSize;
+    *offset += AllocationSiteInfo::kSize;
+  }
+
+  *offset += object_size + elements_size;
+
+  // Copy object header.
+  ASSERT(object->properties()->length() == 0);
+  int inobject_properties = object->map()->inobject_properties();
+  int header_size = object_size - inobject_properties * kPointerSize;
+  HValue* object_header =
+        AddInstruction(new(zone) HInnerAllocatedObject(result, object_offset));
+  for (int i = 0; i < header_size; i += kPointerSize) {

[danno, 2013/04/02 21:08:47]

Be careful here. Specifically, when you copy the object's map, you need to call
BuildStoreMap to make sure that GVN flags are set appropriately. 

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+    HInstruction* header_value;
+    if (has_elements && i == JSObject::kElementsOffset) {
+      header_value = AddInstruction(new(zone) HInnerAllocatedObject(
+          result, elements_offset));
+
+    } else {
+      header_value = AddInstruction(new(zone) HLoadNamedField(source, true, i));

[danno, 2013/04/02 21:08:47]

Are you sure that it's OK not to deep copy the non-inlined properties from the
header?

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Now I am creating HConstants out of the boilerplate copy.

+    }
+    AddInstruction(new(zone) HStoreNamedField(object_header,

[danno, 2013/04/02 21:08:47]

Have you considered loading the value you want to store as a constant here and
below? That way there is no need to both load and store from memory. You can
leave it like this for now, but it might be a good follow-up CL.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+                                              factory->empty_string(),
+                                              header_value,
+                                              true, i));

[danno, 2013/04/02 21:08:47]

Needs to set the kChangesInobjectFields GVN flag

[Hannes Payer (out of office), 2013/04/08 11:03:00]

is done above.

+    AddSimulate(id);
+  }
+
+  // Copy in-object properties.
+  HValue* object_properties =
+      AddInstruction(new(zone) HInnerAllocatedObject(result, object_offset));
+  for (int i = 0; i < inobject_properties; i++) {

[danno, 2013/04/02 21:08:47]

You need to be very careful here to honor the GVN flags. HStoreNamedFields below
need to set the kChangesInobjectFields GVN flag.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

is taken care of by HStoreNamedField.

+    Handle<Object> value = Handle<Object>(object->InObjectPropertyAt(i),
+        isolate());
+    if (value->IsJSObject()) {
+      Handle<JSObject> value_object = Handle<JSObject>::cast(value);
+      HInstruction* value_instruction =
+          AddInstruction(new(zone) HInnerAllocatedObject(result, *offset));
+      AddInstruction(new(zone) HStoreNamedField(
+          object_properties, factory->empty_string(), value_instruction, true,

[danno, 2013/04/02 21:08:47]

Instead of empty_string, perhaps we should add a "unknown_field_string()"?

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+          object->GetInObjectPropertyOffset(i)));
+      AddSimulate(id);
+      source = AddInstruction(new(zone) HConstant(

[danno, 2013/04/02 21:08:47]

name this something else, you mask the parameter called source

[Hannes Payer (out of office), 2013/04/08 11:03:00]

removed

+          value, Representation::Tagged()));
+      BuildEmitDeepCopy(value_object, result, source, offset,
+                        DONT_TRACK_ALLOCATION_SITE, id);
+    } else {
+      HInstruction* value_instruction = AddInstruction(new(zone) HConstant(
+          value, Representation::Tagged()));

[danno, 2013/04/02 21:08:47]

The value must be a smi, but without actually setting the type as such, you will
get a write barrier in the following store (not in new space, but if the
allocation is from old space), which you can elide with the right type.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

This is determined by the HType and not by the Representation.

+      AddInstruction(new(zone) HStoreNamedField(
+          object_properties, factory->empty_string(), value_instruction, true,
+          object->GetInObjectPropertyOffset(i)));

[danno, 2013/04/02 21:08:47]

You need to be very careful here to honor the GVN flags. Specifically, when you
copy the object's map, you need to call BuildStoreMap, and the HStoreNamedFields
below need to set the kChangesInobjectFields GVN flag.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

is done above.

+      AddSimulate(id);
+    }
+  }
+
+  // Build Allocation Site Info if desired
+  if (create_allocation_site_info) {
+    HValue* alloc_site =
+        AddInstruction(new(zone) HInnerAllocatedObject(result, JSArray::kSize));
+    Handle<Map> alloc_site_map(isolate()->heap()->allocation_site_info_map());
+    BuildStoreMap(alloc_site, alloc_site_map, id);
+    int alloc_payload_offset = AllocationSiteInfo::kPayloadOffset;
+    AddInstruction(new(zone) HStoreNamedField(alloc_site,
+                                              factory->empty_string(),

[danno, 2013/04/02 21:08:47]

create a "payload_string()"

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+                                              source,

[danno, 2013/04/02 21:08:47]

Can you really use the source for anything yet? It contains the boilerplate for
arrays (which should work and handle array transition correctly), but for
non-arrays, it's the source boilerplate object. Is it actually used anywhere
yet? Maybe in the case it's not a JSArray, you may want to put something else in
the boilerplate (null?) for now since there isn't much you can do with the
object boilerplate currently and later you will put a AllocationSiteInfo in its
place.  

[Hannes Payer (out of office), 2013/04/08 11:03:00]

I am using now the original_boilerplate.

+                                              true, alloc_payload_offset));
+    AddSimulate(id);
+  }
+
+  if (has_elements) {
+    HValue* boilerplate_elements =
+        AddInstruction(new(zone) HLoadElements(source, NULL));

[danno, 2013/04/02 21:08:47]

In this method have a mix here of loading from source for the value you need and
loading from HConstants (here you load using HLoadElements). Can you use
HConstants wherever possible? Later we can optimize lithium to fold the constant
copy into a field into a single instruction.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

I think it is the right thing to use HLoadElements here because of the GVN
flags.

+    HValue* object_elements =
+        AddInstruction(new(zone) HInnerAllocatedObject(
+            result, elements_offset));
+    AddInstruction(new(zone) HStoreNamedField(object_elements,

[danno, 2013/04/02 21:08:47]

Does this actually work? Why do you store boilerplate_elements into
object_elements? Don't you want to store object_elements into result at offset
kElementsOffset? I think you currently get the original elements object in
result's elements(), and here you store at offset kElementsOffset in
object_elements, which you iron over in the loop below. 

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Yes, this should be the same as what was done before in lithium

+                                              factory->elements_field_string(),
+                                              boilerplate_elements,
+                                              true, JSObject::kElementsOffset));
+    AddSimulate(id);
+    for (int i = 0; i < FixedArray::kHeaderSize; i += kPointerSize) {

[danno, 2013/04/02 21:08:47]

Consider refactoring HGraphBuilder::BuildAllocateElements into
BuildAllocateElements and BuildInitializeElements. You can then use
BuildInitializeElements here... it's a little overkill to use a copy loop for a
FixedArray header, and you need to handle the map store specially with
BuildStoreMap.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

Done.

+      HInstruction* value_instruction =
+          AddInstruction(new(zone) HLoadNamedField(
+              boilerplate_elements, true, i));
+      AddInstruction(new(zone) HStoreNamedField(object_elements,
+                                                factory->empty_string(),
+                                                value_instruction,
+                                                true, i));
+      AddSimulate(id);
+    }
+
+    // Copy elements backing store content.
+    int elements_length = elements->length();

[danno, 2013/04/02 21:08:47]

I think you can use HGraphBuilder::BuildCopyElements here, it saves a bunch of
code.

[Hannes Payer (out of office), 2013/04/08 11:03:00]

I looked into it but the code below calls recursively BuildEmitDeepCopy which
makes it complicated. It does not really save code.

+    if (elements->IsFixedDoubleArray()) {
+      for (int i = 0; i < elements_length; i++) {
+        HValue* key_constant =
+            AddInstruction(new(zone) HConstant(i, Representation::Integer32()));
+        HInstruction* value_instruction =
+            AddInstruction(new(zone) HLoadKeyed(
+                boilerplate_elements, key_constant, NULL, kind));
+        AddInstruction(new(zone) HStoreKeyed(
+                object_elements, key_constant, value_instruction, kind));
+        AddSimulate(id);
+      }
+    } else if (elements->IsFixedArray()) {
+      Handle<FixedArray> fast_elements = Handle<FixedArray>::cast(elements);
+      for (int i = 0; i < elements_length; i++) {
+        Handle<Object> value(fast_elements->get(i), isolate());
+        HValue* key_constant =
+            AddInstruction(new(zone) HConstant(i, Representation::Integer32()));
+        if (value->IsJSObject()) {
+          Handle<JSObject> value_object = Handle<JSObject>::cast(value);
+          HInstruction* value_instruction =
+              AddInstruction(new(zone) HInnerAllocatedObject(result, *offset));
+          AddInstruction(new(zone) HStoreKeyed(
+              object_elements, key_constant, value_instruction, kind));
+          AddSimulate(id);
+          source = AddInstruction(new(zone) HConstant(
+              value_object, Representation::Tagged()));
+          BuildEmitDeepCopy(value_object, result, source, offset,
+                            DONT_TRACK_ALLOCATION_SITE, id);
+        } else {
+          HInstruction* value_instruction =
+              AddInstruction(new(zone) HLoadKeyed(
+                  boilerplate_elements, key_constant, NULL, kind));
+          AddInstruction(new(zone) HStoreKeyed(
+              object_elements, key_constant, value_instruction, kind));
+          AddSimulate(id);
+        }
+      }
+    } else {
+      UNREACHABLE();
+    }
+  }
+}
+
+
+
 void HOptimizedGraphBuilder::VisitThisFunction(ThisFunction* expr) {
   ASSERT(!HasStackOverflow());
   ASSERT(current_block() != NULL);