Build fast literals in hydrogen.

Build fast literals in hydrogen.

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14211

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

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9912
src/hydrogen.cc:9912: boilerplate_object, Representation::Tagged()));
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9913
src/hydrogen.cc:9913: if (CanTransitionToMoreGeneralFastElementsKind(kind,
packed)) {
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9933
src/hydrogen.cc:9933: HInstruction* source,
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?

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9968
src/hydrogen.cc:9968: for (int i = 0; i < header_size; i += kPointerSize) {
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9975
src/hydrogen.cc:9975: header_value = AddInstruction(new(zone)
HLoadNamedField(source, true, i));
Are you sure that it's OK not to deep copy the non-inlined properties from the
header?

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9977
src/hydrogen.cc:9977: AddInstruction(new(zone) HStoreNamedField(object_header,
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9980
src/hydrogen.cc:9980: true, i));
Needs to set the kChangesInobjectFields GVN flag

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9987
src/hydrogen.cc:9987: for (int i = 0; i < inobject_properties; i++) {
You need to be very careful here to honor the GVN flags. HStoreNamedFields below
need to set the kChangesInobjectFields GVN flag.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9995
src/hydrogen.cc:9995: object_properties, factory->empty_string(),
value_instruction, true,
Instead of empty_string, perhaps we should add a "unknown_field_string()"?

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9998
src/hydrogen.cc:9998: source = AddInstruction(new(zone) HConstant(
name this something else, you mask the parameter called source

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10004
src/hydrogen.cc:10004: value, Representation::Tagged()));
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10007
src/hydrogen.cc:10007: object->GetInObjectPropertyOffset(i)));
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10020
src/hydrogen.cc:10020: factory->empty_string(),
create a "payload_string()"

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10021
src/hydrogen.cc:10021: source,
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10028
src/hydrogen.cc:10028: AddInstruction(new(zone) HLoadElements(source, NULL));
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10032
src/hydrogen.cc:10032: AddInstruction(new(zone)
HStoreNamedField(object_elements,
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10037
src/hydrogen.cc:10037: for (int i = 0; i < FixedArray::kHeaderSize; i +=
kPointerSize) {
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.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10049
src/hydrogen.cc:10049: int elements_length = elements->length();
I think you can use HGraphBuilder::BuildCopyElements here, it saves a bunch of
code.

[danno, 2013-04-02 21:18:54]

Ignore my comments about kChangesInobjectFields, that should work automatically.
However, calling BuildStoreMap is important to get the kChangesMaps flag.

[Michael Starzinger, 2013-04-03 09:23:23]

Drive-by nit.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.h#newcode1188
src/hydrogen.h:1188: static const int kMaxLiteralDepth = 3;
We should name this "kMaxFastLiteralDepth" and "kMaxFastLiteralProperties" I
think.

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

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9912
src/hydrogen.cc:9912: boilerplate_object, Representation::Tagged()));
On 2013/04/02 21:08:47, danno wrote:
> 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.

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9913
src/hydrogen.cc:9913: if (CanTransitionToMoreGeneralFastElementsKind(kind,
packed)) {
On 2013/04/02 21:08:47, danno wrote:
> 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.

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9933
src/hydrogen.cc:9933: HInstruction* source,
On 2013/04/02 21:08:47, danno wrote:
> 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? 

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9968
src/hydrogen.cc:9968: for (int i = 0; i < header_size; i += kPointerSize) {
On 2013/04/02 21:08:47, danno wrote:
> 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. 

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9975
src/hydrogen.cc:9975: header_value = AddInstruction(new(zone)
HLoadNamedField(source, true, i));
On 2013/04/02 21:08:47, danno wrote:
> Are you sure that it's OK not to deep copy the non-inlined properties from the
> header?

Now I am creating HConstants out of the boilerplate copy.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9977
src/hydrogen.cc:9977: AddInstruction(new(zone) HStoreNamedField(object_header,
On 2013/04/02 21:08:47, danno wrote:
> 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.

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9980
src/hydrogen.cc:9980: true, i));
On 2013/04/02 21:08:47, danno wrote:
> Needs to set the kChangesInobjectFields GVN flag

is done above.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9987
src/hydrogen.cc:9987: for (int i = 0; i < inobject_properties; i++) {
On 2013/04/02 21:08:47, danno wrote:
> You need to be very careful here to honor the GVN flags. HStoreNamedFields
below
> need to set the kChangesInobjectFields GVN flag.

is taken care of by HStoreNamedField.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9995
src/hydrogen.cc:9995: object_properties, factory->empty_string(),
value_instruction, true,
On 2013/04/02 21:08:47, danno wrote:
> Instead of empty_string, perhaps we should add a "unknown_field_string()"?

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode9998
src/hydrogen.cc:9998: source = AddInstruction(new(zone) HConstant(
On 2013/04/02 21:08:47, danno wrote:
> name this something else, you mask the parameter called source

removed

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10004
src/hydrogen.cc:10004: value, Representation::Tagged()));
On 2013/04/02 21:08:47, danno wrote:
> 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.

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

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10007
src/hydrogen.cc:10007: object->GetInObjectPropertyOffset(i)));
On 2013/04/02 21:08:47, danno wrote:
> 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.

is done above.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10020
src/hydrogen.cc:10020: factory->empty_string(),
On 2013/04/02 21:08:47, danno wrote:
> create a "payload_string()"

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10021
src/hydrogen.cc:10021: source,
On 2013/04/02 21:08:47, danno wrote:
> 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.  

I am using now the original_boilerplate.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10028
src/hydrogen.cc:10028: AddInstruction(new(zone) HLoadElements(source, NULL));
On 2013/04/02 21:08:47, danno wrote:
> 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.

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

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10032
src/hydrogen.cc:10032: AddInstruction(new(zone)
HStoreNamedField(object_elements,
On 2013/04/02 21:08:47, danno wrote:
> 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. 

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

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10037
src/hydrogen.cc:10037: for (int i = 0; i < FixedArray::kHeaderSize; i +=
kPointerSize) {
On 2013/04/02 21:08:47, danno wrote:
> 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.

Done.

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.cc#newcode10049
src/hydrogen.cc:10049: int elements_length = elements->length();
On 2013/04/02 21:08:47, danno wrote:
> I think you can use HGraphBuilder::BuildCopyElements here, it saves a bunch of
> code.

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

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/12880017/diff/1/src/hydrogen.h#newcode1188
src/hydrogen.h:1188: static const int kMaxLiteralDepth = 3;
On 2013/04/03 09:23:23, Michael Starzinger wrote:
> We should name this "kMaxFastLiteralDepth" and "kMaxFastLiteralProperties" I
> think.

Done.

[danno, 2013-04-09 08:52:42]

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode6200
src/hydrogen.cc:6200: new(zone()) HObjectLiteral(context,
nit: strange indenting?

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10200
src/hydrogen.cc:10200: HInstruction* boilerplate_elements =
From here....

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10208
src/hydrogen.cc:10208: true, JSObject::kElementsOffset));
... til here is not needed.

This is a FixedArray, it doesn't have a elements()

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10210
src/hydrogen.cc:10210: 
ASSERT(kFixedArrayHeaderSize = 2 * kPointer);

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10212
src/hydrogen.cc:10212: AddInstruction(new(zone)
HFixedArrayBaseLength(boilerplate_elements));
You can replace this with a constant load, too.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10279
src/hydrogen.cc:10279: HInstruction* value_instruction;
Maybe different names? elements?

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10284
src/hydrogen.cc:10284: header_field, Representation::None()));
Tagged

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10290
src/hydrogen.cc:10290: factory->empty_string(),
nit: indent

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10293
src/hydrogen.cc:10293: AddSimulate(id);
Make sure to set the kChangesElementsPointer GVN flag here

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10296
src/hydrogen.cc:10296: value_instruction = AddInstruction(new(zone) HConstant(
properties

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10301
src/hydrogen.cc:10301: true, JSObject::kPropertiesOffset));
As discussed, perhaps you only want to do this in the cow case? The allocation
of the elements might better belong next to the code above the initializes the
elements header and then copies the elements.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10312
src/hydrogen.cc:10312: factory->empty_string(),
length_field_string

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10314
src/hydrogen.cc:10314: true, JSArray::kLengthOffset));
Make sure to set the kChangesArrayLengths GVN flag here.

[Hannes Payer (out of office), 2013-04-09 14:20:13]

Additionally, I did minor cleanup changes.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode6200
src/hydrogen.cc:6200: new(zone()) HObjectLiteral(context,
On 2013/04/09 08:52:42, danno wrote:
> nit: strange indenting?

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10200
src/hydrogen.cc:10200: HInstruction* boilerplate_elements =
On 2013/04/09 08:52:42, danno wrote:
> From here....

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10208
src/hydrogen.cc:10208: true, JSObject::kElementsOffset));
On 2013/04/09 08:52:42, danno wrote:
> ... til here is not needed.
> 
> This is a FixedArray, it doesn't have a elements()

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10210
src/hydrogen.cc:10210: 
On 2013/04/09 08:52:42, danno wrote:
> ASSERT(kFixedArrayHeaderSize = 2 * kPointer);

What do you want to check but that assert?

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10212
src/hydrogen.cc:10212: AddInstruction(new(zone)
HFixedArrayBaseLength(boilerplate_elements));
On 2013/04/09 08:52:42, danno wrote:
> You can replace this with a constant load, too.

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10279
src/hydrogen.cc:10279: HInstruction* value_instruction;
On 2013/04/09 08:52:42, danno wrote:
> Maybe different names? elements?

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10284
src/hydrogen.cc:10284: header_field, Representation::None()));
On 2013/04/09 08:52:42, danno wrote:
> Tagged

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10290
src/hydrogen.cc:10290: factory->empty_string(),
On 2013/04/09 08:52:42, danno wrote:
> nit: indent

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10293
src/hydrogen.cc:10293: AddSimulate(id);
On 2013/04/09 08:52:42, danno wrote:
> Make sure to set the kChangesElementsPointer GVN flag here

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10296
src/hydrogen.cc:10296: value_instruction = AddInstruction(new(zone) HConstant(
On 2013/04/09 08:52:42, danno wrote:
> properties

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10301
src/hydrogen.cc:10301: true, JSObject::kPropertiesOffset));
On 2013/04/09 08:52:42, danno wrote:
> As discussed, perhaps you only want to do this in the cow case? The allocation
> of the elements might better belong next to the code above the initializes the
> elements header and then copies the elements.

This belongs to elements. Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10312
src/hydrogen.cc:10312: factory->empty_string(),
On 2013/04/09 08:52:42, danno wrote:
> length_field_string

Done.

https://codereview.chromium.org/12880017/diff/7001/src/hydrogen.cc#newcode10314
src/hydrogen.cc:10314: true, JSArray::kLengthOffset));
On 2013/04/09 08:52:42, danno wrote:
> Make sure to set the kChangesArrayLengths GVN flag here.

Done.

[danno, 2013-04-10 13:00:33]

LGTM with comments

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc#newcode10168
src/hydrogen.cc:10168: object_properties, factory->empty_string(),
value_instruction, true,
unknown_field_string

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc#newcode10177
src/hydrogen.cc:10177: object_properties, factory->empty_string(),
value_instruction, true,
unknown_field_string

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc#newcode10294
src/hydrogen.cc:10294: properties_field, Representation::None()));
Are you guaranteed to not have out-of line properties? If so, you may want to
assert that properties is the EmptyFixedArray.

[Hannes Payer (out of office), 2013-04-10 13:38:24]

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc#newcode10168
src/hydrogen.cc:10168: object_properties, factory->empty_string(),
value_instruction, true,
On 2013/04/10 13:00:33, danno wrote:
> unknown_field_string

Done.

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc#newcode10177
src/hydrogen.cc:10177: object_properties, factory->empty_string(),
value_instruction, true,
On 2013/04/10 13:00:33, danno wrote:
> unknown_field_string

Done.

https://codereview.chromium.org/12880017/diff/14001/src/hydrogen.cc#newcode10294
src/hydrogen.cc:10294: properties_field, Representation::None()));
On 2013/04/10 13:00:33, danno wrote:
> Are you guaranteed to not have out-of line properties? If so, you may want to
> assert that properties is the EmptyFixedArray.

Done.

[Hannes Payer (out of office), 2013-04-10 13:52:35]

Committed patchset #5 manually as r14211 (presubmit successful).