Add pixel array handling in keyed IC's for x64 version.

Add pixel array handling in keyed IC's for x64 version.

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

[Lasse Reichstein, 2009-09-30 08:43:02]

LGTM, with suggestions.

http://codereview.chromium.org/251041/diff/1/2
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/251041/diff/1/2#newcode307
Line 307: // ecx: elements array
ecx -> rcx

http://codereview.chromium.org/251041/diff/1/2#newcode309
Line 309: __ Cmp(FieldOperand(rcx, HeapObject::kMapOffset),
Factory::pixel_array_map());
Get the pixel_array_map from the root array instead of from Factory.
Ditto for fixed_array_map above (even if it's not your code :).

http://codereview.chromium.org/251041/diff/1/2#newcode451
Line 451: __ Cmp(FieldOperand(rcx, HeapObject::kMapOffset),
Factory::fixed_array_map());
Use root array here too.

http://codereview.chromium.org/251041/diff/1/2#newcode476
Line 476: __ Cmp(FieldOperand(rcx, HeapObject::kMapOffset),
Factory::pixel_array_map());
Use root array.

http://codereview.chromium.org/251041/diff/1/2#newcode488
Line 488: __ cmpl(rax, Immediate(0));
Use testl(rax, rax) to check sign instead of comparing with a literal.

http://codereview.chromium.org/251041/diff/1/2#newcode490
Line 490: __ movq(rax, Immediate(0));
use xorl(rax, rax) to clear rax instead of using a literal.

http://codereview.chromium.org/251041/diff/1/2#newcode495
Line 495: __ movq(rax, Immediate(255));
Use movl instead of movq (to save a byte).

http://codereview.chromium.org/251041/diff/1/2#newcode496
Line 496: __ bind(&done);
Generally, for this check, it always does at least two checks, even when it's in
range. How about checking for being in the 0.255 range first, and only doing a
second check if outside. E.g.,:

 xor_(kScratchRegister, kScratchRegister);
 movb(kScratchRegister, rax);
 xorl(kScratchRegister, rax);
 j(zero, &done);
 j(negative, &is_negative);
 movl(rax, Immediate(255));
 jmp(&done);
 bind(&is_negative);
 xorl(rax, rax);
 bind(&done);

[Søren Thygesen Gjesse, 2009-09-30 12:01:33]

http://codereview.chromium.org/251041/diff/1/2
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/251041/diff/1/2#newcode307
Line 307: // ecx: elements array
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> ecx -> rcx

Done.

http://codereview.chromium.org/251041/diff/1/2#newcode309
Line 309: __ Cmp(FieldOperand(rcx, HeapObject::kMapOffset),
Factory::pixel_array_map());
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> Get the pixel_array_map from the root array instead of from Factory.
> Ditto for fixed_array_map above (even if it's not your code :).
> 

Done, added CompareRoot taking an Operand to the macro assembler.

http://codereview.chromium.org/251041/diff/1/2#newcode451
Line 451: __ Cmp(FieldOperand(rcx, HeapObject::kMapOffset),
Factory::fixed_array_map());
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> Use root array here too.

Done.

http://codereview.chromium.org/251041/diff/1/2#newcode476
Line 476: __ Cmp(FieldOperand(rcx, HeapObject::kMapOffset),
Factory::pixel_array_map());
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> Use root array.
> 

Done.

http://codereview.chromium.org/251041/diff/1/2#newcode488
Line 488: __ cmpl(rax, Immediate(0));
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> Use testl(rax, rax) to check sign instead of comparing with a literal.

Done.

http://codereview.chromium.org/251041/diff/1/2#newcode490
Line 490: __ movq(rax, Immediate(0));
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> use xorl(rax, rax) to clear rax instead of using a literal.

Done.

http://codereview.chromium.org/251041/diff/1/2#newcode495
Line 495: __ movq(rax, Immediate(255));
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> Use movl instead of movq (to save a byte).

Done.

http://codereview.chromium.org/251041/diff/1/2#newcode496
Line 496: __ bind(&done);
On 2009/09/30 08:43:02, Lasse Reichstein wrote:
> Generally, for this check, it always does at least two checks, even when it's
in
> range. How about checking for being in the 0.255 range first, and only doing a
> second check if outside. E.g.,:
> 
>  xor_(kScratchRegister, kScratchRegister);
>  movb(kScratchRegister, rax);
>  xorl(kScratchRegister, rax);
>  j(zero, &done);
>  j(negative, &is_negative);
>  movl(rax, Immediate(255));
>  jmp(&done);
>  bind(&is_negative);
>  xorl(rax, rax);
>  bind(&done);

Used this scheme with andl(rax, 0xffffff00) to check for 0--255 range.

Also applied this scheme to the IA32 version.

[Søren Thygesen Gjesse, 2009-09-30 12:05:39]

Please take another look.

[Lasse Reichstein, 2009-09-30 16:50:40]

LGTM.

http://codereview.chromium.org/251041/diff/3001/1003
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/251041/diff/3001/1003#newcode494
Line 494: __ j(negative, &is_negative);
A thought: It might be faster to do the out-of-range part using setcc:
  // __ xor_(rax, rax);  // maybe clear rax first, to avoid unnecessary
dependencies
  __ setcc(negative, rax);  // 1 if negative, 0 if positive.
  __ subb(rax, Immediate(1));  // 0 if negative, 255 if positive.
(can use subl too if we cleared rax first).

[Søren Thygesen Gjesse, 2009-09-30 20:27:05]

http://codereview.chromium.org/251041/diff/3001/1003
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/251041/diff/3001/1003#newcode494
Line 494: __ j(negative, &is_negative);
On 2009/09/30 16:50:40, Lasse Reichstein wrote:
> A thought: It might be faster to do the out-of-range part using setcc:
>   // __ xor_(rax, rax);  // maybe clear rax first, to avoid unnecessary
> dependencies
>   __ setcc(negative, rax);  // 1 if negative, 0 if positive.
>   __ subb(rax, Immediate(1));  // 0 if negative, 255 if positive.
> (can use subl too if we cleared rax first).

Nice jump-free clamping. I will try to see if it has a mesurable performance
difference and create a new changelist if it has. The clamping should not be the
common case, but then again who knows what code is out there.