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.
https://codereview.chromium.org/12880017/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
