Here's a first round.
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);
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.
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,
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)
}
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5258
src/hydrogen-instructions.h:5258: void SetGVNFlags(HValue *instr, bool
is_store) {
Make this protected and add HLoadNamedFiled and HStoreNamedField
friends.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5266
src/hydrogen-instructions.h:5266: if (may_allocate_)
instr->SetGVNFlag(kChangesNewSpacePromotion);
When does this actually happen? We should try to get rid of it,
loads/stores shouldn't allocate themselves.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5271
src/hydrogen-instructions.h:5271: is_store ? kChangesElementsKind :
kDependsOnElementsKind);
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?
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5293
src/hydrogen-instructions.h:5293: }
This guy really belongs in the .cc file.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5304
src/hydrogen-instructions.h:5304: inline Handle<String> name() {
This can be calculated by looking in the map's field descriptors for the
right offset.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#newcode5312
src/hydrogen-instructions.h:5312: Handle<String> name_;
You don't need to store this it can be looked-up if necessary. Part of
your change should remove the hard requirement on stores for the field
name (or maybe a precursor CL).
I think the only values you should have to store are:
int offset_: 30
bool in_object_ : 1
Handle<Map> map_;
If you just have these fields, you can just embed a copy of the
ObjectAccess into the HLoadNamedField/HStoreField instruction itself.
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)
Unless the objects is the size of intptr_t, then we always pass by
pointer (not by reference) and do not copy.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.cc#newcode1274
src/hydrogen.cc:1274: length = AddInstruction(
while your here, and you add a AddLoad (like your AddStore?)
length = AddLoad(object, ObjectAccess::ForArrayLength(this), mapcheck);
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.cc#newcode1601
src/hydrogen.cc:1601: ObjectAccess access = AccessArray(i);
Careful, this isn't what you think. This loop doesn't copy the elements
of an array, it copies the fields of a JSArrayObject, so using
AccessArray() isn't correct. In fact, given all of the different
representations we have for Arrays, AccessArray doesn't make since.
Instead, you should have:
ObjectAccess::ForElement(i);
ObjectAccess::ForDoubleElement(i);
which map to the appropriate flags for FixedArray and FixedDoubleArray,
respectively,
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.cc#newcode1615
src/hydrogen.cc:1615: AddStore(alloc_site, AccessMap(),
alloc_site_map_constant);
I still think a wrapper utility BuildStoreMap would make sense so that
you don't have to do the HConstant gymnastics again and again.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.cc#newcode1729
src/hydrogen.cc:1729: name = factory->map_field_string();
See my other comments about consolidating this logic in ObjectAccess'
class.
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);
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);
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.h#newcode1027
src/hydrogen.h:1027: ObjectAccess AccessField(Handle<Map> map,
Handle<String> name,
Shouldn't need the name here, the LookupResult has it.
https://codereview.chromium.org/14284010/diff/1/src/hydrogen.h#newcode1029
src/hydrogen.h:1029: ObjectAccess AccessInobject(Handle<String> name,
int offset);
Should take a map here. Look up the name in the descriptor array if you
need it.
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.
