Port of optimized ICs for external and pixel arrays from ia32 to ARM.

Port of optimized ICs for external and pixel arrays from ia32 to ARM.


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

[Mads Ager (chromium), 2010-03-16 07:15:58]

Erik,

ARM stuff inside.  :)  Could you review this one?

Thanks,    -- Mads

[Erik Corry, 2010-03-16 11:04:56]

http://codereview.chromium.org/993002/diff/1/7
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/7#newcode1330
src/arm/assembler-arm.cc:1330: ASSERT(offset % 4 == 0);
Here and in the function above we should assert that the offset is not out of
range.

http://codereview.chromium.org/993002/diff/1/7#newcode1407
src/arm/assembler-arm.cc:1407: 
There should be 2 blank lines here.

http://codereview.chromium.org/993002/diff/1/7#newcode1411
src/arm/assembler-arm.cc:1411: 
And here.

http://codereview.chromium.org/993002/diff/1/7#newcode1412
src/arm/assembler-arm.cc:1412: static bool is_signed(VFPType type) {
Function names are CapitalCamelCase.  This goes for all of them.

http://codereview.chromium.org/993002/diff/1/7#newcode1423
src/arm/assembler-arm.cc:1423: 
Two lines here.

http://codereview.chromium.org/993002/diff/1/7#newcode1454
src/arm/assembler-arm.cc:1454: int *Vm,
Wrong placement of * and wrong capitalization of parameter name.

http://codereview.chromium.org/993002/diff/1/7#newcode1468
src/arm/assembler-arm.cc:1468: const int dst_code,
Wrong indentation.

http://codereview.chromium.org/993002/diff/1/6
File src/arm/constants-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/6#newcode88
src/arm/constants-arm.cc:88: 
2 blank lines please.

http://codereview.chromium.org/993002/diff/1/6#newcode89
src/arm/constants-arm.cc:89: int VFPRegisters::Number(const char* name, bool
*is_double) {
Asterisk placement.

http://codereview.chromium.org/993002/diff/1/10
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/10#newcode934
src/arm/disasm-arm.cc:934: if (instr->Bit(23) == 1) {
This is getting very messy (not your fault) and I would like it cleaned up.  See
Table A7-24.

http://codereview.chromium.org/993002/diff/1/11
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/11#newcode644
src/arm/ic-arm.cc:644: // This functions would not work correctly for 0.
functions would -> function does

http://codereview.chromium.org/993002/diff/1/11#newcode650
src/arm/ic-arm.cc:650: const int meaningfull_bits
meaningfull -> meaningful
Please don't split over two lines if it fits on one.

http://codereview.chromium.org/993002/diff/1/11#newcode654
src/arm/ic-arm.cc:654: = meaningfull_bits - HeapNumber::kMantissaBitsInTopWord;
= should go on the line before (also 2 other places).

http://codereview.chromium.org/993002/diff/1/11#newcode722
src/arm/ic-arm.cc:722: __ mov(r0, Operand(r0, ASR, kSmiTagSize));  // Untag key.
You don't need to untag this.  Instead you should assert that the kSmiTag is 0
and that the kSmiTagSize is 1, then you should just leave r0 tagged.

http://codereview.chromium.org/993002/diff/1/11#newcode789
src/arm/ic-arm.cc:789: __ vcvt_f64_s32(d0, s0);
We can do vstr here.

http://codereview.chromium.org/993002/diff/1/11#newcode866
src/arm/ic-arm.cc:866: // VFP is not available do manual single to double
conversion.
available do -> available, do

http://codereview.chromium.org/993002/diff/1/11#newcode1058
src/arm/ic-arm.cc:1058: __ BranchOnNotSmi(r0, &slow);
You might use GetInt32 to do this without going into the runtime system (a
different change probably).

http://codereview.chromium.org/993002/diff/1/11#newcode1144
src/arm/ic-arm.cc:1144: static void ConvertIntToFloat(MacroAssembler* masm,
I ran out of time here.

[Vyacheslav Egorov (Chromium), 2010-03-18 10:27:52]

http://codereview.chromium.org/993002/diff/1/7
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/7#newcode1330
src/arm/assembler-arm.cc:1330: ASSERT(offset % 4 == 0);
On 2010/03/16 11:04:56, Erik Corry wrote:
> Here and in the function above we should assert that the offset is not out of
> range.

Done.

http://codereview.chromium.org/993002/diff/1/7#newcode1407
src/arm/assembler-arm.cc:1407: 
On 2010/03/16 11:04:56, Erik Corry wrote:
> There should be 2 blank lines here.

Some style issues like this were already fixed in patch set #3, but it seems you
already started review when I uploaded it.

http://codereview.chromium.org/993002/diff/1/7#newcode1411
src/arm/assembler-arm.cc:1411: 
On 2010/03/16 11:04:56, Erik Corry wrote:
> And here.

Done.

http://codereview.chromium.org/993002/diff/1/7#newcode1412
src/arm/assembler-arm.cc:1412: static bool is_signed(VFPType type) {
On 2010/03/16 11:04:56, Erik Corry wrote:
> Function names are CapitalCamelCase.  This goes for all of them.

Done.

http://codereview.chromium.org/993002/diff/1/7#newcode1423
src/arm/assembler-arm.cc:1423: 
On 2010/03/16 11:04:56, Erik Corry wrote:
> Two lines here.

Done.

http://codereview.chromium.org/993002/diff/1/7#newcode1454
src/arm/assembler-arm.cc:1454: int *Vm,
On 2010/03/16 11:04:56, Erik Corry wrote:
> Wrong placement of * and wrong capitalization of parameter name.

Done.

http://codereview.chromium.org/993002/diff/1/7#newcode1468
src/arm/assembler-arm.cc:1468: const int dst_code,
On 2010/03/16 11:04:56, Erik Corry wrote:
> Wrong indentation.

Done.

http://codereview.chromium.org/993002/diff/1/6
File src/arm/constants-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/6#newcode88
src/arm/constants-arm.cc:88: 
On 2010/03/16 11:04:56, Erik Corry wrote:
> 2 blank lines please.

Done.

http://codereview.chromium.org/993002/diff/1/6#newcode88
src/arm/constants-arm.cc:88: 
On 2010/03/16 11:04:56, Erik Corry wrote:
> 2 blank lines please.

Done.

http://codereview.chromium.org/993002/diff/1/6#newcode89
src/arm/constants-arm.cc:89: int VFPRegisters::Number(const char* name, bool
*is_double) {
On 2010/03/16 11:04:56, Erik Corry wrote:
> Asterisk placement.

Done.

http://codereview.chromium.org/993002/diff/1/6#newcode89
src/arm/constants-arm.cc:89: int VFPRegisters::Number(const char* name, bool
*is_double) {
On 2010/03/16 11:04:56, Erik Corry wrote:
> Asterisk placement.

Done.

http://codereview.chromium.org/993002/diff/1/10
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/10#newcode934
src/arm/disasm-arm.cc:934: if (instr->Bit(23) == 1) {
On 2010/03/16 11:04:56, Erik Corry wrote:
> This is getting very messy (not your fault) and I would like it cleaned up. 
See
> Table A7-24.

Done.

http://codereview.chromium.org/993002/diff/1/11
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/993002/diff/1/11#newcode644
src/arm/ic-arm.cc:644: // This functions would not work correctly for 0.
On 2010/03/16 11:04:56, Erik Corry wrote:
> functions would -> function does

Done.

http://codereview.chromium.org/993002/diff/1/11#newcode650
src/arm/ic-arm.cc:650: const int meaningfull_bits
On 2010/03/16 11:04:56, Erik Corry wrote:
> meaningfull -> meaningful
> Please don't split over two lines if it fits on one.

Done.

http://codereview.chromium.org/993002/diff/1/11#newcode654
src/arm/ic-arm.cc:654: = meaningfull_bits - HeapNumber::kMantissaBitsInTopWord;
On 2010/03/16 11:04:56, Erik Corry wrote:
> = should go on the line before (also 2 other places).

Done.

http://codereview.chromium.org/993002/diff/1/11#newcode722
src/arm/ic-arm.cc:722: __ mov(r0, Operand(r0, ASR, kSmiTagSize));  // Untag key.
On 2010/03/16 11:04:56, Erik Corry wrote:
> You don't need to untag this.  Instead you should assert that the kSmiTag is 0
> and that the kSmiTagSize is 1, then you should just leave r0 tagged.

Done.

http://codereview.chromium.org/993002/diff/1/11#newcode789
src/arm/ic-arm.cc:789: __ vcvt_f64_s32(d0, s0);
On 2010/03/16 11:04:56, Erik Corry wrote:
> We can do vstr here.

Done.

http://codereview.chromium.org/993002/diff/1/11#newcode866
src/arm/ic-arm.cc:866: // VFP is not available do manual single to double
conversion.
On 2010/03/16 11:04:56, Erik Corry wrote:
> available do -> available, do

Done.

http://codereview.chromium.org/993002/diff/1/11#newcode1058
src/arm/ic-arm.cc:1058: __ BranchOnNotSmi(r0, &slow);
I tried this, but it does not pass tests. It seems that rounding mode for pixel
arrays is round towards nearest, not round towards zero. 

On 2010/03/16 11:04:56, Erik Corry wrote:
> You might use GetInt32 to do this without going into the runtime system (a
> different change probably).

[Mads Ager (chromium), 2010-03-23 11:46:54]

LGTM!

http://codereview.chromium.org/993002/diff/49001/50003
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/993002/diff/49001/50003#newcode4753
src/arm/codegen-arm.cc:4753: // We have -1, 0 or 1, which we treat specially.
Could we clearify the comment to state that source contains a positive number
(and therefore the removal of the compare to zero is correct).

http://codereview.chromium.org/993002/diff/49001/50004
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/993002/diff/49001/50004#newcode680
src/arm/codegen-arm.h:680: // Minor key encoding in 16 bits.
This is not new code, but we do not seem to use these bit fields.  Could you
change this to follow the pattern used for the other stubs.  Define BitFields
here and create the MinorKey using bitwise or of the bitfield encodings?

http://codereview.chromium.org/993002/diff/49001/50004#newcode687
src/arm/codegen-arm.h:687: return  the_int_.code() +
Please remove extra space.

http://codereview.chromium.org/993002/diff/49001/50010
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/993002/diff/49001/50010#newcode650
src/arm/ic-arm.cc:650: const int meaningfull_bits = kBitsPerInt - leading_zeroes
- 1;
meaningfull -> meaningful

http://codereview.chromium.org/993002/diff/49001/50010#newcode697
src/arm/ic-arm.cc:697: // Check that the object is a JS object. Load map into r2
End comment with period.

http://codereview.chromium.org/993002/diff/49001/50010#newcode719
src/arm/ic-arm.cc:719: 
Remove empty line?

http://codereview.chromium.org/993002/diff/49001/50010#newcode797
src/arm/ic-arm.cc:797: __ Jump(stub.GetCode(), RelocInfo::CODE_TARGET);
Please use "__ TailCallStub(&stub);"