Move representation into HObjectAccess and remove from HLoadNamedField and HStoreNamedField.

Move representation into HObjectAccess and remove from HLoadNamedField and HStoreNamedField.

BUG=
R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15875

[danno, 2013-07-05 12:23:37]

Im not sure about this. Isn't there something else we need, like "Format" that
implies a specific, more generic representation? When we load a byte or short
from a buffer, the representation that is output is the same (the compiler only
has int32), but the format in memory is different and needs to be handled by the
load/store differently.

[titzer, 2013-07-08 08:13:21]

On 2013/07/05 12:23:37, danno wrote:
> Im not sure about this. Isn't there something else we need, like "Format" that
> implies a specific, more generic representation? When we load a byte or short
> from a buffer, the representation that is output is the same (the compiler
only
> has int32), but the format in memory is different and needs to be handled by
the
> load/store differently.

We need to add representations for signed and unsigned bytes and shorts, and
conversions between them. Then we can pattern match at the lithium generation of
loads and stores and elide the sign/zero extensions of those representations.
This CL is a first step to getting the representation directly into the access.

[danno, 2013-07-08 11:12:25]

https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5308: Representation::Smi() :
Representation::Tagged());
You should probably gate using ::Smi() here and below with the FLAG_track_fields
flag.

https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5314: Representation::Smi() :
Representation::Tagged());
It's _always_ ::Smi() for FixedArrays, no need to pass in is_fast_elements

https://codereview.chromium.org/18503006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/18503006/diff/1/src/hydrogen.cc#newcode1361
src/hydrogen.cc:1361:
HObjectAccess::ForFixedArrayLength(IsFastElementsKind(kind)), capacity);
This is always Smi()

[titzer, 2013-07-09 15:22:34]

On 2013/07/08 11:12:25, danno wrote:
> https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h#n...
> src/hydrogen-instructions.h:5308: Representation::Smi() :
> Representation::Tagged());
> You should probably gate using ::Smi() here and below with the
FLAG_track_fields
> flag.
> 

Done.

>
https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h#n...
> src/hydrogen-instructions.h:5314: Representation::Smi() :
> Representation::Tagged());
> It's _always_ ::Smi() for FixedArrays, no need to pass in is_fast_elements
> 

Done.

> https://codereview.chromium.org/18503006/diff/1/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
> https://codereview.chromium.org/18503006/diff/1/src/hydrogen.cc#newcode1361
> src/hydrogen.cc:1361:
> HObjectAccess::ForFixedArrayLength(IsFastElementsKind(kind)), capacity);
> This is always Smi()

Done.

[titzer, 2013-07-15 13:47:31]

On 2013/07/09 15:22:34, titzer wrote:
> On 2013/07/08 11:12:25, danno wrote:
> > https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h
> > File src/hydrogen-instructions.h (right):
> > 
> >
>
https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h#n...
> > src/hydrogen-instructions.h:5308: Representation::Smi() :
> > Representation::Tagged());
> > You should probably gate using ::Smi() here and below with the
> FLAG_track_fields
> > flag.
> > 
> 
> Done.
> 
> >
>
https://codereview.chromium.org/18503006/diff/1/src/hydrogen-instructions.h#n...
> > src/hydrogen-instructions.h:5314: Representation::Smi() :
> > Representation::Tagged());
> > It's _always_ ::Smi() for FixedArrays, no need to pass in is_fast_elements
> > 
> 
> Done.
> 
> > https://codereview.chromium.org/18503006/diff/1/src/hydrogen.cc
> > File src/hydrogen.cc (right):
> > 
> > https://codereview.chromium.org/18503006/diff/1/src/hydrogen.cc#newcode1361
> > src/hydrogen.cc:1361:
> > HObjectAccess::ForFixedArrayLength(IsFastElementsKind(kind)), capacity);
> > This is always Smi()
> 
> Done.

Ping

[danno, 2013-07-16 11:51:57]

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5802: = Representation::Tagged())
nit: start the first parameter "obj" on a new line, that prevents the funky
indentation.

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5342: inline HObjectAccess
WithRepresentation(Representation representation) {
Ouch. I really wish we didn't need this version. If I understand correctly, this
is currently only this way because of the implicit unboxing of fields in
HeapNumbers and for polymorphic access. Is there a way to make if more specific,
e.g. ForHeapNumberBox() without the extra representation parameter? And do we
really need it for polymorphic access? You can always use the access of the most
generic type.

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5355: static HObjectAccess ForArrayLength(bool
is_fast_elements = false) {
Can you pass in the ElementsKind here? The boolean parameter doesn't give much
context,

[titzer, 2013-07-23 15:04:09]

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5802: = Representation::Tagged())
On 2013/07/16 11:51:58, danno wrote:
> nit: start the first parameter "obj" on a new line, that prevents the funky
> indentation.

Because I removed this parameter, the funky indentation is now gone.

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5342: inline HObjectAccess
WithRepresentation(Representation representation) {
On 2013/07/16 11:51:58, danno wrote:
> Ouch. I really wish we didn't need this version. If I understand correctly,
this
> is currently only this way because of the implicit unboxing of fields in
> HeapNumbers and for polymorphic access. Is there a way to make if more
specific,
> e.g. ForHeapNumberBox() without the extra representation parameter? And do we
> really need it for polymorphic access? You can always use the access of the
most
> generic type.

Not really, because Representation::generalize() can return either one of the
inputs or Representation::Tagged().

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5355: static HObjectAccess ForArrayLength(bool
is_fast_elements = false) {
On 2013/07/16 11:51:58, danno wrote:
> Can you pass in the ElementsKind here? The boolean parameter doesn't give much
> context,

What would be the default elements kind? We have 3 kinds of uses: ones where we
don't know (assume tagged), ones where we know (assume smi), and ones where we
have an elements kind.

[danno, 2013-07-23 16:15:43]

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5355: static HObjectAccess ForArrayLength(bool
is_fast_elements = false) {
I would prefer that you use ElementsKind and not have a default at all, the
information about which elements you have is always available.  See below.
On 2013/07/23 15:04:10, titzer wrote:
> On 2013/07/16 11:51:58, danno wrote:
> > Can you pass in the ElementsKind here? The boolean parameter doesn't give
much
> > context,
> 
> What would be the default elements kind? We have 3 kinds of uses: ones where
we
> don't know (assume tagged), ones where we know (assume smi), and ones where we
> have an elements kind.

https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode1095
src/hydrogen.cc:1095: AddStore(object,
HObjectAccess::ForArrayLength(IsFastElementsKind(kind)),
Here just pass 'kind'

https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode1164
src/hydrogen.cc:1164: ? AddLoad(object, HObjectAccess::ForArrayLength(true),
NULL)
Here just pass from_kind

https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode5406
src/hydrogen.cc:5406: HObjectAccess::ForArrayLength());
Here it's map->elements_kind()

https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode5658
src/hydrogen.cc:5658: object, HObjectAccess::ForArrayLength(true), mapcompare);
And here it's also map->elements_kind()

[titzer, 2013-07-23 16:47:54]

On 2013/07/23 16:15:43, danno wrote:
> https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/18503006/diff/8001/src/hydrogen-instructions....
> src/hydrogen-instructions.h:5355: static HObjectAccess ForArrayLength(bool
> is_fast_elements = false) {
> I would prefer that you use ElementsKind and not have a default at all, the
> information about which elements you have is always available.  See below.
> On 2013/07/23 15:04:10, titzer wrote:
> > On 2013/07/16 11:51:58, danno wrote:
> > > Can you pass in the ElementsKind here? The boolean parameter doesn't give
> much
> > > context,
> > 
> > What would be the default elements kind? We have 3 kinds of uses: ones where
> we
> > don't know (assume tagged), ones where we know (assume smi), and ones where
we
> > have an elements kind.
> 
> https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode1095
> src/hydrogen.cc:1095: AddStore(object,
> HObjectAccess::ForArrayLength(IsFastElementsKind(kind)),
> Here just pass 'kind'
> 

Done.

>
https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode1164
> src/hydrogen.cc:1164: ? AddLoad(object, HObjectAccess::ForArrayLength(true),
> NULL)
> Here just pass from_kind
>

Done.
 
>
https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode5406
> src/hydrogen.cc:5406: HObjectAccess::ForArrayLength());
> Here it's map->elements_kind()
> 

Done.

>
https://codereview.chromium.org/18503006/diff/15001/src/hydrogen.cc#newcode5658
> src/hydrogen.cc:5658: object, HObjectAccess::ForArrayLength(true),
mapcompare);
> And here it's also map->elements_kind()

Done.

[danno, 2013-07-24 13:13:15]

lgtm

[titzer, 2013-07-25 09:37:59]

Committed patchset #4 manually as r15875 (presubmit successful).