Port optimized comparison of a string to a constant single character string t...

Port optimized comparison of a string to a constant single character string to X64 platform.  Fix small mistake on ia32 platform.

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

[Lasse Reichstein, 2010-04-13 08:25:01]

LGTM with comments.

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

http://codereview.chromium.org/1627014/diff/1/2#newcode5317
src/x64/codegen-x64.cc:5317: Condition is_smi = __ CheckSmi(left_reg);
Use masm()->CheckSmi (or however the __ macro is defined by default).
The __ macro doesn't return a value if code-coverage is enabled.

http://codereview.chromium.org/1627014/diff/1/2#newcode5322
src/x64/codegen-x64.cc:5322: FieldOperand(left_reg, HeapObject::kMapOffset));
Indent second line by one more.

http://codereview.chromium.org/1627014/diff/1/2#newcode5335
src/x64/codegen-x64.cc:5335: // They are symbols, so do identity compare.
Are all one-character strings symbols? 
Otherwise, where did we check that the rhs is a symbol?

http://codereview.chromium.org/1627014/diff/1/2#newcode5338
src/x64/codegen-x64.cc:5338: dest->false_target()->Branch(not_equal);
Second branch can be unconditional.

http://codereview.chromium.org/1627014/diff/1/2#newcode5341
src/x64/codegen-x64.cc:5341: // If the receiver is not a string of the type we
handle call the stub.
Comma after "handle"?

http://codereview.chromium.org/1627014/diff/1/2#newcode5342
src/x64/codegen-x64.cc:5342: __ andl(temp.reg(),
Can we use andb instead? I should make the immediate 8-bit instead of 32.

http://codereview.chromium.org/1627014/diff/1/2#newcode5343
src/x64/codegen-x64.cc:5343: Immediate(kIsNotStringMask |
Indentation.

http://codereview.chromium.org/1627014/diff/1/2#newcode5347
src/x64/codegen-x64.cc:5347: Immediate(kStringTag | kSeqStringTag |
kAsciiStringTag));
Ditto for cmpb.

http://codereview.chromium.org/1627014/diff/1/2#newcode5356
src/x64/codegen-x64.cc:5356: __ testq(result.reg(), result.reg());
Don't compare a smi to 0 like this. It assumes the smi representation. Use
SmiCompare(result.reg(), Smi::FromInt(0)) or SmiTest(result.reg()) instead (it
will use testq for you, but if the representation changes, your code will still
work).
In general: Never assume the smi representation in x64 code, and never work
directly on smis, but use the MacroAssemblerX64 functions instead.

http://codereview.chromium.org/1627014/diff/1/2#newcode5374
src/x64/codegen-x64.cc:5374:
static_cast<uint8_t>(String::cast(*right_side.handle())->Get(0));
How do we know that the right-hand side is an ASCII string?
Otherwise this cast loses information.

http://codereview.chromium.org/1627014/diff/1/2#newcode5390
src/x64/codegen-x64.cc:5390: // Compare the first character of the string with
out constant
out->our

http://codereview.chromium.org/1627014/diff/1/2#newcode5393
src/x64/codegen-x64.cc:5393:
static_cast<uint8_t>(String::cast(*right_side.handle())->Get(0));
Ditto for this cast.

http://codereview.chromium.org/1627014/diff/1/2#newcode5400
src/x64/codegen-x64.cc:5400: Immediate(1));
This is probably a rare case, but isn't there some register you can use instead
to avoid re-reading the length field here? If you could store the result of the
subl in line 5381, you could just test that directly.

[William Hesse, 2010-04-13 12:29:00]

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

http://codereview.chromium.org/1627014/diff/1/2#newcode5335
src/x64/codegen-x64.cc:5335: // They are symbols, so do identity compare.
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Are all one-character strings symbols? 
> Otherwise, where did we check that the rhs is a symbol?
> 
Yes, the constant one-character strings from the function's source code are
symbols.  Corrected comment above to say that.

http://codereview.chromium.org/1627014/diff/1/2#newcode5338
src/x64/codegen-x64.cc:5338: dest->false_target()->Branch(not_equal);
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Second branch can be unconditional.

But then the jump-target semantics leave no fall-through frame to compile with. 
I had a proposed change to add the condition "always" to those accepted by
Branch, so it would emit a jump, but leave the frame as a fall-through. 
Alternatively, we can use a JumpTarget for not_a_symbol.

http://codereview.chromium.org/1627014/diff/1/2#newcode5341
src/x64/codegen-x64.cc:5341: // If the receiver is not a string of the type we
handle call the stub.
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Comma after "handle"?

Done.

http://codereview.chromium.org/1627014/diff/1/2#newcode5342
src/x64/codegen-x64.cc:5342: __ andl(temp.reg(),
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Can we use andb instead? I should make the immediate 8-bit instead of 32.
The immediate should be 8-bit regardless of whether the operand size is 32 or 8.

http://codereview.chromium.org/1627014/diff/1/2#newcode5343
src/x64/codegen-x64.cc:5343: Immediate(kIsNotStringMask |
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Indentation.

Done.

http://codereview.chromium.org/1627014/diff/1/2#newcode5356
src/x64/codegen-x64.cc:5356: __ testq(result.reg(), result.reg());
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Don't compare a smi to 0 like this. It assumes the smi representation. Use
> SmiCompare(result.reg(), Smi::FromInt(0)) or SmiTest(result.reg()) instead (it
> will use testq for you, but if the representation changes, your code will
still
> work).
> In general: Never assume the smi representation in x64 code, and never work
> directly on smis, but use the MacroAssemblerX64 functions instead.

Done.

http://codereview.chromium.org/1627014/diff/1/2#newcode5374
src/x64/codegen-x64.cc:5374:
static_cast<uint8_t>(String::cast(*right_side.handle())->Get(0));
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> How do we know that the right-hand side is an ASCII string?
> Otherwise this cast loses information.

Good catch.  We now test this when computing the bool
{left,right}_side_constant_1_char_string.

http://codereview.chromium.org/1627014/diff/1/2#newcode5390
src/x64/codegen-x64.cc:5390: // Compare the first character of the string with
out constant
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> out->our

Done.

http://codereview.chromium.org/1627014/diff/1/2#newcode5393
src/x64/codegen-x64.cc:5393:
static_cast<uint8_t>(String::cast(*right_side.handle())->Get(0));
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> Ditto for this cast.

Done.

http://codereview.chromium.org/1627014/diff/1/2#newcode5400
src/x64/codegen-x64.cc:5400: Immediate(1));
On 2010/04/13 08:25:01, Lasse Reichstein wrote:
> This is probably a rare case, but isn't there some register you can use
instead
> to avoid re-reading the length field here? If you could store the result of
the
> subl in line 5381, you could just test that directly.

Nowhere else in this inline code do we use more than one temp register, so this
could cause a spill.  Also, the load may not be expensive, since it repeats a
load done just before.  Rearranging the code is tricky, since the flags
resulting from two comparisons are merged here, and then used by the comparison.

[Lasse Reichstein, 2010-04-13 12:53:16]

LGTM

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

http://codereview.chromium.org/1627014/diff/1/2#newcode5342
src/x64/codegen-x64.cc:5342: __ andl(temp.reg(),
You are right that there is a case for an 8-bit immediate value with a 32-bit
operand, but it's only for signed 8-bit immediate values (-128..127). This
immediate here is >= 128, so that case does not apply.

http://codereview.chromium.org/1627014/diff/5001/6002
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1627014/diff/5001/6002#newcode2775
src/ia32/codegen-ia32.cc:2775: __ test(answer.reg(), Operand(answer.reg()));
These two tests seem to assume that 0 is Smi::FromInt(0).
Also, doing test(r,r) and cmp(r,0) are completely equivalent, so the branch
isn't needed.
Could you change them to just a SmiTest(answer.reg());

[William Hesse, 2010-04-13 13:42:22]

> You are right that there is a case for an 8-bit immediate value with a 32-bit
> operand, but it's only for signed 8-bit immediate values (-128..127). This
> immediate here is >= 128, so that case does not apply.

OK, changed.

> http://codereview.chromium.org/1627014/diff/5001/6002#newcode2775
> src/ia32/codegen-ia32.cc:2775: __ test(answer.reg(), Operand(answer.reg()));
> These two tests seem to assume that 0 is Smi::FromInt(0).
> Also, doing test(r,r) and cmp(r,0) are completely equivalent, so the branch
> isn't needed.
> Could you change them to just a SmiTest(answer.reg());
That is an ia32 file.  We don't have SmiTest on that platform.  Removed the
branch.