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);
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.cc#newcode3738
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.cc#newcode3752
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.cc#newcode3757
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.cc#newcode3759
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.h#newcode5234
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/
--
--
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.
