Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14284010:
Introduce HObjectAccess, which is used by LoadNamedField and StoreNamedField to denote what parts (Closed)
|
Created:
7 years, 8 months ago by titzer Modified:
7 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionIntroduce ObjectAccess, which is used by LoadNamedField and StoreNamedField to denote what parts of an object are referred to by a given load or store. Refactor HGraphBuilder to use ObjectAccess, which removes the need to manually set GVN flags and simplifies the code as well.
Committed: https://code.google.com/p/v8/source/detail?r=14791
Patch Set 1 #
Total comments: 30
Patch Set 2 : Integrate feedback from danno; rename ObjectAccess HObjectAccess, allocate them in the zone. #Patch Set 3 : Some small cleanups; add AddLoad to simplify construction of loads #
Total comments: 19
Patch Set 4 : Move all construction of HObjectAccess into static methods, hiding Portion. #
Total comments: 31
Patch Set 5 : Make HObjectAccess into a free-wheelin' hippy-style peace-and-love value object again. #Patch Set 6 : Make constructor of HObjectAccess and HObjectAccess::Portion private. #
Total comments: 22
Patch Set 7 : Split apart ForOffset into ForJSObjectOffset, ForJSArrayOffset, and ForFixedArrayOffset #
Total comments: 4
Patch Set 8 : Add HObjectAccess::ForBackingStoreOffset and HObjectAccess::ForAllocationSitePayload. #
Total comments: 23
Patch Set 9 : Merge with changes from verwaest #Patch Set 10 : Rename HObjectAccess::ForFixedArrayOffset, change formatting of HObject::PrintTo, use HKeyedLoad in… #
Total comments: 3
Patch Set 11 : Make portion 4 bits; remove kXDoubleFields GVNFlag. #
Total comments: 1
Patch Set 12 : Use BitField utility instead of C-language bitfield for portion and offset in HObjectAccess. #
Messages
Total messages: 21 (0 generated)
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#ne... 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#n... 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#n... 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#n... 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#n... 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#n... src/hydrogen-instructions.h:5293: } This guy really belongs in the .cc file. https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#n... 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#n... 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#n... 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/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#ne... 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#n... 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#n... 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#n... 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#n... 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#n... 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#n... 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#n... 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/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/1/src/hydrogen-instructions.h#n... src/hydrogen-instructions.h:5312: Handle<String> name_; You didn't address this comment. Why not? On 2013/04/26 09:39:38, danno wrote: > 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.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); Tricky/non-trick is an arbitrary judgment call, I don't think encapsulation is... and it's binary. I feel pretty strongly that if we're going through all this trouble to create a class to hide the details of mapping ObjectAccess to portions/flags/whatever, all of the logic for that should be in on place... the ObjectAccess class and not partially sprinkled through some methods on HGraphBuilder. I don't think there should be any reason to expose the "portion" of the ObjectAccess in the public API. Also, "saves a lot of characters" doesn't seem like a very good argument for choosing readable names. For example, I think "AccessInobject" doesn't tell me much. AccessInobject what? Properties? If so, why not put that in the name? Access is a verb, so does the method actually access something? I don't mean to be pedantic, but for others reading the code for the first time, a few extra characters can drastically improve readability if they are the right ones. On 2013/04/30 15:56:47, titzer wrote: > 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/10001/src/code-stubs-hydrogen.cc File src/code-stubs-hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/10001/src/code-stubs-hydrogen.c... src/code-stubs-hydrogen.cc:379: HObjectAccess* access = AccessInobject(factory->empty_string(), i); s/AccessInobject/AccessInObjectProperty https://codereview.chromium.org/14284010/diff/10001/src/code-stubs-hydrogen.c... src/code-stubs-hydrogen.cc:381: AddInstruction(new(zone) HLoadNamedField(boilerplate, access)); nit: 4 char indent (please run tools/presubmit.py before uploading) https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3655: nit: 2 lines https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3658: if (!is_store) { Why not just switch the else and then blocks so it's "if (is_store)" https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3668: case kArrayLengths: Weird indentation. Elsewhere in the code we use 2-char indents for cases, please use that here, too. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode1944 src/hydrogen.cc:1944: HObjectAccess* HGraphBuilder::AccessArrayHeader(int offset) { I am not convinced this should be separate method. I think it's useful to have factories for specific fields, but the only one "generic" version that handles everything else. This avoid logic duplication. In this case, JSObjects all have elements pointers, including arrays, so there should be a ObjectAccess::ForElements for all JSObjects/JSArrays. For Maps, we already have a accessor, nothing needed there. Adding ObjectAccess::ForArrayLength seems to make sense. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode1964 src/hydrogen.cc:1964: return new (zone()) HObjectAccess(HObjectAccess::kArrayLengths, This method and the methods below should return the address of singletons. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode2008 src/hydrogen.cc:2008: HObjectAccess(HObjectAccess::kBackingStore, offset, name); Seems like a wast of memory, these can be stack allocated with the right constructor. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode2013 src/hydrogen.cc:2013: return new(zone()) HObjectAccess(HObjectAccess::kInobject, offset, name); Same here... https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.h#newcode1038 src/hydrogen.h:1038: HObjectAccess* AccessInobject(Handle<String> name, int offset); Why not have this as a constructor for HObjectAccess so it can be allocated on the stack? Prevents having to allocate lots of temporary objects that don't have to live very long anyway.
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/05/02 14:16:47, danno wrote: > Tricky/non-trick is an arbitrary judgment call, I don't think encapsulation > is... and it's binary. I feel pretty strongly that if we're going through all > this trouble to create a class to hide the details of mapping ObjectAccess to > portions/flags/whatever, all of the logic for that should be in on place... the > ObjectAccess class and not partially sprinkled through some methods on > HGraphBuilder. I don't think there should be any reason to expose the "portion" > of the ObjectAccess in the public API. > > Also, "saves a lot of characters" doesn't seem like a very good argument for > choosing readable names. For example, I think "AccessInobject" doesn't tell me > much. AccessInobject what? Properties? If so, why not put that in the name? > Access is a verb, so does the method actually access something? I don't mean to > be pedantic, but for others reading the code for the first time, a few extra > characters can drastically improve readability if they are the right ones. > > > On 2013/04/30 15:56:47, titzer wrote: > > 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) > I've moved these methods into static constructors in HObjectAccess. What I meant by saving characters was that writing AccessX() within the GraphBuilder code is shorter than writing HObjectAccess::X(zone()), because GraphBuilder already closes over zone, and the code is already in the GraphBuilder scope. I did not mean to argue X versus a more readable AccessX. But I'm OK with putting it all in one place at the expense of needing HObjectAccess:: as a prefix. https://codereview.chromium.org/14284010/diff/10001/src/code-stubs-hydrogen.cc File src/code-stubs-hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/10001/src/code-stubs-hydrogen.c... src/code-stubs-hydrogen.cc:379: HObjectAccess* access = AccessInobject(factory->empty_string(), i); On 2013/05/02 14:16:47, danno wrote: > s/AccessInobject/AccessInObjectProperty I've moved this method to HObjectAccess::ForInobjectOffset. https://codereview.chromium.org/14284010/diff/10001/src/code-stubs-hydrogen.c... src/code-stubs-hydrogen.cc:381: AddInstruction(new(zone) HLoadNamedField(boilerplate, access)); On 2013/05/02 14:16:47, danno wrote: > nit: 4 char indent (please run tools/presubmit.py before uploading) Done. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3655: On 2013/05/02 14:16:47, danno wrote: > nit: 2 lines Done. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3658: if (!is_store) { On 2013/05/02 14:16:47, danno wrote: > Why not just switch the else and then blocks so it's "if (is_store)" Done. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3668: case kArrayLengths: On 2013/05/02 14:16:47, danno wrote: > Weird indentation. Elsewhere in the code we use 2-char indents for cases, please > use that here, too. Done. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode1944 src/hydrogen.cc:1944: HObjectAccess* HGraphBuilder::AccessArrayHeader(int offset) { On 2013/05/02 14:16:47, danno wrote: > I am not convinced this should be separate method. I think it's useful to have > factories for specific fields, but the only one "generic" version that handles > everything else. This avoid logic duplication. > > In this case, JSObjects all have elements pointers, including arrays, so there > should be a ObjectAccess::ForElements for all JSObjects/JSArrays. For Maps, we > already have a accessor, nothing needed there. Adding > ObjectAccess::ForArrayLength seems to make sense. I've moved this and other constructor methods to HObjectAccess. The original reason for having an AccessArrayHeader method was that it was used in places where it was known that an array header (but not exactly which kind of array--no map) was being accessed. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode1964 src/hydrogen.cc:1964: return new (zone()) HObjectAccess(HObjectAccess::kArrayLengths, On 2013/05/02 14:16:47, danno wrote: > This method and the methods below should return the address of singletons. Done (in HObjectAccess). https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.cc#newcode2008 src/hydrogen.cc:2008: HObjectAccess(HObjectAccess::kBackingStore, offset, name); On 2013/05/02 14:16:47, danno wrote: > Seems like a wast of memory, these can be stack allocated with the right > constructor. Can't do that, since a pointer to them is retained in HLoadNamedField and HStoreNamedField. https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/14284010/diff/10001/src/hydrogen.h#newcode1038 src/hydrogen.h:1038: HObjectAccess* AccessInobject(Handle<String> name, int offset); On 2013/05/02 14:16:47, danno wrote: > Why not have this as a constructor for HObjectAccess so it can be allocated on > the stack? Prevents having to allocate lots of temporary objects that don't have > to live very long anyway. Because the HObjectAccess pointers are actually being stored in the load/store instructions and thus live longer than the graph builder's stack.
Getting pretty close, I really like the direction this is going. https://codereview.chromium.org/14284010/diff/17001/.gitignore File .gitignore (right): https://codereview.chromium.org/14284010/diff/17001/.gitignore#newcode27 .gitignore:27: bsuite Put this in a separate CL. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (left): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3653: Stray whitespace removal, please add back. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3685: nit: two newlines https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3701: nit: two lines https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3706: nit: two lines https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5228: // internal use only; different parts of an object or array Can you please make the following enum protected/private? https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5237: HObjectAccess(Portion portion, int offset, This should be private. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5241: inline bool IsInobject() { Does IsInobject or offset have to be public? https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5253: static HObjectAccess* ForElementsPointer(); OK, you've convinced me. There is precidence elsewhere to return objects by value rather than object by pointer, and after talking to Jakob about zone usage, I think allocating a new object for each field is probably a bad idea. Sorry, can you got back to the non-pointer version. :-) https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5263: static HObjectAccess* ForOffset(Zone *zone, int offset, Roll ForInobjectOffset directly into this method. There should only be ForOffset, and it should handle all cases. We should try to minimize the idea of "inobject/outofobject" in the API, since it will be going away when we make all out-of-properties two-step anyway. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5274: Portion portion_; Please put these together into a bit field, int portion_: 30; int offset_ : 1; https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode1918 src/hydrogen.cc:1918: instr->set_field_representation(representation); Any reason you took this out of the constructor? https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode1923 src/hydrogen.cc:1923: HLoadNamedField* HGraphBuilder::AddLoad(HValue *object, HObjectAccess* access, nit: Here and elsewhere, although not strictly dictated by the style guide, we tend to indent parameters spilled to the next line with the first parameter on the previous one... https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode1925 src/hydrogen.cc:1925: HLoadNamedField *instr = new(zone()) nit: we usually would line break before the new(zone()) ... so entire new clause and construction name are on the same line. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode6921 src/hydrogen.cc:6921: instr->set_field_representation(ComputeLoadStoreRepresentation(map, lookup)); Any reason that you took the setting of the representation and moved it out of the constructor? https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode7665 src/hydrogen.cc:7665: HCheckPrototypeMaps(prototype, holder, zone())); nit: unnecessary whitespace change
https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (left): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3653: On 2013/05/06 15:53:06, danno wrote: > Stray whitespace removal, please add back. Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3685: On 2013/05/06 15:53:06, danno wrote: > nit: two newlines Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3701: On 2013/05/06 15:53:06, danno wrote: > nit: two lines Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3706: On 2013/05/06 15:53:06, danno wrote: > nit: two lines Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5228: // internal use only; different parts of an object or array On 2013/05/06 15:53:06, danno wrote: > Can you please make the following enum protected/private? Haha. Tried that. Can't do that because I can't make initialized static members of the class that are not primitives. I wanted to make some global shared HObjectAccess instances, so they have to be outside the class, which means they need access to the enum. Love it. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5237: HObjectAccess(Portion portion, int offset, On 2013/05/06 15:53:06, danno wrote: > This should be private. Wish I could do that, but I can't, for the same reason. Aggh. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5241: inline bool IsInobject() { On 2013/05/06 15:53:06, danno wrote: > Does IsInobject or offset have to be public? They are used in HandlePolymorphicNamedField to check that the accesses are equal in terms of inobject-ness and offset. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5274: Portion portion_; On 2013/05/06 15:53:06, danno wrote: > Please put these together into a bit field, > int portion_: 30; > int offset_ : 1; Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode1918 src/hydrogen.cc:1918: instr->set_field_representation(representation); On 2013/05/06 15:53:06, danno wrote: > Any reason you took this out of the constructor? Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode1923 src/hydrogen.cc:1923: HLoadNamedField* HGraphBuilder::AddLoad(HValue *object, HObjectAccess* access, On 2013/05/06 15:53:06, danno wrote: > nit: Here and elsewhere, although not strictly dictated by the style guide, we > tend to indent parameters spilled to the next line with the first parameter on > the previous one... Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode1925 src/hydrogen.cc:1925: HLoadNamedField *instr = new(zone()) On 2013/05/06 15:53:06, danno wrote: > nit: we usually would line break before the new(zone()) ... so entire new clause > and construction name are on the same line. Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode6921 src/hydrogen.cc:6921: instr->set_field_representation(ComputeLoadStoreRepresentation(map, lookup)); On 2013/05/06 15:53:06, danno wrote: > Any reason that you took the setting of the representation and moved it out of > the constructor? Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen.cc#newcode7665 src/hydrogen.cc:7665: HCheckPrototypeMaps(prototype, holder, zone())); On 2013/05/06 15:53:06, danno wrote: > nit: unnecessary whitespace change want: auto code formatter
https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5228: // internal use only; different parts of an object or array On 2013/05/07 17:51:03, titzer wrote: > On 2013/05/06 15:53:06, danno wrote: > > Can you please make the following enum protected/private? > > Haha. Tried that. Can't do that because I can't make initialized static members > of the class that are not primitives. I wanted to make some global shared > HObjectAccess instances, so they have to be outside the class, which means they > need access to the enum. Love it. Nevermind, moving it to a value object made this possible. Done. https://codereview.chromium.org/14284010/diff/17001/src/hydrogen-instructions... src/hydrogen-instructions.h:5237: HObjectAccess(Portion portion, int offset, On 2013/05/07 17:51:03, titzer wrote: > On 2013/05/06 15:53:06, danno wrote: > > This should be private. > > Wish I could do that, but I can't, for the same reason. Aggh. Spoke too soon. Done.
I'll come by in person to discuss some of this. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3645: portion = kArrayLengths; // XXX: only true for arrays? This is only true for arrays, which should _always_ use the ForArrayLength variant. This case should be removed, it is just another case of kInobject. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions... src/hydrogen-instructions.h:5222: static HObjectAccess For(bool is_inobject, int offset); This should be "ForJSObjectField". It only works for JSObjects (the concept of "is_inobject" only is valid for JSObjects and their sub-classes. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions... src/hydrogen-instructions.h:5225: static HObjectAccess ForOffset(int offset, This guy is *super* dangerous. Either it should only work on JSObjects, or you *must* pass in a map. It's important to be clear that object's are not always JSObjects, there are all sorts of HeapObject that we store to. ForOffset can't be the swiss-army knife you'd like it to be without knowing exactly what kind of object you're operating on. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1411 src/hydrogen.cc:1411: IsFastElementsKind(kind) Calculate this in an intermediate Representation variable that is passed in, it's more readable. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1666 src/hydrogen.cc:1666: HObjectAccess access = HObjectAccess::ForOffset(i); Replace this loop with a version of with a call to a new routine in the style of BuildCopyObjectHeader that copies from a HValue rather than from a boilerplate. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1688 src/hydrogen.cc:1688: HObjectAccess access = HObjectAccess::ForOffset(i); Yikes, this is dangerous. The offset in a fixed array will give you the properties objects rather then array length. Again, probably a helper routine BuildCopyFixedArrayHeader might be helpful. Load -> Store map Load -> Store ForFixedArrayLength() https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1798 src/hydrogen.cc:1798: HObjectAccess access = HObjectAccess::ForOffset( This is the contents of a FixedArray, so you need a new routine ForFixedArrayElement(i), which ends up setting a new GVN flag for ArrayElements. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1804 src/hydrogen.cc:1804: access = HObjectAccess::ForOffset(offset); Same here, this is an access into the elements of a FixedArray. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1807 src/hydrogen.cc:1807: offset = kind_ * kPointerSize + FixedArrayBase::kHeaderSize; Same here. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode10684 src/hydrogen.cc:10684: HObjectAccess access = HObjectAccess::ForOffset( For(true, i) https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode10695 src/hydrogen.cc:10695: HObjectAccess access = HObjectAccess::ForOffset( For(true, i), how about you more the access calculation an the AddStore after the "if (value->IsJSObject)" clause, it's the same in both cases. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode10794 src/hydrogen.cc:10794: HObjectAccess access = HObjectAccess::ForOffset(JSObject::kPropertiesOffset); You should have a ForPropertiesPointer(). It should current use the inobject properties GVN flag. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode11197 src/hydrogen.cc:11197: AddStore(object, HObjectAccess::ForOffset(JSValue::kValueOffset), value); JSValues are not necessarily JSObjects, there should be a separate For.... for this guy. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.h#newcode1002 src/hydrogen.h:1002: HValue* BuildCheckForCapacityGrow( In general, please avoid extraneous whitespace changes, they just make the likelihood of being able to merge this patch to other branches in other contexts lower. If you want to do whitespace cleanup (in this case it's a bit iffy if it's necessary), then please in another CL.
https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3645: portion = kArrayLengths; // XXX: only true for arrays? On 2013/05/08 12:05:35, danno wrote: > This is only true for arrays, which should _always_ use the ForArrayLength > variant. This case should be removed, it is just another case of kInobject. I've split this into ForJSObjectOffset and ForJSArrayOffset. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions... src/hydrogen-instructions.h:5222: static HObjectAccess For(bool is_inobject, int offset); On 2013/05/08 12:05:35, danno wrote: > This should be "ForJSObjectField". It only works for JSObjects (the concept of > "is_inobject" only is valid for JSObjects and their sub-classes. The only remaining use of this is for accessing a heap object allocation site (not a JSObject), and the code stubs, which now check whether they are inobject, and if so, call the ForJSObjectOffset. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen-instructions... src/hydrogen-instructions.h:5225: static HObjectAccess ForOffset(int offset, On 2013/05/08 12:05:35, danno wrote: > This guy is *super* dangerous. Either it should only work on JSObjects, or you > *must* pass in a map. It's important to be clear that object's are not always > JSObjects, there are all sorts of HeapObject that we store to. ForOffset can't > be the swiss-army knife you'd like it to be without knowing exactly what kind of > object you're operating on. I've split it apart into ForJSObjectOffset, ForJSArrayOffset, and ForFixedArrayOffset. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1411 src/hydrogen.cc:1411: IsFastElementsKind(kind) On 2013/05/08 12:05:35, danno wrote: > Calculate this in an intermediate Representation variable that is passed in, > it's more readable. Done. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1666 src/hydrogen.cc:1666: HObjectAccess access = HObjectAccess::ForOffset(i); On 2013/05/08 12:05:35, danno wrote: > Replace this loop with a version of with a call to a new routine in the style of > BuildCopyObjectHeader that copies from a HValue rather than from a boilerplate. Now that I've added ForJSArrayOffset(), I actually find the loop to be clearer, and should be safer, if ever someone were crazy enough to add another field to a JSArray. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1688 src/hydrogen.cc:1688: HObjectAccess access = HObjectAccess::ForOffset(i); On 2013/05/08 12:05:35, danno wrote: > Yikes, this is dangerous. The offset in a fixed array will give you the > properties objects rather then array length. Again, probably a helper routine > BuildCopyFixedArrayHeader might be helpful. > Load -> Store map > Load -> Store ForFixedArrayLength() I've fixed this with ForFixedArrayOffset. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.cc#newcode1798 src/hydrogen.cc:1798: HObjectAccess access = HObjectAccess::ForOffset( On 2013/05/08 12:05:35, danno wrote: > This is the contents of a FixedArray, so you need a new routine > ForFixedArrayElement(i), which ends up setting a new GVN flag for ArrayElements. > I've replaced each of these three accesses with ForFixedArrayOffset. https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/14284010/diff/28001/src/hydrogen.h#newcode1002 src/hydrogen.h:1002: HValue* BuildCheckForCapacityGrow( On 2013/05/08 12:05:35, danno wrote: > In general, please avoid extraneous whitespace changes, they just make the > likelihood of being able to merge this patch to other branches in other contexts > lower. If you want to do whitespace cleanup (in this case it's a bit iffy if > it's necessary), then please in another CL. Done.
Very close... https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3637: return HObjectAccess(kInobject, offset); If think you might want to check offset == FixedArray::kArrayLength and return portion kArrayLengths in that case. https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions... src/hydrogen-instructions.h:5226: static HObjectAccess For(bool is_inobject, int offset, If you add ForAllocationSitePayload, then you can just make this ForBackingStoreProperty?
https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3633: return HObjectAccess(kInobject, offset); How about handling the map at offset zero like others? For now offsets > FixedArray::kLengthOffset should assert, since they need to be KeyedLoads. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3639: ASSERT(offset >= 0); I think this assert is bogus (maps?) https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3653: ASSERT(offset >= 0); I think this assert is bogus (maps?) https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.h:5234: Handle<String> name = Handle<String>::null()); Why do you pass names in here and to the other variants below? You never pass anything but the default value. Also, I think you lose some functionality here. Some fields (like array lengths) have pseudo names like %length. Do they still get output in the hydrogen code?
https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3637: return HObjectAccess(kInobject, offset); On 2013/05/08 15:40:18, danno wrote: > If think you might want to check offset == FixedArray::kArrayLength and return > portion kArrayLengths in that case. Done. https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/35001/src/hydrogen-instructions... src/hydrogen-instructions.h:5226: static HObjectAccess For(bool is_inobject, int offset, On 2013/05/08 15:40:18, danno wrote: > If you add ForAllocationSitePayload, then you can just make this > ForBackingStoreProperty? Done. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3633: return HObjectAccess(kInobject, offset); On 2013/05/10 15:59:19, danno wrote: > How about handling the map at offset zero like others? For now offsets > > FixedArray::kLengthOffset should assert, since they need to be KeyedLoads. Unfortunately we use this variant to construct a direct access to an element of a fixed array in BuildNativeGetContext :( https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3639: ASSERT(offset >= 0); On 2013/05/10 15:59:19, danno wrote: > I think this assert is bogus (maps?) Maps seem to be at offset 0 on ia32. Is this true for all platforms? https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.h:5234: Handle<String> name = Handle<String>::null()); On 2013/05/10 15:59:19, danno wrote: > Why do you pass names in here and to the other variants below? You never pass > anything but the default value. I was thinking that I would pass in the name of the field, but you are right; we only use an explicit name in the case of ForField(). > Also, I think you lose some functionality here. Some fields (like array lengths) > have pseudo names like %length. Do they still get output in the hydrogen code? Yes. I added a PrintTo() method for HObjectAccess that knows what name to print for the various special Portion enums. This means the loads also print names correctly now.
https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3633: return HObjectAccess(kInobject, offset); OK, then you will have to be very careful here. There are two types of FixedArrays, FixedArrays and FixedDouble arrays. They both share a common base class, but depending on which sub-class you use of FixedArray base, you need to not invalidate kInobject but rather ArrayElements or DoubleArrayElements. I think this should actually be FixedArrayBaseOffset that only handles the header, and you should change the context-accessing code to use a KeyedLoad, which handled the dependencies correctly depending on the elements kind, IIRC (in this case, using a NamedLoad to access the context probably works but is not right in principle). You can just pass a HConstant as the right index and it will be just as efficient as the HNamedLoad On 2013/05/13 11:23:19, titzer wrote: > On 2013/05/10 15:59:19, danno wrote: > > How about handling the map at offset zero like others? For now offsets > > > FixedArray::kLengthOffset should assert, since they need to be KeyedLoads. > > Unfortunately we use this variant to construct a direct access to an element of > a fixed array in BuildNativeGetContext :( > https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3639: ASSERT(offset >= 0); Ah, yes, it's not -1, it's still untagged at this point. On 2013/05/13 11:23:19, titzer wrote: > On 2013/05/10 15:59:19, danno wrote: > > I think this assert is bogus (maps?) > > Maps seem to be at offset 0 on ia32. Is this true for all platforms? https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3653: ASSERT(offset >= 0); Yeah, ignore this On 2013/05/10 15:59:19, danno wrote: > I think this assert is bogus (maps?) https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3738: stream->Add(".@%d", offset_); I'd prefer the output of the form (it's the way it was before and it was more JavaScript-esque): o.field_name@2345 field_name should use "%"-prefixed field names for private (internal) field as they were defined before your CL, e.g. (%map, %length,etc). https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3752: stream->Add("[in-object]"); Shouldn't you only print this if there is no name, i.e. when name.is_null: o.<unknown-in-object> https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3757: if (!name_.is_null()) { Seems like this should be mutually exclusive with everything except kInobject. An assert to make sure? https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3759: stream->Add(*String::cast(*name_)->ToCString()); See above, please use existing JavaScript-like syntax for printing field names. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.h:5234: Handle<String> name = Handle<String>::null()); It seems, however, that there is a regression. In lithium-xxx, you no longer output the name of the field. Can you please fix that before landing this? On 2013/05/13 11:23:19, titzer wrote: > On 2013/05/10 15:59:19, danno wrote: > > Why do you pass names in here and to the other variants below? You never pass > > anything but the default value. > > I was thinking that I would pass in the name of the field, but you are right; we > only use an explicit name in the case of ForField(). > > > Also, I think you lose some functionality here. Some fields (like array > lengths) > > have pseudo names like %length. Do they still get output in the hydrogen code? > > Yes. I added a PrintTo() method for HObjectAccess that knows what name to print > for the various special Portion enums. This means the loads also print names > correctly now. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen.cc#newcode1195 src/hydrogen.cc:1195: IsFastElementsKind(kind) nit: For readability, please calculate the representation in a separate variable above.
https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3633: return HObjectAccess(kInobject, offset); On 2013/05/13 15:44:32, danno wrote: > OK, then you will have to be very careful here. There are two types of > FixedArrays, FixedArrays and FixedDouble arrays. They both share a common base > class, but depending on which sub-class you use of FixedArray base, you need to > not invalidate kInobject but rather ArrayElements or DoubleArrayElements. > > I think this should actually be FixedArrayBaseOffset that only handles the > header, and you should change the context-accessing code to use a KeyedLoad, > which handled the dependencies correctly depending on the elements kind, IIRC > (in this case, using a NamedLoad to access the context probably works but is not > right in principle). You can just pass a HConstant as the right index and it > will be just as efficient as the HNamedLoad > > On 2013/05/13 11:23:19, titzer wrote: > > On 2013/05/10 15:59:19, danno wrote: > > > How about handling the map at offset zero like others? For now offsets > > > > FixedArray::kLengthOffset should assert, since they need to be KeyedLoads. > > > > Unfortunately we use this variant to construct a direct access to an element > of > > a fixed array in BuildNativeGetContext :( I've renamed this to ForFixedArrayHeader, and only allow this method to be used with offsets within the header, replacing the other uses with HKeyedLoad as suggested. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3738: stream->Add(".@%d", offset_); On 2013/05/13 15:44:32, danno wrote: > I'd prefer the output of the form (it's the way it was before and it was more > JavaScript-esque): > > o.field_name@2345 > > field_name should use "%"-prefixed field names for private (internal) field as > they were defined before your CL, e.g. (%map, %length,etc). Done. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3752: stream->Add("[in-object]"); On 2013/05/13 15:44:32, danno wrote: > Shouldn't you only print this if there is no name, i.e. when name.is_null: > > o.<unknown-in-object> I believe the old code would print [in-object] or [backing-store] in any case. I think this may be useful in the hydrogen output as well. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3757: if (!name_.is_null()) { On 2013/05/13 15:44:32, danno wrote: > Seems like this should be mutually exclusive with everything except kInobject. > An assert to make sure? Currently, yes, but I'd rather not put an assert in for something harmless. Having an extra name in an access might be useful in the future, e.g. to designate "payload" and other special fields. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.cc:3759: stream->Add(*String::cast(*name_)->ToCString()); On 2013/05/13 15:44:32, danno wrote: > See above, please use existing JavaScript-like syntax for printing field names. Done. https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen-instructions... src/hydrogen-instructions.h:5234: Handle<String> name = Handle<String>::null()); On 2013/05/13 15:44:32, danno wrote: > It seems, however, that there is a regression. In lithium-xxx, you no longer > output the name of the field. Can you please fix that before landing this? I've changed lithium to ask the hydrogen's HObjectAccess to print itself. > > On 2013/05/13 11:23:19, titzer wrote: > > On 2013/05/10 15:59:19, danno wrote: > > > Why do you pass names in here and to the other variants below? You never > pass > > > anything but the default value. > > > > I was thinking that I would pass in the name of the field, but you are right; > we > > only use an explicit name in the case of ForField(). > > > > > Also, I think you lose some functionality here. Some fields (like array > > lengths) > > > have pseudo names like %length. Do they still get output in the hydrogen > code? > > > > Yes. I added a PrintTo() method for HObjectAccess that knows what name to > print > > for the various special Portion enums. This means the loads also print names > > correctly now. > https://codereview.chromium.org/14284010/diff/39001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/39001/src/hydrogen.cc#newcode1195 src/hydrogen.cc:1195: IsFastElementsKind(kind) On 2013/05/13 15:44:32, danno wrote: > nit: For readability, please calculate the representation in a separate variable > above. Done.
https://codereview.chromium.org/14284010/diff/50001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/50001/src/hydrogen-instructions... src/hydrogen-instructions.h:5286: unsigned portion_ : 3; Can't you just make this "Portion" instead of unsigned? You need to watch out on windows, because it screws up this construct, sign-extending the top bit, so will need to give the enum 4 bits rather than 3 to ensure that it always ends up a positive value in the enum range. https://codereview.chromium.org/14284010/diff/50001/src/hydrogen-instructions... src/hydrogen-instructions.h:5313: SetGVNFlag(kDependsOnDoubleFields); Yikes! Don't you want to only set this inside SetGVNFlags? The LookupResult should have a representation to know if it's a double so you can make the appropriate decisions inside of ForField, and you can add a new portion for these values. https://codereview.chromium.org/14284010/diff/50001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/14284010/diff/50001/src/hydrogen.cc#newcode1825 src/hydrogen.cc:1825: HLoadKeyed(map_array, kind_index, NULL, FAST_ELEMENTS)); Nice :-)
On 2013/05/14 14:41:58, danno wrote: > https://codereview.chromium.org/14284010/diff/50001/src/hydrogen-instructions.h > File src/hydrogen-instructions.h (right): > > https://codereview.chromium.org/14284010/diff/50001/src/hydrogen-instructions... > src/hydrogen-instructions.h:5286: unsigned portion_ : 3; > Can't you just make this "Portion" instead of unsigned? You need to watch out on > windows, because it screws up this construct, sign-extending the top bit, so > will need to give the enum 4 bits rather than 3 to ensure that it always ends up > a positive value in the enum range. I don't think the compiler will pack the bit fields if it's just in there as Portion. I've made the bitfield 4 bits for retarded Windows. > https://codereview.chromium.org/14284010/diff/50001/src/hydrogen-instructions... > src/hydrogen-instructions.h:5313: SetGVNFlag(kDependsOnDoubleFields); > Yikes! Don't you want to only set this inside SetGVNFlags? The LookupResult > should have a representation to know if it's a double so you can make the > appropriate decisions inside of ForField, and you can add a new portion for > these values. As per in-person discussion, I've removed this GVNFlag in hopes that we can get finer granularity with several offset flags in the future. > > https://codereview.chromium.org/14284010/diff/50001/src/hydrogen.cc > File src/hydrogen.cc (right): > > https://codereview.chromium.org/14284010/diff/50001/src/hydrogen.cc#newcode1825 > src/hydrogen.cc:1825: HLoadKeyed(map_array, kind_index, NULL, FAST_ELEMENTS)); > Nice :-)
https://codereview.chromium.org/14284010/diff/56001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/14284010/diff/56001/src/hydrogen-instructions... src/hydrogen-instructions.h:5267: unsigned portion_ : 4; I think you misinterpreted my comment. With unsigned, you only need 3 bits, but with the enum type, you need 4. I am not sure why you don't think this will work, we do it elsewhere. If you are worried that the compiler won't pack, then please use a single field unsigned bit_field_; and define strongly-type BitFields for both parts. That approach is guaranteed to work.
On 2013/05/15 09:15:30, danno wrote: > https://codereview.chromium.org/14284010/diff/56001/src/hydrogen-instructions.h > File src/hydrogen-instructions.h (right): > > https://codereview.chromium.org/14284010/diff/56001/src/hydrogen-instructions... > src/hydrogen-instructions.h:5267: unsigned portion_ : 4; > I think you misinterpreted my comment. With unsigned, you only need 3 bits, but > with the enum type, you need 4. I am not sure why you don't think this will > work, we do it elsewhere. If you are worried that the compiler won't pack, then > please use a single field unsigned bit_field_; and define strongly-type > BitFields for both parts. That approach is guaranteed to work. I've updated HObjectAccess to use the BitField utility.
lgtm if there are no regressions on the big three benchmarks
Message was sent while issue was closed.
Committed patchset #12 manually as r14791 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
