A64: Port LSeqStringSetChar optimizations from r16707 and r17521.

A64: Port LSeqStringSetChar optimizations from r16707 and r17521.

BUG=
TEST=mjsunit/lithium/SeqStringSetChar
R=baptiste.afsa@arm.com

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

[ulan, 2014-02-06 14:23:43]

PTAL

https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64.h
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64....
src/a64/macro-assembler-a64.h:1477: Register scratch,
This is different from arm port, which takes value register here and doesn't use
it.

[baptiste.afsa1, 2014-02-06 14:52:07]

I don't really like the fact that you have to untag/retag the index in
fullcodegen. IMO it would be better to add an extra argument to the
MacroAssembler function to specify whether the index is tag or untagged and make
the function able to cope with both situations. What do you think?

https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/141713009/diff/1/src/a64/macro-assembler-a64....
src/a64/macro-assembler-a64.cc:3645: Cmp(index, scratch);
You can avoid to Tag/Untag the index by untagging scratch here just for the Cmp.
Here it will be something similar to:
  Cmp(index, Operand::UntagSmi(scratch))

It will make use of shifted register and will avoid adding extra instructions.

[ulan, 2014-02-06 15:27:22]

On 2014/02/06 14:52:07, baptiste.afsa1 wrote:
> I don't really like the fact that you have to untag/retag the index in
> fullcodegen. IMO it would be better to add an extra argument to the
> MacroAssembler function to specify whether the index is tag or untagged and
make
> the function able to cope with both situations. What do you think?
Good idea, I uploaded a patch set that makes the function handle both cases.
PTAL.

[baptiste.afsa1, 2014-02-06 15:48:13]

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:3648: Cmp(index, Operand(Smi::FromInt(0)));
Although it will work, it doesn't really make sense to compare with
Smi::FromInt(0) when the index is not a SMI.

Maybe you should compare with 0 and assert that Smi::FromInt(0) == 0 with a
comment explaining what's going on here.

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.h:1479: bool index_is_smi,
Usually v8 functions which use that kind of trick use an enum to specify the
"mode" instead of a bool. Maybe you should do the same here.

[ulan, 2014-02-06 16:06:14]

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:3648: Cmp(index, Operand(Smi::FromInt(0)));
On 2014/02/06 15:48:13, baptiste.afsa1 wrote:
> Although it will work, it doesn't really make sense to compare with
> Smi::FromInt(0) when the index is not a SMI.
> 
> Maybe you should compare with 0 and assert that Smi::FromInt(0) == 0 with a
> comment explaining what's going on here.

I added assert, but didn't add comment since the assert and the code above makes
it clear.

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
File src/a64/macro-assembler-a64.h (right):

https://codereview.chromium.org/141713009/diff/100001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.h:1479: bool index_is_smi,
On 2014/02/06 15:48:13, baptiste.afsa1 wrote:
> Usually v8 functions which use that kind of trick use an enum to specify the
> "mode" instead of a bool. Maybe you should do the same here.

Done.

[baptiste.afsa1, 2014-02-06 16:11:21]

lgtm

[ulan, 2014-02-06 16:16:37]

Committed patchset #4 manually as r19166 (presubmit successful).