Generated code for substring slices in x64 and arm.

Generated code for substring slices in x64 and arm.

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

[Yang, 2011-08-30 13:22:15]

Please take a look.

[antonm, 2011-08-31 13:08:42]

LGTM, but Erik and William might be better reviewers for ARM and x64.

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

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5658: STATIC_ASSERT(kIsIndirectStringMask ==
(kSlicedStringTag & kConsStringTag));
those constants might be worth lifting into objects.h

[Yang, 2011-08-31 13:13:09]

+Erik +Bill as Anton suggested.

[Erik Corry, 2011-08-31 14:14:13]

ARM part LGTM, but may have redundant code.

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

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5486: // Special handling of sub-strings of length 1
and 2. One character strings
This comment is out of date, since the code handles substrings of length 2
without going into the runtime system.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5508: __ b(ge, &return_r0);
I think eq is clearer than ge.  A comment to the effect that r2 is the length
would be nice.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5527: __ b(gt, &runtime);  // External strings go to
runtime.
Update comment to say that external and sliced strings go to runtime.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5538: __ b(ne, &runtime);  // Cons and External
strings go to runtime.
and slices, or can they not be found under a cons string?

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5546: // r3: from index (untaged smi)
untaged -> untagged
also at least one other place.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5554: // r0: original string
Original string or left hand side of the original cons string.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5600: // r5: first character of sub string to copy
sub string -> substring

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5623: // byte character.
Add a static assert that the kSmiTag is 0 and kSmiTagSize is 1.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5631: // r5: first character of sub string to copy
sub string -> substring

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5666: // Cons string.  Check whether it is flat, then
fetch first part.
How do we get here if it's a cons string?

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5675: // Sliced string.  Fetch parent and correct
start index by offset.
Do you have test coverage here?  I think we already bailed to runtime in this
case.

[Yang, 2011-08-31 15:57:59]

On 2011/08/31 14:14:13, Erik Corry wrote:
> ARM part LGTM, but may have redundant code.
> 
> http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5486: // Special handling of sub-strings of length 1
> and 2. One character strings
> This comment is out of date, since the code handles substrings of length 2
> without going into the runtime system.
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5508: __ b(ge, &return_r0);
> I think eq is clearer than ge.  A comment to the effect that r2 is the length
> would be nice.
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5527: __ b(gt, &runtime);  // External strings go to
> runtime.
> Update comment to say that external and sliced strings go to runtime.
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5538: __ b(ne, &runtime);  // Cons and External
> strings go to runtime.
> and slices, or can they not be found under a cons string?
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5546: // r3: from index (untaged smi)
> untaged -> untagged
> also at least one other place.
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5554: // r0: original string
> Original string or left hand side of the original cons string.
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5600: // r5: first character of sub string to copy
> sub string -> substring
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5623: // byte character.
> Add a static assert that the kSmiTag is 0 and kSmiTagSize is 1.
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5631: // r5: first character of sub string to copy
> sub string -> substring
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5666: // Cons string.  Check whether it is flat,
then
> fetch first part.
> How do we get here if it's a cons string?
> 
>
http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
> src/arm/code-stubs-arm.cc:5675: // Sliced string.  Fetch parent and correct
> start index by offset.
> Do you have test coverage here?  I think we already bailed to runtime in this
> case.

Changed a few things according to suggestion and added a cctest to test slice of
slice.

[Yang, 2011-08-31 15:58:16]

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

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5527: __ b(gt, &runtime);  // External strings go to
runtime.
On 2011/08/31 14:14:13, Erik Corry wrote:
> Update comment to say that external and sliced strings go to runtime.

Note that (with FLAG_string_slices==true) this bailout only happens if the
string is shorter than SlicedString::kMinLength, and an external or sliced
string.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5666: // Cons string.  Check whether it is flat, then
fetch first part.
On 2011/08/31 14:14:13, Erik Corry wrote:
> How do we get here if it's a cons string?

A cons string's string representation bits has the kIsIndirectStringMask set
(both cons and slices are "indirect strings"), therefore it is not bailed out to
runtime, and further checked whether it has the kSlicedNotConsMask bit set.
Since that bit is not set, we do not jump to sliced_string, but proceed with the
part that handles cons strings.

http://codereview.chromium.org/7795018/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5675: // Sliced string.  Fetch parent and correct
start index by offset.
On 2011/08/31 14:14:13, Erik Corry wrote:
> Do you have test coverage here?  I think we already bailed to runtime in this
> case.

We only bail out in the case when the original string is a slice if the
substring length is shorter than SlicedString::kMinLength. In that case we do
not create a slice, but create a seq string, to which we copy the content.

If the substring length is longer or equal SlicedString::kMinLength and
FLAG_string_slices==true, then the code jumps to the label create_slice. 

I added a cctest for slice of slice to check the string shapes.

[William Hesse, 2011-09-01 08:26:02]

LGTM.

http://codereview.chromium.org/7795018/diff/3002/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7795018/diff/3002/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4763: __ testb(rbx, Immediate(kAsciiStringTag));
I would use kStringEncodingMask and assert
kStringEncodingMask & kAsciiStringTag != 0
and
kStringEncodingMask & kTwoByteStringTag == 0

http://codereview.chromium.org/7795018/diff/3002/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7795018/diff/3002/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:3662: void
MacroAssembler::AllocateSlicedString(Register result,
I would call this "AllocateTwoByteSlicedString".  Even though constants leave
off "TwoByte" in their names, I think we use it in function names.

[Yang, 2011-09-01 11:27:02]

http://codereview.chromium.org/7795018/diff/3002/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7795018/diff/3002/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4763: __ testb(rbx, Immediate(kAsciiStringTag));
On 2011/09/01 08:26:02, William Hesse wrote:
> I would use kStringEncodingMask and assert
> kStringEncodingMask & kAsciiStringTag != 0
> and
> kStringEncodingMask & kTwoByteStringTag == 0

Done. Also changed for a bunch of similar places.

http://codereview.chromium.org/7795018/diff/3002/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7795018/diff/3002/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:3662: void
MacroAssembler::AllocateSlicedString(Register result,
On 2011/09/01 08:26:02, William Hesse wrote:
> I would call this "AllocateTwoByteSlicedString".  Even though constants leave
> off "TwoByte" in their names, I think we use it in function names.

Done. Also changed for ia32.