Perform string add in generated code on X64 platform...

Perform string add in generated code on X64 platform

This is a port of the IA-32 version from r3400 (http://code.google.com/p/v8/source/detail?r=3400). In the X64 version the additional registers are used to avoid loading the instance type and arguments several times.

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

[Lasse Reichstein, 2009-12-08 14:16:09]

LGTM, only minor comments.

http://codereview.chromium.org/460109/diff/3001/3005
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/460109/diff/3001/3005#newcode537
src/x64/assembler-x64.h:537: void movw(Register src, const Operand& dst);
I would use movzxwl for loading instead, to avoid dependencies on the higher
bits.

http://codereview.chromium.org/460109/diff/3001/3006
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/460109/diff/3001/3006#newcode7800
src/x64/codegen-x64.cc:7800: StringAddStub stub(NO_STRING_CHECK_IN_STUB);
so NO_STRING_CHECK_IN_STUB means that the arguments are known to be strings.
Maybe rename it to ARGUMENTS_ARE_STRINGS.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7931
src/x64/codegen-x64.cc:7931: // edx: second string
Maybe comment on r8/r9 potential content here.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7940
src/x64/codegen-x64.cc:7940: // If arguments where known to be strings maps
where where not loaded to
"where" -> "were" (or "are"), comma after "strings", "where where"->"were"

http://codereview.chromium.org/460109/diff/3001/3006#newcode7953
src/x64/codegen-x64.cc:7953: ASSERT((String::kMaxLength & 0x80000000) == 0);
It should be safe to assume that kMaxLength isn't negative :)

http://codereview.chromium.org/460109/diff/3001/3006#newcode7957
src/x64/codegen-x64.cc:7957: // If result is not supposed to be flat allocate a
cons string object. If both
comma after "flat".

http://codereview.chromium.org/460109/diff/3001/3006#newcode7966
src/x64/codegen-x64.cc:7966: __ and_(rcx, r9);
Seems to assume that kAsciiStringTag == kStringEncodingMask, and that it's only
one bit (which is true, but could you assert it?)

http://codereview.chromium.org/460109/diff/3001/3006#newcode8001
src/x64/codegen-x64.cc:8001: __ j(equal, &string_add_runtime);
Is there a plan to make it work for external strings (shouldn't be that hard,
they are known to be flat so it's just finding a different place to copy from
than for sequential strings).

http://codereview.chromium.org/460109/diff/3001/3006#newcode8030
src/x64/codegen-x64.cc:8030: //__ movq(rdx, Operand(rsp, 1 * kPointerSize));
Code in comment?

http://codereview.chromium.org/460109/diff/3001/3006#newcode8089
src/x64/codegen-x64.cc:8089: bool ascii) {
You are really moving (count * (ascii ? 1 : 2)) bytes.
Maybe just do that, and avoid the movw operations.

You could use the counter as index in the operands and avoid updating dst and
src in the loop.

But again, it's just for short loops.

http://codereview.chromium.org/460109/diff/3001/3007
File src/x64/codegen-x64.h (right):

http://codereview.chromium.org/460109/diff/3001/3007#newcode751
src/x64/codegen-x64.h:751: NO_STRING_CHECK_IN_STUB = 1 << 0  // Omit string
check in stub.
Is the "0" in "1 << 0" a significant constant? If it is, give it a name,
otherwise just use "1".

http://codereview.chromium.org/460109/diff/3001/3007#newcode763
src/x64/codegen-x64.h:763: int MinorKey() { return string_check_ ? 0 : 1; }
Why not use the flag value directly?

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

http://codereview.chromium.org/460109/diff/3001/3002#newcode2264
src/x64/macro-assembler-x64.cc:2264: shl(scratch1, Immediate(1));
lea(scratch1, Operand(length, length, times_1, 0))
does the above two operations in one :)

http://codereview.chromium.org/460109/diff/3001/3002#newcode2296
src/x64/macro-assembler-x64.cc:2296: movsxlq(scratch1, length);
Sign extending shouldn't be necessary, since length should not be negative.

[Søren Thygesen Gjesse, 2009-12-09 09:35:10]

http://codereview.chromium.org/460109/diff/3001/3005
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/460109/diff/3001/3005#newcode537
src/x64/assembler-x64.h:537: void movw(Register src, const Operand& dst);
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> I would use movzxwl for loading instead, to avoid dependencies on the higher
> bits.

Good point. Changed the use of movw to movzxwl in the string copy code. Removed
this instruction from the assembler again, as it is  not used then and probably
it will not be useful.

http://codereview.chromium.org/460109/diff/3001/3006
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/460109/diff/3001/3006#newcode7800
src/x64/codegen-x64.cc:7800: StringAddStub stub(NO_STRING_CHECK_IN_STUB);
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> so NO_STRING_CHECK_IN_STUB means that the arguments are known to be strings.
> Maybe rename it to ARGUMENTS_ARE_STRINGS.

This follows the naming convention used by other stubs e.g.
NO_SMI_CHECK_IN_STUB, so I will leave it like this.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7931
src/x64/codegen-x64.cc:7931: // edx: second string
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Maybe comment on r8/r9 potential content here.

Done.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7940
src/x64/codegen-x64.cc:7940: // If arguments where known to be strings maps
where where not loaded to
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> "where" -> "were" (or "are"), comma after "strings", "where where"->"were"

Done.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7953
src/x64/codegen-x64.cc:7953: ASSERT((String::kMaxLength & 0x80000000) == 0);
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> It should be safe to assume that kMaxLength isn't negative :)

String::kMaxLength has type int so I am not sure thats right. Is 0x80000000
negative if sizeof(int) is 8? The cmpl below should be changed to cmpq then.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7957
src/x64/codegen-x64.cc:7957: // If result is not supposed to be flat allocate a
cons string object. If both
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> comma after "flat".

Done.

http://codereview.chromium.org/460109/diff/3001/3006#newcode7966
src/x64/codegen-x64.cc:7966: __ and_(rcx, r9);
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Seems to assume that kAsciiStringTag == kStringEncodingMask, and that it's
only
> one bit (which is true, but could you assert it?)

Added

  ASSERT_EQ(kStringEncodingMask, kAsciiStringTag);

and changed

  ASSERT(kAsciiStringTag != 0);

below as well.

Same changes to codegen-ia32.cc.

http://codereview.chromium.org/460109/diff/3001/3006#newcode8001
src/x64/codegen-x64.cc:8001: __ j(equal, &string_add_runtime);
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Is there a plan to make it work for external strings (shouldn't be that hard,
> they are known to be flat so it's just finding a different place to copy from
> than for sequential strings).

The problem with this is that the external string has a pointer to a C++ object
which provides the address and length of the string buffer through virtual
methods implemented by the embedder.

http://codereview.chromium.org/460109/diff/3001/3006#newcode8030
src/x64/codegen-x64.cc:8030: //__ movq(rdx, Operand(rsp, 1 * kPointerSize));
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Code in comment?

Deleted.

http://codereview.chromium.org/460109/diff/3001/3006#newcode8089
src/x64/codegen-x64.cc:8089: bool ascii) {
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> You are really moving (count * (ascii ? 1 : 2)) bytes.
> Maybe just do that, and avoid the movw operations.
> 
> You could use the counter as index in the operands and avoid updating dst and
> src in the loop.
> 
> But again, it's just for short loops.

I agree that this is a rather brain-dead implementation. I am quite sure we are
going to be copying longer strings soon, so I will leave it like this for now.

http://codereview.chromium.org/460109/diff/3001/3007
File src/x64/codegen-x64.h (right):

http://codereview.chromium.org/460109/diff/3001/3007#newcode751
src/x64/codegen-x64.h:751: NO_STRING_CHECK_IN_STUB = 1 << 0  // Omit string
check in stub.
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Is the "0" in "1 << 0" a significant constant? If it is, give it a name,
> otherwise just use "1".

This way of specifying bits for flag sets as well (even when only one flag is
used), so I will leave it like this.

http://codereview.chromium.org/460109/diff/3001/3007#newcode763
src/x64/codegen-x64.h:763: int MinorKey() { return string_check_ ? 0 : 1; }
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Why not use the flag value directly?

The code stub minor key needs to be an integer.

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

http://codereview.chromium.org/460109/diff/3001/3002#newcode2264
src/x64/macro-assembler-x64.cc:2264: shl(scratch1, Immediate(1));
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> lea(scratch1, Operand(length, length, times_1, 0))
> does the above two operations in one :)

Good point, changed it to 

  lea(scratch1, Operand(length, length, times_1, kObjectAlignmentMask));

and got rid of the addq below as well.

Same change to macro-assembler-ia32.cc.

http://codereview.chromium.org/460109/diff/3001/3002#newcode2296
src/x64/macro-assembler-x64.cc:2296: movsxlq(scratch1, length);
On 2009/12/08 14:16:11, Lasse Reichstein wrote:
> Sign extending shouldn't be necessary, since length should not be negative.

Right, I was missing movzxlq which turns out to be just movl which is now used.

[Lasse Reichstein, 2009-12-09 10:00:32]

http://codereview.chromium.org/460109/diff/3001/3006
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/460109/diff/3001/3006#newcode7953
src/x64/codegen-x64.cc:7953: ASSERT((String::kMaxLength & 0x80000000) == 0);
It's definitly true that kMaxLength isn't negative.
Whether it's the same as not having bit 31 set, that's a different question :)
We assume that int is 32-bit in many, many places already, and you read the
string length from a 32-bit field in other parts of this code, so string lengths
are necessarily restricted to 32 bits.
kMaxLength should probably be unsigned, but the "above" test takes care of that
anyway.

That strings lengths are actually less than 31 bits is more important to check
above, where you add two lengths and don't check for overflow (you don't need
to, but it isn't asserted there).

http://codereview.chromium.org/460109/diff/3001/3007
File src/x64/codegen-x64.h (right):

http://codereview.chromium.org/460109/diff/3001/3007#newcode763
src/x64/codegen-x64.h:763: int MinorKey() { return string_check_ ? 0 : 1; }
Yes, but if you just return string_check from a method returning int, it will be
converted. 
Here you actively swap the 0 and 1 values, and I was wondering if there was a
reason for that.