ARM: Implement native substring copying.

ARM: Implement native substring copying.

[Lasse Reichstein, 2010-01-27 14:19:03]

ARM review for you.

[Erik Corry, 2010-01-28 09:22:05]

http://codereview.chromium.org/552186/diff/3001/3005
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/552186/diff/3001/3005#newcode938
src/arm/macro-assembler-arm.cc:938: ASSERT((SeqAsciiString::kHeaderSize &
kObjectAlignmentMask) == 0);
SeqAsciiString?

http://codereview.chromium.org/552186/diff/3001/3005#newcode940
src/arm/macro-assembler-arm.cc:940: add(scratch1, length,
Operand(kObjectAlignmentMask));
Nooo.

Do we need better tests?

[Erik Corry, 2010-01-28 10:05:35]

http://codereview.chromium.org/552186/diff/3001/3003
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/552186/diff/3001/3003#newcode6797
src/arm/codegen-arm.cc:6797: bool ascii) {
All this complexity needs to be justified at least by an artificial benchmark.

http://codereview.chromium.org/552186/diff/3001/3003#newcode6834
src/arm/codegen-arm.cc:6834: if (FLAG_debug_code) {
I hate FLAG_debug_code because if you get a bug related to it being missing you
can't easily debug the bug in the debug version of the VM.  Can we remove this
when debugging is done.

http://codereview.chromium.org/552186/diff/3001/3003#newcode6994
src/arm/codegen-arm.cc:6994: __ cmp(r1,
Operand(static_cast<int32_t>(kSeqStringTag | kTwoByteStringTag)));
This happens to be zero.  Perhaps you should reorder the ASCII and twobyte cases
then you can use a SetCC on the and_ and tada!

http://codereview.chromium.org/552186/diff/3001/3005
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/552186/diff/3001/3005#newcode952
src/arm/macro-assembler-arm.cc:952: mov(scratch1,
Operand(Factory::ascii_string_map()));
This just feels like the wrong map to be loading here :-)
Also, please use LoadRoot if the map is in the roots array.

[Lasse Reichstein, 2010-02-02 14:43:22]

Please re-review. 
It actually works now. And is called.

http://codereview.chromium.org/552186/diff/3001/3003
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/552186/diff/3001/3003#newcode6797
src/arm/codegen-arm.cc:6797: bool ascii) {
I'll see how artificial I can make it.

http://codereview.chromium.org/552186/diff/3001/3003#newcode6834
src/arm/codegen-arm.cc:6834: if (FLAG_debug_code) {
Not understood. FLAG_debug_code is off by default in debug mode too.
I'll remove this, because all it really tests is that the previous code does
what it's supposed to do.

http://codereview.chromium.org/552186/diff/3001/3003#newcode6994
src/arm/codegen-arm.cc:6994: __ cmp(r1,
Operand(static_cast<int32_t>(kSeqStringTag | kTwoByteStringTag)));
Rewritten completely.

http://codereview.chromium.org/552186/diff/3001/3005
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/552186/diff/3001/3005#newcode940
src/arm/macro-assembler-arm.cc:940: add(scratch1, length,
Operand(kObjectAlignmentMask));
Nope, better coders.
Stack alignment was off, so the wrong argument was read ... and it was never a
string. I.e., we hit the runtime system every time.

http://codereview.chromium.org/552186/diff/3001/3005#newcode952
src/arm/macro-assembler-arm.cc:952: mov(scratch1,
Operand(Factory::ascii_string_map()));
Fixed. Along with all the ASCII's.

[Lasse Reichstein, 2010-02-03 13:32:15]

Transfer of reviewer. You're it.

[Lasse Reichstein, 2011-06-14 11:13:06]

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:6858: __ ldrb(scratch, MemOperand(src, count), pl);
Done.

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:6870: 
Done.

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:6882: bool dest_always_aligned = (flags &
DEST_ALWAYS_ALIGNED) != 0;
Added.

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:6908: __ and_(scratch4, dest, Operand(0x03), SetCC);
More like "kReadAlignmentMask" (which is word alignment, which just happens to
be pointer alignment too). I'll add a local constant.

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:6986: __ cmp(scratch3, Operand(8));
Added comment to explain.

http://codereview.chromium.org/552186/diff/9001/src/arm/codegen-arm.cc#newcod...
src/arm/codegen-arm.cc:7106: __ add(r1, r0, Operand(SeqAsciiString::kHeaderSize
- kHeapObjectTag));
Comment added.

http://codereview.chromium.org/552186/diff/9001/test/mjsunit/substr.js
File test/mjsunit/substr.js (right):

http://codereview.chromium.org/552186/diff/9001/test/mjsunit/substr.js#newcode47
test/mjsunit/substr.js:47: //for (var i = 0; i < s.length; i++)
Whoops. Removed.