Implement inline string compare on ARM.

Implement inline string compare on ARM.
Backport optimizations from x64 version to ia32.

[Lasse Reichstein, 2010-01-20 11:34:15]

Please review.
Erik, please check the ARM version.

[Erik Corry, 2010-01-20 12:02:33]

LGTM with comments.

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

http://codereview.chromium.org/546087/diff/1/2#newcode6776
src/arm/codegen-arm.cc:6776: {
If there are a lot of strings that almost match then a word-oriented loop
prepended to this byte-oriented loop would probably be a big win.  But if, as
seems likely, most strings differ already on the first character then it's
probably not a win.

http://codereview.chromium.org/546087/diff/1/2#newcode6794
src/arm/codegen-arm.cc:6794: __ mov(r0, Operand(Smi::FromInt(LESS)->value()));
Might as well write Operand(LESS) here.  There are two more of these.

http://codereview.chromium.org/546087/diff/1/2#newcode6808
src/arm/codegen-arm.cc:6808: __ b(ne, &result_not_equal);
I think you should replace this with a b(lt, followed by a b(gt,

http://codereview.chromium.org/546087/diff/1/4
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/546087/diff/1/4#newcode1234
src/arm/macro-assembler-arm.cc:1234: // Test that both strings are
This sentence is

[antonm, 2010-01-20 12:13:17]

Minor nit.

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

http://codereview.chromium.org/546087/diff/1/3#newcode517
src/arm/codegen-arm.h:517: explicit StringCompareStub() {
nit: explicit is not needed?

[Søren Thygesen Gjesse, 2010-01-20 12:30:18]

LGTM

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

http://codereview.chromium.org/546087/diff/1/2#newcode6762
src/arm/codegen-arm.cc:6762: // in the loop.
Does using a PostIndex on ARM actually spend more time than not using it?

http://codereview.chromium.org/546087/diff/1/2#newcode6794
src/arm/codegen-arm.cc:6794: __ mov(r0, Operand(Smi::FromInt(LESS)->value()));
Return tagged smi.

http://codereview.chromium.org/546087/diff/1/6
File src/ia32/assembler-ia32.cc (right):

http://codereview.chromium.org/546087/diff/1/6#newcode863
src/ia32/assembler-ia32.cc:863: void Assembler::cmpb(const Operand& dst,
Register src) {
Assert register is byte register.

http://codereview.chromium.org/546087/diff/1/6#newcode871
src/ia32/assembler-ia32.cc:871: void Assembler::cmpb(Register dst, const
Operand& src) {
Ditto.

http://codereview.chromium.org/546087/diff/1/7
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/546087/diff/1/7#newcode563
src/ia32/assembler-ia32.h:563: void cmpb(const Operand& dst, Register src);
Add use of both cmpb versions to test-disasm-ia32.cc.

http://codereview.chromium.org/546087/diff/1/8
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/546087/diff/1/8#newcode9662
src/ia32/codegen-ia32.cc:9662: // Find minimum length. If either length is zero,
just compare lengths.
Move the "If either length is zero, just compare lengths." part of the comment
down to

  __ test(min_length, Operand(min_length));

http://codereview.chromium.org/546087/diff/1/8#newcode9715
src/ia32/codegen-ia32.cc:9715: __ Set(eax,
Immediate(Smi::FromInt(EQUAL)->value()));
As you mentioned offline remove ->value().

http://codereview.chromium.org/546087/diff/1/10
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/546087/diff/1/10#newcode1549
src/ia32/macro-assembler-ia32.cc:1549: const int kFlatAsciiStringTag =
kStringTag | kSeqStringTag | kAsciiStringTag;
How about moving kFlatAsciiStringMask and kFlatAsciiStringTag to objects.h?

http://codereview.chromium.org/546087/diff/1/11
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/546087/diff/1/11#newcode425
src/ia32/macro-assembler-ia32.h:425: Label *on_not_flat_ascii_strings);
Remove on_ prefix?

[Lasse Reichstein, 2010-01-21 09:40:50]

Addressed comments.
Made ARM version actually work.

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

http://codereview.chromium.org/546087/diff/1/2#newcode6762
src/arm/codegen-arm.cc:6762: // in the loop.
Probably not, but it doesn't work as I momentarily thought either, so this is
simply broken. Seems I forgot ARM addressing over christmas.
Turns out non of our tests actually hit this stub.
I have added a call to the code from the generic compare stub, which clearly
show that it's broken.
I'll completely rewrite the loop.

http://codereview.chromium.org/546087/diff/1/2#newcode6776
src/arm/codegen-arm.cc:6776: {
We can test it later. For now, I'll assume that the most common case is failing
to match at the first character.

http://codereview.chromium.org/546087/diff/1/2#newcode6794
src/arm/codegen-arm.cc:6794: __ mov(r0, Operand(Smi::FromInt(LESS)->value()));
Fixed to return a smi both here and in ia32.

http://codereview.chromium.org/546087/diff/1/2#newcode6808
src/arm/codegen-arm.cc:6808: __ b(ne, &result_not_equal);
Done.

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

http://codereview.chromium.org/546087/diff/1/3#newcode517
src/arm/codegen-arm.h:517: explicit StringCompareStub() {
Good catch.

http://codereview.chromium.org/546087/diff/1/4
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/546087/diff/1/4#newcode1234
src/arm/macro-assembler-arm.cc:1234: // Test that both strings are
now complete.

http://codereview.chromium.org/546087/diff/1/6
File src/ia32/assembler-ia32.cc (right):

http://codereview.chromium.org/546087/diff/1/6#newcode863
src/ia32/assembler-ia32.cc:863: void Assembler::cmpb(const Operand& dst,
Register src) {
Done.

http://codereview.chromium.org/546087/diff/1/6#newcode871
src/ia32/assembler-ia32.cc:871: void Assembler::cmpb(Register dst, const
Operand& src) {
Ditto.

http://codereview.chromium.org/546087/diff/1/7
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/546087/diff/1/7#newcode563
src/ia32/assembler-ia32.h:563: void cmpb(const Operand& dst, Register src);
Done.

http://codereview.chromium.org/546087/diff/1/8
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/546087/diff/1/8#newcode9662
src/ia32/codegen-ia32.cc:9662: // Find minimum length. If either length is zero,
just compare lengths.
Done.

http://codereview.chromium.org/546087/diff/1/10
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/546087/diff/1/10#newcode1549
src/ia32/macro-assembler-ia32.cc:1549: const int kFlatAsciiStringTag =
kStringTag | kSeqStringTag | kAsciiStringTag;
If we did that, we think we should have all the possible
representation/type/symbol/string/etc. combinations present, not just one that
we happen to need here.

On the other hand, we do have kFlatAsciiStringTag already, as ASCII_STRING_TYPE.

http://codereview.chromium.org/546087/diff/1/11
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/546087/diff/1/11#newcode425
src/ia32/macro-assembler-ia32.h:425: Label *on_not_flat_ascii_strings);
Guess it isn't the traditional notation.

[Erik Corry, 2010-01-21 10:23:19]

LGTM

http://codereview.chromium.org/546087/diff/6001/7001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/546087/diff/6001/7001#newcode5209
src/arm/codegen-arm.cc:5209: // Both the builtin and the StringCompareStub code
expects arguments on the
expects -> expect

http://codereview.chromium.org/546087/diff/6001/7001#newcode6809
src/arm/codegen-arm.cc:6809: __ b(gt, &result_greater);
This b could be eliminated by using conditional mov.

http://codereview.chromium.org/546087/diff/6001/7001#newcode6824
src/arm/codegen-arm.cc:6824: __ b(gt, &result_greater);
I recommended this, but now I would suggest conditional mov instead.  In fact if
you use r0 instead of length_delta then you don't need to move 0 to the
register.

[Søren Thygesen Gjesse, 2010-01-21 10:26:27]

http://codereview.chromium.org/546087/diff/6001/7007
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/546087/diff/6001/7007#newcode9683
src/ia32/codegen-ia32.cc:9683: // which doesn't need an additional compare.
Doesn't these two lea's have the kHeapObjectTag == 1 asumption?

[Lasse Reichstein, 2010-01-21 12:03:03]

How about this then, Erik?

http://codereview.chromium.org/546087/diff/6001/7001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/546087/diff/6001/7001#newcode5209
src/arm/codegen-arm.cc:5209: // Both the builtin and the StringCompareStub code
expects arguments on the
Actually comment is no longer correct. I changed the GenerateComplareFlat.... to
not expect arguments lying around on the stack.
Comment removed.

http://codereview.chromium.org/546087/diff/6001/7001#newcode6809
src/arm/codegen-arm.cc:6809: __ b(gt, &result_greater);
On 2010/01/21 10:23:19, Erik Corry wrote:
> This b could be eliminated by using conditional mov.

Interesting. Let's see how much crud can be eliminated by just making the moves
conditional.

http://codereview.chromium.org/546087/diff/6001/7001#newcode6824
src/arm/codegen-arm.cc:6824: __ b(gt, &result_greater);
On 2010/01/21 10:23:19, Erik Corry wrote:
> I recommended this, but now I would suggest conditional mov instead.  In fact
if
> you use r0 instead of length_delta then you don't need to move 0 to the
> register.

Done.

[Erik Corry, 2010-01-21 12:07:23]

LGTM (you sick puppy!)

[Søren Thygesen Gjesse, 2010-01-21 12:30:34]

http://codereview.chromium.org/546087/diff/6001/7007
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/546087/diff/6001/7007#newcode9683
src/ia32/codegen-ia32.cc:9683: // which doesn't need an additional compare.
On 2010/01/21 10:26:27, Søren Gjesse wrote:
> Doesn't these two lea's have the kHeapObjectTag == 1 asumption?

Ignore that I just noticed that FieldOperand is used and it fits with the rest.