https://codereview.chromium.org/14284010/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/14284010/diff/1/src/code-stubs-hydrogen.cc#newcode371
src/code-stubs-hydrogen.cc:371: ObjectAccess access =
AccessInobject(factory->empty_string(), i);
On 2013/04/26 09:39:38, danno wrote:
Instead of taking the field name, I think this version should take a
map, in
this case it should be context->object_function()->initial_map(). The
logic
inside AccessInObject should be able to map to a string if it needs
it.
Sure. Deferring this change to the follow-up CL.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5246
src/hydrogen-instructions.h:5246: kElementsKind,
On 2013/04/26 09:39:38, danno wrote:
The seems redundant to me. It seems like this is what the flags
represent in the
first place. Instead, I suggest SetGVNFlags use the map and offset and
a little
more complicated logic to determine the right flags. This should do
the trick:
if (map.is_null) {
// Might want to change "SpecializedArrayElements" to
"NonHeapMemory"
instr->SetGVNFlag(is_store ? kChangesSpecializedArrayElements :
kDependsOnSpecializedArrayElements);
return;
} else if (offset == 0) {
instr->SetGVNFlag(is_store ? kChangesMaps : kDependsOnMaps);
return;
} else if (map == heap->fixed_array_map() ||
map == heap->fixed_double_array_map()) {
if (offset == FixedArray::kLengthOffset) {
instr->SetGVNFlag(is_store ? kChangesArrayLengths :
kDependsOnArrayLengths);
} else {
if (map == heap->fixed_array_map()) {
instr->SetGVNFlag(is_store ? kChangesArrayElements :
kDependsOnArrayElements);
} else {
instr->SetGVNFlag(is_store ? kChangesDoubleArrayElements :
kDependsOnDoubleArrayElements);
}
}
return;
}
// Must be a JSObject or a JSArray
ASSERT(map->instance_type() == JS_OBJECT ||
map->instance_type() == JS_ARRAY);
if (offset == JSObject::kPropertiesOffset) {
instr->SetGVNFlag(is_store ? kChangesPropertyPointer :
kDependsOnPropertyPointer)
return;
} else if (offset == JSObject::kElementsOffset) {
instr->SetGVNFlag(is_store ? kChangesElementsPointer :
kDependsOnElementsPointer)
return;
}
if (map->instance_type() == JS_ARRAY) {
ASSERT(offset == JSArray::kArrayLength);
instr->SetGVNFlag(is_store ? kChangesArrayLengths :
kDependsOnArrayLengths)
return;
if (in_object) {
instr->SetGVNFlag(is_store ? kChangesInobjectFields :
kDependsOnInobjectFields)
} else {
instr->SetGVNFlag(is_store ? kChangesBackingStoreFields :
kDependsOnBackingStoreFields)
}
As discussed in person, I think we can make this work in a follow-up CL
where we move away from having an enum specifying different object
portions. For this intermediate point, the enum essentially preserves
what is known at the object access construction sites, rather than
relying on a method like the above to reconstruct it.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5258
src/hydrogen-instructions.h:5258: void SetGVNFlags(HValue *instr, bool
is_store) {
On 2013/04/26 09:39:38, danno wrote:
Make this protected and add HLoadNamedFiled and HStoreNamedField
friends.
Done.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5266
src/hydrogen-instructions.h:5266: if (may_allocate_)
instr->SetGVNFlag(kChangesNewSpacePromotion);
On 2013/04/26 09:39:38, danno wrote:
When does this actually happen? We should try to get rid of it,
loads/stores
shouldn't allocate themselves.
Done.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5271
src/hydrogen-instructions.h:5271: is_store ? kChangesElementsKind :
kDependsOnElementsKind);
On 2013/04/26 09:39:38, danno wrote:
Don't know if it makes the code cleaner, but maybe you can make use of
ConvertChangesToDependsFlags here somehow for the store case in one
place?
That method takes a GVNFlagSet and does a shift. I toyed with having a
version that converts a single changes flag to a depends flag, but it is
essentially a mysterious +1 and didn't save enough here to be worth it
IMO.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5293
src/hydrogen-instructions.h:5293: }
On 2013/04/26 09:39:38, danno wrote:
This guy really belongs in the .cc file.
Done.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5304
src/hydrogen-instructions.h:5304: inline Handle<String> name() {
On 2013/04/26 09:39:38, danno wrote:
This can be calculated by looking in the map's field descriptors for
the right
offset.
I agree with that approach in the long run. We can do that when
ObjectAccesses consists of a map and an offset.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5318
src/hydrogen-instructions.h:5318: HLoadNamedField(HValue* object,
ObjectAccess access, HValue* typecheck = NULL)
On 2013/04/26 09:39:38, danno wrote:
Unless the objects is the size of intptr_t, then we always pass by
pointer (not
by reference) and do not copy.
Done.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.h#newcode1022
src/hydrogen.h:1022: ObjectAccess AccessArray(int offset);
On 2013/04/26 09:39:38, danno wrote:
I think these methods are better suited as static methods on the
ObjectAccess
class in order to encapsulate the logic mapping fields to their
flags/etc. in
one class (even though it means passing the builder--or at least the
isolate--as
a parameter):
ObjectAccess::ForArrayLength(this);
I'm OK with putting the guts of those methods in ObjectAccess if they
have to do anything tricky, but as for hydrogen.cc, having these methods
saves a lot of characters, reducing the need for other intermediate
build methods (e.g. BuildStoreMap is now gone)
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.h#newcode1027
src/hydrogen.h:1027: ObjectAccess AccessField(Handle<Map> map,
Handle<String> name,
On 2013/04/26 09:39:38, danno wrote:
Shouldn't need the name here, the LookupResult has it.
I hunted around inside the LookupResult and couldn't find it. Does it
require looking up in the map with the index result from the
LookupResult?
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.h#newcode1029
src/hydrogen.h:1029: ObjectAccess AccessInobject(Handle<String> name,
int offset);
On 2013/04/26 09:39:38, danno wrote:
Should take a map here. Look up the name in the descriptor array if
you need it.
We might need another version that takes a map when we move the
ObjectAccess to use maps.
https://codereview.chromium.org/14284010/
--
--
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.
