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.
https://codereview.chromium.org/12880017/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
