Improve constant element index access code generation

Improve constant element index access code generation

R=jkummerow@chromium.org

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

[danno, 2012-07-27 14:07:57]

PTAL

[Jakob Kummerow, 2012-07-30 11:58:08]

Overall LGTM, but I have comments on the ARM changes.

http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc...
src/arm/lithium-codegen-arm.cc:2793: if
(instr->hydrogen()->key()->representation().IsTagged()) {
I don't see how this condition can ever be true. HLoadKeyedFastElements forces
Integer32 representation for its |key| input.

http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc...
src/arm/lithium-codegen-arm.cc:2802: Operand(instr->additional_index() <<
kPointerSizeLog2));
What's the benefit of having this addition in generated code here?
instr->additional_index() is a constant; adding it to FixedArray::kHeaderSize at
Hydrogen runtime seems fine to me.

[danno, 2012-07-30 16:11:11]

https://chromiumcodereview.appspot.com/10831049/diff/1/src/arm/lithium-codege...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10831049/diff/1/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:2793: if
(instr->hydrogen()->key()->representation().IsTagged()) {
Actually, this does get triggered. It's scary. The HLoadKeyedFastElement
instruction depends on the BoundsCheck instruction. The representation type of
the bounds check isn't important, but it has to be consistent with the index
input to the load. During the phase that eliminates redundant bounds checks, the
key input to the load is replaced by the input to the boundscheck, which can be
tagged. Adding a comment to this effect.

On 2012/07/30 11:58:08, Jakob wrote:
> I don't see how this condition can ever be true. HLoadKeyedFastElements forces
> Integer32 representation for its |key| input.

https://chromiumcodereview.appspot.com/10831049/diff/1/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:2802: Operand(instr->additional_index() <<
kPointerSizeLog2));
I changed this here and in the DoStoreKeyedFastElement code to be consistent (I
copied it here from there originally to make sure the cases were handled
identically).
On 2012/07/30 11:58:08, Jakob wrote:
> What's the benefit of having this addition in generated code here?
> instr->additional_index() is a constant; adding it to FixedArray::kHeaderSize
at
> Hydrogen runtime seems fine to me.

Done.