ARM native string addition.

ARM native string addition.

[Lasse Reichstein, 2010-02-04 13:16:47]

Review of string add for ARM.

[Søren Thygesen Gjesse, 2010-02-05 08:17:16]

LGTM - nice use of ARM conditional instructions.

http://codereview.chromium.org/571005/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/571005/diff/1/2#newcode7228
src/arm/codegen-arm.cc:7228: //  sp[4]: left string
Woops. Good catch.

http://codereview.chromium.org/571005/diff/1/2#newcode7271
src/arm/codegen-arm.cc:7271: ASSERT_EQ(0, kSmiTag);
Add JumpIfNotBothSmi to macro assembler?

http://codereview.chromium.org/571005/diff/1/2#newcode7403
src/arm/codegen-arm.cc:7403: __ add(r6, r7, Operand(SeqAsciiString::kHeaderSize
- kHeapObjectTag));
Use MemOperand to avoid "- kHeapObjectTag"?

http://codereview.chromium.org/571005/diff/1/2#newcode7405
src/arm/codegen-arm.cc:7405: __ add(r0, r0, Operand(SeqAsciiString::kHeaderSize
- kHeapObjectTag));
Ditto.

http://codereview.chromium.org/571005/diff/1/2#newcode7415
src/arm/codegen-arm.cc:7415: __ add(r1, r1, Operand(SeqAsciiString::kHeaderSize
- kHeapObjectTag));
Ditto.

http://codereview.chromium.org/571005/diff/1/2#newcode7441
src/arm/codegen-arm.cc:7441: __ add(r6, r7,
Operand(SeqTwoByteString::kHeaderSize - kHeapObjectTag));
Ditto.

http://codereview.chromium.org/571005/diff/1/2#newcode7443
src/arm/codegen-arm.cc:7443: __ add(r0, r0,
Operand(SeqTwoByteString::kHeaderSize - kHeapObjectTag));
Ditto.

http://codereview.chromium.org/571005/diff/1/2#newcode7454
src/arm/codegen-arm.cc:7454: __ add(r1, r1,
Operand(SeqTwoByteString::kHeaderSize - kHeapObjectTag));
Ditto.

http://codereview.chromium.org/571005/diff/1/3
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/571005/diff/1/3#newcode572
src/arm/codegen-arm.h:572: class StringAddStub: public StringStubBase {
Now this exists and is (I think) identical on all platforms. Would it be  a good
idea to move it to codegen.h?

[Lasse Reichstein, 2010-02-05 08:36:21]

http://codereview.chromium.org/571005/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/571005/diff/1/2#newcode7271
src/arm/codegen-arm.cc:7271: ASSERT_EQ(0, kSmiTag);
Done. And JumpIfEitherSmi, which is what we need here.

http://codereview.chromium.org/571005/diff/1/2#newcode7403
src/arm/codegen-arm.cc:7403: __ add(r6, r7, Operand(SeqAsciiString::kHeaderSize
- kHeapObjectTag));
Doesn't work in ARM. Operand and MemOperand are distinct, and we only have
FieldMemOperand, not FieldOperand (and I don't think we should).

http://codereview.chromium.org/571005/diff/1/3
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/571005/diff/1/3#newcode572
src/arm/codegen-arm.h:572: class StringAddStub: public StringStubBase {
I don't think so (but could be convinced).
The fact that the three implementations are identical doesn't necessarily mean
that they have to be so. Different platforms can have different requirements and
parameterizations. In fact, we currently don't create both kinds of stubs on ARM
(which is a mistake that I'll fix and upload a new version).
If the versions are not inherently identical, I don't want to collapse them. It
will make it harder to change one of them later.

[Søren Thygesen Gjesse, 2010-02-05 11:54:02]

LGTM