Some assembler-level optimizations on ARM.

Some assembler-level optimizations on ARM.

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

[Yang, 2012-01-27 09:31:36]

PTAL.

Common patterns:
- JumpIfSmi and SmiUntag can be combined, leveraging the carry flag (shifter
carry).
- Moving loads ahead, further from usage of the value, if readability does not
suffer.
- Merge branching using conditional instructions.
- Use CompareRoot instead of LoadRoot and cmp (not an actual optimization, just
easier to read).

[ulan, 2012-01-27 13:19:11]

LGTM if comments are addressed.

http://codereview.chromium.org/9223011/diff/1/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/9223011/diff/1/src/arm/builtins-arm.cc#newcode157
src/arm/builtins-arm.cc:157: __ LoadRoot(scratch3,
Heap::kFixedArrayMapRootIndex);
In this case it seems to reduce readability. I think such optimizations should
be done on hot paths only.

http://codereview.chromium.org/9223011/diff/1/src/arm/builtins-arm.cc#newcode258
src/arm/builtins-arm.cc:258: __ LoadRoot(scratch1,
Heap::kFixedArrayMapRootIndex);
Ditto.

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:587: STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag == 0);
Can we introduce UntagAndJumpIfSmi for these tree lines?

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:650: STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag == 0);
UntagAndJumpIfSmi

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:846: // If the object is a smi, return it in
destination register, untagged.
UntagAndJumpIfSmi

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3477: __ mov(scratch, Operand(base, ASR, kSmiTagSize),
SetCC);
UntagAndJumpIfSmi

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3493: __ mov(scratch, Operand(exponent, ASR,
kSmiTagSize), SetCC);
UntagAndJumpIfSmi

http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:3503: __ mov(scratch, Operand(exponent, ASR,
kSmiTagSize), SetCC);
UntagAndJumpIfSmi

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

http://codereview.chromium.org/9223011/diff/1/src/arm/codegen-arm.cc#newcode158
src/arm/codegen-arm.cc:158: STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag == 0);
Can we introduce UntagAndJumpIfNotSmi for these tree lines?

http://codereview.chromium.org/9223011/diff/1/src/arm/codegen-arm.cc#newcode344
src/arm/codegen-arm.cc:344: __ b(ne, call_runtime);  // This branch refers to
the previous CompareRoot.
This branch should be above the comment in line 343.

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

http://codereview.chromium.org/9223011/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:3931: STATIC_CHECK(kSmiTagSize == 1 && kSmiTag ==
0);
UntagAndJumpIfSmi

[Yang, 2012-01-27 15:00:34]

Please take another look.

On 2012/01/27 13:19:11, ulan wrote:
> LGTM if comments are addressed.
> 
> http://codereview.chromium.org/9223011/diff/1/src/arm/builtins-arm.cc
> File src/arm/builtins-arm.cc (right):
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/builtins-arm.cc#newcode157
> src/arm/builtins-arm.cc:157: __ LoadRoot(scratch3,
> Heap::kFixedArrayMapRootIndex);
> In this case it seems to reduce readability. I think such optimizations should
> be done on hot paths only.
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/builtins-arm.cc#newcode258
> src/arm/builtins-arm.cc:258: __ LoadRoot(scratch1,
> Heap::kFixedArrayMapRootIndex);
> Ditto.
> 
> http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:587: STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag ==
0);
> Can we introduce UntagAndJumpIfSmi for these tree lines?
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:650: STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag ==
0);
> UntagAndJumpIfSmi
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:846: // If the object is a smi, return it in
> destination register, untagged.
> UntagAndJumpIfSmi
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:3477: __ mov(scratch, Operand(base, ASR,
kSmiTagSize),
> SetCC);
> UntagAndJumpIfSmi
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:3493: __ mov(scratch, Operand(exponent, ASR,
> kSmiTagSize), SetCC);
> UntagAndJumpIfSmi
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:3503: __ mov(scratch, Operand(exponent, ASR,
> kSmiTagSize), SetCC);
> UntagAndJumpIfSmi
> 
> http://codereview.chromium.org/9223011/diff/1/src/arm/codegen-arm.cc
> File src/arm/codegen-arm.cc (right):
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/codegen-arm.cc#newcode158
> src/arm/codegen-arm.cc:158: STATIC_ASSERT(kSmiTagSize == 1 && kSmiTag == 0);
> Can we introduce UntagAndJumpIfNotSmi for these tree lines?
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/codegen-arm.cc#newcode344
> src/arm/codegen-arm.cc:344: __ b(ne, call_runtime);  // This branch refers to
> the previous CompareRoot.
> This branch should be above the comment in line 343.
> 
> http://codereview.chromium.org/9223011/diff/1/src/arm/lithium-codegen-arm.cc
> File src/arm/lithium-codegen-arm.cc (right):
> 
>
http://codereview.chromium.org/9223011/diff/1/src/arm/lithium-codegen-arm.cc#...
> src/arm/lithium-codegen-arm.cc:3931: STATIC_CHECK(kSmiTagSize == 1 && kSmiTag
==
> 0);
> UntagAndJumpIfSmi

[ulan, 2012-01-27 15:24:35]

LGTM with nits.

http://codereview.chromium.org/9223011/diff/1008/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/9223011/diff/1008/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:2977: Register dst, Register src, Label*
smi_case) {
smi_case should be non_smi_case

http://codereview.chromium.org/9223011/diff/1008/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/9223011/diff/1008/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.h:1152: // Tentatively untag and jump if the value
has been a smi.
"Tentatively" is a bit misleading, since the function doesn't restore the
original values. 
Consider something along this lines:
Untag the source register to the destination register and jump if the source
register is a smi. The source and the destination registers can be the same.