ARM/ARM64: Optimise HLoadNamedField and HStoreNamedField.

ARM/ARM64: Optimise HLoadNamedField and HStoreNamedField.

The access to the object properties is abstracted in via a separate
HLoadNamedField to be shared across accesses to the same object.

[Alexandre Rames, 2014-06-10 13:28:19]

This does not show a noticeable impact on benchmarks, but the code generated is
cleaner.

[ulan, 2014-06-12 13:39:18]

[+Ben]

Ben, wdyt about the HObjectAccess comment?

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

https://codereview.chromium.org/324093002/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6785: return true;
Could you please port this to other architectures and get rid of this function?

https://codereview.chromium.org/324093002/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6791: return !(access.IsExternalMemory() ||
This predicate should be part of HObjectAccess (if Ben doesn't disagree).

[titzer, 2014-06-13 09:33:29]

On 2014/06/12 13:39:18, ulan wrote:
> [+Ben]
> 
> Ben, wdyt about the HObjectAccess comment?
> 
>
https://codereview.chromium.org/324093002/diff/20001/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/324093002/diff/20001/src/hydrogen-instruction...
> src/hydrogen-instructions.h:6785: return true;
> Could you please port this to other architectures and get rid of this
function?
> 
>
https://codereview.chromium.org/324093002/diff/20001/src/hydrogen-instruction...
> src/hydrogen-instructions.h:6791: return !(access.IsExternalMemory() ||
> This predicate should be part of HObjectAccess (if Ben doesn't disagree).

We tried to go down this route before, but by simplifying all object accesses
into a either two loads or a load and a store. That had the disadvantage of
reducing the accuracy of GVN and load/store elimination.

This approach doesn't that have the disadvantage of reducing the accuracy of
those phases, but it further entangles the whole graph with these extra uses,
that are only used on two backends. That seems like added complexity that I'm
not sure is worth it.

[Alexandre Rames, 2014-06-13 10:08:49]

> We tried to go down this route before, but by simplifying all object accesses
> into a either two loads or a load and a store. That had the disadvantage of
> reducing the accuracy of GVN and load/store elimination.
> 
> This approach doesn't that have the disadvantage of reducing the accuracy of
> those phases, but it further entangles the whole graph with these extra uses,
> that are only used on two backends. That seems like added complexity that I'm
> not sure is worth it.

Maybe the patch could be simplified to have the second load take the newly
introduced first load as its object input rather than as a separate input. If
that works the modifications to the graphs could be acceptable; what do you
think?
We have another similar patch extracting the 'load map' outside of 'compare map'
instructions. Like this patch it doesn't impact the benchmarks, but cleans the
generated code.
The arch-dependent code in hydrogen.h and hydrogen-instructions.h could also be
moved to separate arm64 headers, but I don't think it is preferable.

[titzer, 2014-06-17 09:06:53]

On 2014/06/13 10:08:49, Alexandre Rames wrote:
> > We tried to go down this route before, but by simplifying all object
accesses
> > into a either two loads or a load and a store. That had the disadvantage of
> > reducing the accuracy of GVN and load/store elimination.
> > 
> > This approach doesn't that have the disadvantage of reducing the accuracy of
> > those phases, but it further entangles the whole graph with these extra
uses,
> > that are only used on two backends. That seems like added complexity that
I'm
> > not sure is worth it.
> 
> Maybe the patch could be simplified to have the second load take the newly
> introduced first load as its object input rather than as a separate input. If
> that works the modifications to the graphs could be acceptable; what do you
> think?
> We have another similar patch extracting the 'load map' outside of 'compare
map'
> instructions. Like this patch it doesn't impact the benchmarks, but cleans the
> generated code.

This is overall a good idea, since that allows the loads to be GVN'd together.
However, there will need to be some modifications to check elimination, since
the load of the map gets the set of maps at the point of the load, not at the
point of the compare. That is almost like combining load elimination with check
elimination, but I am not quite sure.

> The arch-dependent code in hydrogen.h and hydrogen-instructions.h could also
be
> moved to separate arm64 headers, but I don't think it is preferable.

[titzer, 2015-11-30 09:13:00]

Description was changed from

==========
ARM/ARM64: Optimise HLoadNamedField and HStoreNamedField.

The access to the object properties is abstracted in via a separate
HLoadNamedField to be shared across accesses to the same object.
==========

to

==========
ARM/ARM64: Optimise HLoadNamedField and HStoreNamedField.

The access to the object properties is abstracted in via a separate
HLoadNamedField to be shared across accesses to the same object.
==========

[titzer, 2015-11-30 09:13:01]

titzer@chromium.org changed reviewers:
- titzer@chromium.org