Avoid using a register for constant external array indices.

Avoid using a register for constant external array indices.

This CL is based on and obsoletes CL 6879037.

TEST=mjsunit/external-array.js

Committed: http://code.google.com/p/v8/source/detail?r=7844

[Jakob Kummerow, 2011-04-28 15:16:37]

Finally... this has taken way longer than expected. 

PTAL.

[danno, 2011-04-28 17:59:55]

LGTM

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

http://codereview.chromium.org/6902112/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2495: key_is_constant ? Operand(constant_key * 4)
: Operand(key, LSL, 2));
Can you break out the Operand into another line to avoid the line break, like
you did in the ia32 and x64 versions?

http://codereview.chromium.org/6902112/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:2502: key_is_constant ? Operand(constant_key * 8)
: Operand(key, LSL, 3));
Same here and below?

http://codereview.chromium.org/6902112/diff/1/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/6902112/diff/1/test/mjsunit/external-array.js#...
test/mjsunit/external-array.js:127: gc();  // Makes V8 forget about type
information for test_func.
I'm not sure you need this, since you call with different functions every time.
Since you don't rely on it anywhere, AFAICT, it's a bit fragile, is it really
needed?

[fschneider, 2011-04-29 09:13:47]

Drive-by style comment:

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

http://codereview.chromium.org/6902112/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2424: if (key_is_constant)
Same comment as for x64 here  - { } needed.

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

http://codereview.chromium.org/6902112/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2389: if (key_is_constant)
Please always add { } for multiline if-statements.

Here you can move the returns into the if-statment above.

[Jakob Kummerow, 2011-04-29 10:04:16]

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

http://codereview.chromium.org/6902112/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2424: if (key_is_constant)
On 2011/04/29 09:13:47, fschneider wrote:
> Same comment as for x64 here  - { } needed.

Done.

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

http://codereview.chromium.org/6902112/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2389: if (key_is_constant)
On 2011/04/29 09:13:47, fschneider wrote:
> Please always add { } for multiline if-statements.

OK, I'll remember that.

> Here you can move the returns into the if-statment above.

Good idea, that makes the whole thing easier. Done.

http://codereview.chromium.org/6902112/diff/1/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/6902112/diff/1/test/mjsunit/external-array.js#...
test/mjsunit/external-array.js:127: gc();  // Makes V8 forget about type
information for test_func.
On 2011/04/28 17:59:55, danno wrote:
> I'm not sure you need this, since you call with different functions every
time.
> Since you don't rely on it anywhere, AFAICT, it's a bit fragile, is it really
> needed?

Yes, it is needed, because for each given test_func, run_test() is called once
for each external array type. If gc() isn't called explicitly, leftover type
info makes V8 think test_func is megamorphic on subsequent runs, causing deopts
immediately after the forced optimization.

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

http://codereview.chromium.org/6902112/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2494: if (key_is_constant) {
On 2011/04/28 17:59:55, danno wrote:
> Can you break out the Operand into another line to avoid the line break, like
> you did in the ia32 and x64 versions?

Sure I can. Like this? 
(I don't really like duplicating the instruction code, but it's the second-most
concise way to do this.)

http://codereview.chromium.org/6902112/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2504: Operand operand(key, LSL, 3);
Or like this?
(Empty initialization isn't possible, so I have to initialize for one case and
then overwrite for the other if necessary.)

http://codereview.chromium.org/6902112/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2513: case kExternalByteArray: {
Or maybe like this?
(Doesn't avoid the multiline x?y:z construct, only moves it. Also, block scoping
with {} is necessary in this case.)

http://codereview.chromium.org/6902112/diff/7001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2522: __ ldrb(result, key_is_constant
Or, on second thought, maybe stick with my original approach?

I can't quite decide which of these four solutions I find least ugly, so I went
for the most concise ugliness originally :-/

[Jakob Kummerow, 2011-05-09 11:02:57]

As discussed offline, I factored out the mapping from ExternalArrayType to a
shift size (i.e. power of two of the corresponding element size in bytes) into a
separate function, and made use of it to clean up the arch-specific code.

Danno: happy with this?

Note that some of the delta to the previous patch set stems from rebasing onto
current master.

[danno, 2011-05-10 09:48:54]

LGTM