Array index computation dehoisting. (issue 10382055)

[mma...@chromium.org]

Reviewers: Jakob,

Message:
This should be reviewable: all tests pass and also more complex code
executes
correctly.

Description:
Array index computation dehoisting.

When an array index (in an array access) is a simple "expression +
constant",
just embed the constant in the array access operation so that the full index
expression is (potentially) no longer used and its live range can be much
shorter.
This is effective in conjunction with array bounds check removal (otherwise
the
index is anyway used in the check).


Please review this at https://chromiumcodereview.appspot.com/10382055/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/arm/lithium-arm.h
M src/arm/lithium-arm.cc
M src/arm/lithium-codegen-arm.cc
M src/flag-definitions.h
M src/hydrogen-instructions.h
M src/hydrogen.h
M src/hydrogen.cc
M src/ia32/lithium-codegen-ia32.h
M src/ia32/lithium-codegen-ia32.cc
M src/ia32/lithium-ia32.h
M src/ia32/lithium-ia32.cc
M src/v8-counters.h
M src/x64/lithium-codegen-x64.h
M src/x64/lithium-codegen-x64.cc
M src/x64/lithium-x64.h
M src/x64/lithium-x64.cc

[jkum...@chromium.org]

Looks good overall. I have some suggestions for simplification.


https://chromiumcodereview.appspot.com/10382055/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/flag-definitions.h#newcode200
src/flag-definitions.h:200: DEFINE_bool(array_index_dehoisting, true,
Is there a reason to keep the flag around?

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h#newcode3992
src/hydrogen-instructions.h:3992: uint32_t index_offset_;
Why is this a uint32_t? In hydrogen.cc, you're passing in an int32_t
that indeed can be negative. Please use consistently signed variable
types. (Same below.)

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h#newcode3998
src/hydrogen-instructions.h:3998: HLoadKeyedFastDoubleElement(HValue*
elements, HValue* key) :
nit: the colon goes in the next line (see HLoadKeyedFastElement as an
example)

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3122
src/hydrogen.cc:3122: // FIXME(mmassi): decide meaningful bound
fix this before landing? :-)

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3127
src/hydrogen.cc:3127: counters->array_indexes_removed()->Increment();
Is it useful to keep those counters, or have they served their purpose?

https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3143
src/hydrogen.cc:3143: if (instr->IsLoadKeyedFastElement()) {
This is a lot of duplicate code...

Suggestion:
class ArrayInstructionInterface {
public:
virtual HInstruction* key() = 0;
virtual int32_t key_position { return 1; }
virtual void SetIndexOffset(int32_t index_offset) = 0;
}
Then you can let
H{Load,Store}Keyed{Fast,FastDouble,SpecializedArray}Element inherit that
and make DehoistArrayIndex() a whole lot easier (or even inline it into
its single remaining call site).
The downside would be that those three virtual methods can no longer be
inlined, but I think they're not that performance critical.

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2433
src/ia32/lithium-codegen-ia32.cc:2433: instr->additional_index()));
You can use "instr->hydrogen()->index_offset()" here. (Same below.)

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2458
src/ia32/lithium-codegen-ia32.cc:2458: instr->elements(),
Leaving these on one line is fine, actually. But if you prefer, you can
split them.

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2472
src/ia32/lithium-codegen-ia32.cc:2472: uint32_t additional_index) {
int32_t

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode3485
src/ia32/lithium-codegen-ia32.cc:3485: - kHeapObjectTag,
Nit: this is ugly, please move all parameters to the next line, indented
by 4 spaces.

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.h#newcode246
src/ia32/lithium-codegen-ia32.h:246: uint32_t additional_index = 0);
int32_t please.

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.cc#newcode1964
src/ia32/lithium-ia32.cc:1964: instr->index_offset());
Again, I think you don't need any of the changes to this file.

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.h#newcode1250
src/ia32/lithium-ia32.h:1250: uint32_t additional_index_;
You don't need to pass this value to the LInstruction. Just use the
hydrogen() accessor to fetch it from the associated HInstruction.
I think that obsoletes all of the changes to this file.

https://chromiumcodereview.appspot.com/10382055/

[fschn...@chromium.org]

dbc:

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2481
src/ia32/lithium-codegen-ia32.cc:2481: (constant_value +
additional_index) * (1 << shift_size)
You could just rewrite

expr * (1 << shift_size) into

expr << shift_size

https://chromiumcodereview.appspot.com/10382055/