Implement local load/store elimination on basic blocks.

Implement local load/store elimination on basic blocks.

BUG=
R=mstarzinger@chromium.org

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

[Michael Starzinger, 2013-09-12 20:04:25]

Looking good. First round of comments, just one real issue about stores with map
transitions. But this needs dedicated test coverage in an
mjsunit/compiler/load-store-elimination.js file.

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc
File src/hydrogen-load-elimination.cc (right):

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:39: class HFieldApprox : public ZoneObject {
nit: Can we use the full name HFieldApproximation?

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:44: HFieldApprox *next_;
nit: Asterisk sticks to the left.

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:51: class HLoadElimTable {
nit: Can we use the full name HLoadEliminationTable? Also can we mark the table
as BASE_EMBEDDED?

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:109: void Kill(HStoreNamedField* store) {
This method is never called, let's drop it.

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:218: Zone* zone_;
nit: Make the field private. Maybe even make the internal methods like FieldOf()
and KillFieldInternal() private.

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:249: case HValue::kLoadNamedField: {
style: Indent cases by one level.

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.cc:271: HValue* result = table.store(store);
An HStoreNamedField with a transition also changes what is stored in field 0,
because it performs an implicit map change. This must be reflected in the table
accordingly.

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.h
File src/hydrogen-load-elimination.h (right):

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.h:28: #ifndef V8_LOAD_ELIMINATION_H_
nit: s/V8_LOAD_ELIMINATION_H_/V8_HYDROGEN_LOAD_ELIMINATION_H_/

https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
src/hydrogen-load-elimination.h:42: void EliminateLoads(HBasicBlock* block);
nit: Can we make EliminateLoads() private?

[titzer, 2013-09-16 17:17:39]

On 2013/09/12 20:04:25, Michael Starzinger wrote:
> Looking good. First round of comments, just one real issue about stores with
map
> transitions. But this needs dedicated test coverage in an
> mjsunit/compiler/load-store-elimination.js file.
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc
> File src/hydrogen-load-elimination.cc (right):
> 
>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:39: class HFieldApprox : public ZoneObject {
> nit: Can we use the full name HFieldApproximation?
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:44: HFieldApprox *next_;
> nit: Asterisk sticks to the left.
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:51: class HLoadElimTable {
> nit: Can we use the full name HLoadEliminationTable? Also can we mark the
table
> as BASE_EMBEDDED?
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:109: void Kill(HStoreNamedField* store) {
> This method is never called, let's drop it.
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:218: Zone* zone_;
> nit: Make the field private. Maybe even make the internal methods like
FieldOf()
> and KillFieldInternal() private.
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:249: case HValue::kLoadNamedField: {
> style: Indent cases by one level.
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.cc:271: HValue* result = table.store(store);
> An HStoreNamedField with a transition also changes what is stored in field 0,
> because it performs an implicit map change. This must be reflected in the
table
> accordingly.
> 

Done. I've also improved the heuristic for telling when the stored value is the
same (using HValue::Equals()).

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.h
> File src/hydrogen-load-elimination.h (right):
> 
>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.h:28: #ifndef V8_LOAD_ELIMINATION_H_
> nit: s/V8_LOAD_ELIMINATION_H_/V8_HYDROGEN_LOAD_ELIMINATION_H_/
> 

Done.

>
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination...
> src/hydrogen-load-elimination.h:42: void EliminateLoads(HBasicBlock* block);
> nit: Can we make EliminateLoads() private?

Done.

[Michael Starzinger, 2013-09-17 13:12:42]

LGTM.

[titzer, 2013-09-17 15:32:34]

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