ARM: Optimize stubs for incremental marking.

ARM: Optimize stubs for incremental marking.

R=erik.corry@gmail.com
BUG=v8:1456
Committed: http://code.google.com/p/v8/source/detail?r=8954

[Erik Corry, 2011-08-16 09:29:23]

LGTM

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

http://codereview.chromium.org/7639015/diff/1006/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:6785: ne,
Seems like you should make this 'eq', change the label immediately below to
need_incremental and then you can get rid of the unconditional branch.

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

http://codereview.chromium.org/7639015/diff/1006/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3327: cmp(map,
Operand(FACTORY->heap_number_map()));
Use CompareRoot here.  It saves an entry in the constant pool and reduces the
amount of relocation info associated with the stub.

http://codereview.chromium.org/7639015/diff/1006/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3353: and_(instance_type, instance_type,
Operand(kStringEncodingMask));
This code goes to great lengths to avoid a hard-to-predict branch on Intel.  I
think on ARM it would be simpler to do a tst on the relevant bit and then a
conditional shift instruction.

http://codereview.chromium.org/7639015/diff/1006/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3364: mov(length, Operand(length, LSR, 2 +
kSmiTagSize));
Why are we dividing by 4 here?

[Michael Starzinger, 2011-08-16 15:31:05]

Added new patch set.

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

http://codereview.chromium.org/7639015/diff/1006/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:6785: ne,
On 2011/08/16 09:29:23, Erik Corry wrote:
> Seems like you should make this 'eq', change the label immediately below to
> need_incremental and then you can get rid of the unconditional branch.

Done.

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

http://codereview.chromium.org/7639015/diff/1006/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3327: cmp(map,
Operand(FACTORY->heap_number_map()));
On 2011/08/16 09:29:23, Erik Corry wrote:
> Use CompareRoot here.  It saves an entry in the constant pool and reduces the
> amount of relocation info associated with the stub.

Done.

http://codereview.chromium.org/7639015/diff/1006/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3353: and_(instance_type, instance_type,
Operand(kStringEncodingMask));
On 2011/08/16 09:29:23, Erik Corry wrote:
> This code goes to great lengths to avoid a hard-to-predict branch on Intel.  I
> think on ARM it would be simpler to do a tst on the relevant bit and then a
> conditional shift instruction.

Done. Simplified this part of the stub a lot. It now just checks for the string
encoding bit and then conditionally shifts the (char-)length value to get the
byte-size. Thanks for the great hint!

http://codereview.chromium.org/7639015/diff/1006/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3364: mov(length, Operand(length, LSR, 2 +
kSmiTagSize));
On 2011/08/16 09:29:23, Erik Corry wrote:
> Why are we dividing by 4 here?

Done. This one is gone now due to the above simplification. The SHR by 2 was in
place because char size was SHL by 2 in the above code. For ASCII
(instance_type==4) holds, for UC16 (instance_type==8) holds. The IA32 code is
very complex at that point and I subconsciously duplicated that complexity.
Should be much simpler now.

[Erik Corry, 2011-08-17 07:56:25]

Still LGTM

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

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3353: tst(instance_type,
Operand(kStringEncodingMask));
Please move the tst to be adjacent to the conditional mov.  This is clearer and
probably faster as the load has a minimum latency of two cycles so it needs to
be started earlier, at least on older CPUs that cannot lift it automatically.

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3359: mov(length, Operand(length, LSR,
kSmiTagSize), LeaveCC, ne);
Instead of kSmiTagSize I would suggest just '1' since the shift distance here
does not correspond to the size of the tag.  It's the difference between the
size of the tag and the log2 of the sizeof a UC16 character.  But that seems a
little complicated to express in code, so I suggest just writing '1' in
conjunction with the asserts you already have.

[Michael Starzinger, 2011-08-17 08:57:15]

Added new patch set.

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

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3353: tst(instance_type,
Operand(kStringEncodingMask));
On 2011/08/17 07:56:25, Erik Corry wrote:
> Please move the tst to be adjacent to the conditional mov.  This is clearer
and
> probably faster as the load has a minimum latency of two cycles so it needs to
> be started earlier, at least on older CPUs that cannot lift it automatically.

Done. I had to switch to the IP register, because instance_type and length are
both aliased to the same register.

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3359: mov(length, Operand(length, LSR,
kSmiTagSize), LeaveCC, ne);
On 2011/08/17 07:56:25, Erik Corry wrote:
> Instead of kSmiTagSize I would suggest just '1' since the shift distance here
> does not correspond to the size of the tag.  It's the difference between the
> size of the tag and the log2 of the sizeof a UC16 character.  But that seems a
> little complicated to express in code, so I suggest just writing '1' in
> conjunction with the asserts you already have.

Done.