Perform string add in generated code on IA-32 platforms...

Perform string add in generated code on IA-32 platforms

This adds a code stub which can do most of what Heap::AllocateConsString can do. It bails out if the result cannot fit in new space or if the result is a short (flat) string and one argument is an ascii string and the other a two byte string. It also bails out if adding two one character strings as Heap::AllocateConsString has special handling of this utilizing the symbol table. The stub is used both for the binary add operation and for StringAdd calls from runtime JavaScript files. Extended the string add test to cover all sizes of flat result stings.

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

[Mads Ager (chromium), 2009-11-27 08:13:54]

LGTM!

Nice comments in the code generator.  The code is easy to follow.

http://codereview.chromium.org/442024/diff/2001/3002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/442024/diff/2001/3002#newcode7059
src/ia32/codegen-ia32.cc:7059: // First and second argument are strings jump to
the string add stub.
"... are strings. Jump to the ..."?

http://codereview.chromium.org/442024/diff/2001/3002#newcode8365
src/ia32/codegen-ia32.cc:8365: void
StringAddStub::GenerateCopyAsciiCharacters(MacroAssembler* masm,
You could have just one GenerateCopyCharacters and pass it an argument
indicating whether the string is ascii or two-byte.

http://codereview.chromium.org/442024/diff/2001/3003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/442024/diff/2001/3003#newcode773
src/ia32/codegen-ia32.h:773: bool string_check_;  // Should the stub check
whether arguments are strings?
Put the comment on the line before?

[Lasse Reichstein, 2009-11-27 08:40:16]

http://codereview.chromium.org/442024/diff/2001/3002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/442024/diff/2001/3002#newcode8365
src/ia32/codegen-ia32.cc:8365: void
StringAddStub::GenerateCopyAsciiCharacters(MacroAssembler* masm,
This seems inefficient at first, moving one byte at a time, but since its only
used for small counts (<13), it's probably not worth changing.
Could you add a comment saying that count is usually low?

[Erik Corry, 2009-11-27 14:29:57]

http://codereview.chromium.org/442024/diff/2001/3002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/442024/diff/2001/3002#newcode8242
src/ia32/codegen-ia32.cc:8242: __ j(equal, &string_add_runtime);
Did you check that this is worth it?

http://codereview.chromium.org/442024/diff/2001/3002#newcode8247
src/ia32/codegen-ia32.cc:8247: __ cmp(ebx, String::kMaxLength);
Perhaps assert here that kMaxLength*2 fits in 32 bits.

http://codereview.chromium.org/442024/diff/2001/3002#newcode8298
src/ia32/codegen-ia32.cc:8298: __ add(Operand(ecx),
Immediate(SeqAsciiString::kHeaderSize - kHeapObjectTag));
What about external strings?

[Søren Thygesen Gjesse, 2009-12-03 07:48:41]

Added more tests in  cctest/test-strings/ExternalShortStringAdd adding internal
and external strings.

http://codereview.chromium.org/442024/diff/2001/3002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/442024/diff/2001/3002#newcode7059
src/ia32/codegen-ia32.cc:7059: // First and second argument are strings jump to
the string add stub.
On 2009/11/27 08:13:54, Mads Ager wrote:
> "... are strings. Jump to the ..."?

Done.

http://codereview.chromium.org/442024/diff/2001/3002#newcode8242
src/ia32/codegen-ia32.cc:8242: __ j(equal, &string_add_runtime);
On 2009/11/27 14:29:57, Erik Corry wrote:
> Did you check that this is worth it?

There is no measurable difference between calling to the runtime for two one
character strings and not. I will leave it like this and do some more
experiments on the handling of two char strings.

http://codereview.chromium.org/442024/diff/2001/3002#newcode8247
src/ia32/codegen-ia32.cc:8247: __ cmp(ebx, String::kMaxLength);
On 2009/11/27 14:29:57, Erik Corry wrote:
> Perhaps assert here that kMaxLength*2 fits in 32 bits.

Done.

http://codereview.chromium.org/442024/diff/2001/3002#newcode8298
src/ia32/codegen-ia32.cc:8298: __ add(Operand(ecx),
Immediate(SeqAsciiString::kHeaderSize - kHeapObjectTag));
On 2009/11/27 14:29:57, Erik Corry wrote:
> What about external strings?

Good catch. Added check for external strings and handle this in the runtime
system.

http://codereview.chromium.org/442024/diff/2001/3002#newcode8365
src/ia32/codegen-ia32.cc:8365: void
StringAddStub::GenerateCopyAsciiCharacters(MacroAssembler* masm,
On 2009/11/27 08:13:54, Mads Ager wrote:
> You could have just one GenerateCopyCharacters and pass it an argument
> indicating whether the string is ascii or two-byte.

Done.

http://codereview.chromium.org/442024/diff/2001/3002#newcode8365
src/ia32/codegen-ia32.cc:8365: void
StringAddStub::GenerateCopyAsciiCharacters(MacroAssembler* masm,
On 2009/11/27 08:40:16, Lasse Reichstein wrote:
> This seems inefficient at first, moving one byte at a time, but since its only
> used for small counts (<13), it's probably not worth changing.
> Could you add a comment saying that count is usually low?

Added comment.

http://codereview.chromium.org/442024/diff/2001/3003
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/442024/diff/2001/3003#newcode773
src/ia32/codegen-ia32.h:773: bool string_check_;  // Should the stub check
whether arguments are strings?
On 2009/11/27 08:13:54, Mads Ager wrote:
> Put the comment on the line before?

Done.

[Erik Corry, 2009-12-03 08:13:55]

LGTM.

I want it for ARM too!  (and 64 bit).

Would it be hard to call the string add code directly from the BinaryOpStub? 
Would it help?

http://codereview.chromium.org/442024/diff/7001/8002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/442024/diff/7001/8002#newcode8305
src/ia32/codegen-ia32.cc:8305: __ movzx_b(ecx, FieldOperand(ecx,
Map::kInstanceTypeOffset));
Here you load the type fields again that you just loaded in likes 8285 and 8290.
 Can you somehow rearrange the registers to avoid this?

http://codereview.chromium.org/442024/diff/7001/8002#newcode8341
src/ia32/codegen-ia32.cc:8341: __ movzx_b(ecx, FieldOperand(ecx,
Map::kInstanceTypeOffset));
Same issue here.