Re: ARM: Implement native substring copying. (issue552186)

[l...@chromium.org]

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.

http://codereview.chromium.org/552186

[l...@chromium.org]

Transfer of reviewer. You're it.

http://codereview.chromium.org/552186

[sgj...@chromium.org]

LGTM

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

http://codereview.chromium.org/552186/diff/9001/10003#newcode6858
src/arm/codegen-arm.cc:6858: __ ldrb(scratch, MemOperand(src, count),
pl);
Maybe add a comment on why ldrb is before b. Maybe before bind(&loop).

http://codereview.chromium.org/552186/diff/9001/10003#newcode6870
src/arm/codegen-arm.cc:6870:
Add an empty line.

http://codereview.chromium.org/552186/diff/9001/10003#newcode6882
src/arm/codegen-arm.cc:6882: bool dest_always_aligned = (flags &
DEST_ALWAYS_ALIGNED) != 0;
Perhaps generated code assert that dest is actually aligned when it is
suposed to be?

http://codereview.chromium.org/552186/diff/9001/10003#newcode6884
src/arm/codegen-arm.cc:6884: // Check that reading an entire aligned
word containing the last character
Check -> Ensure.

http://codereview.chromium.org/552186/diff/9001/10003#newcode6908
src/arm/codegen-arm.cc:6908: __ and_(scratch4, dest, Operand(0x03),
SetCC);
0x03 -> kPointerAlignmentMask?

http://codereview.chromium.org/552186/diff/9001/10003#newcode6924
src/arm/codegen-arm.cc:6924: __ and_(scratch4, scratch4, Operand(0x03),
SetCC);
0x03 -> kPointerAlignmentMask?

http://codereview.chromium.org/552186/diff/9001/10003#newcode6956
src/arm/codegen-arm.cc:6956: // scratch (eight times that number in
scratch4). We may have read past
scratch -> scratch1

http://codereview.chromium.org/552186/diff/9001/10003#newcode6986
src/arm/codegen-arm.cc:6986: __ cmp(scratch3, Operand(8));
I guess that this need to be 8 and not 4.

http://codereview.chromium.org/552186/diff/9001/10003#newcode7106
src/arm/codegen-arm.cc:7106: __ add(r1, r0,
Operand(SeqAsciiString::kHeaderSize - kHeapObjectTag));
// Locate first character of "from".

http://codereview.chromium.org/552186/diff/9001/10003#newcode7134
src/arm/codegen-arm.cc:7134: __ add(r1, r0,
Operand(SeqTwoByteString::kHeaderSize - kHeapObjectTag));
// Locate first character of "from".

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

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

http://codereview.chromium.org/552186