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.cc#newcode3633
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.cc#newcode3639
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.cc#newcode3653
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.cc#newcode3738
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.cc#newcode3752
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.cc#newcode3757
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.cc#newcode3759
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.h#newcode5234
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/
--
--
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.
