Fix SmiCompare on 64 bit to distinguish between comparisons where...

Fix SmiCompare on 64 bit to distinguish between comparisons where
we know that both sides are Smi and those where we don't.  Fix inlined
symbol table probes to cope with strings, undefined and null (indicating
a deleted entry).  Some changes to other architectures that were found
with the new asserts.

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

[Lasse Reichstein, 2011-03-14 08:51:01]

http://codereview.chromium.org/6682026/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/assembler-x64.cc#newcod...
src/x64/assembler-x64.cc:1603: movq(dst, reinterpret_cast<int64_t>(value),
RelocInfo::NONE);
Please consider using LoadSmi instead. It tries to optimize small smis by lea's
on kSmiConstantRegister, and otherwise defaults to loading it verbatim like
this.

http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2254: Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR));
You can use Cmp(const Operand* dst, Handle<Object> source) here. If you don't
want to make a handle, consider renaming this to just Cmp(..Operand.., Smi*).
The "SmiX" operations all work on operands that are both smis.

http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:4562: __ nop();
Why the nop?

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

http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc#newcode2249
src/x64/codegen-x64.cc:2249: __ cmpq(left_side.reg(), right_side.reg());
Why not SmiCompare? You are assuming the layout of smis outside of Smi code,
please try to avoid that (if it really seems necessary, maybe it's reason to
introduce another Smi-macro). Only use cmpq for pointer-equality test, not any
of greater/less than.

http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc#newcode6810
src/x64/codegen-x64.cc:6810: deferred->Branch(below_equal);
Do use SmiCompare!

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

http://codereview.chromium.org/6682026/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3035: __ cmpq(temp, index_2);
Do use SmiCompare.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (left):

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:841: cmpq(dst, src);
Don't remove the SmiCompare function. Do use it when you compare smis for
anything but equality!
In general, don't make assumptions (in genrated code) about the representation
of smis outside of the MacroAssembler smi code.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:865: }
Adding asserts: Good!

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:893: cmpq(dst, kScratchRegister);
This is wasteful, since it doesn't use the kSmiConstantRegister for the values
where it can be used.
It's better to use:
  Register const_reg = GetSmiConstant(kScratchRegister, src);
  cmpq(dst, const_reg);
as in SmiCompare(Register,Smi*).

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:281: // Simple comparison of Smis.
Actually, I believe we try to write "smi" as lower-case when referring to the
concept, not the Class.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:288: void SmiCompareWithObject(const Operand& dst,
Smi* src);
Rename them to Cmp and group them with the Cmp functions instead.

[Erik Corry, 2011-03-14 16:26:45]

New version uploaded

http://codereview.chromium.org/6682026/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/assembler-x64.cc#newcod...
src/x64/assembler-x64.cc:1603: movq(dst, reinterpret_cast<int64_t>(value),
RelocInfo::NONE);
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Please consider using LoadSmi instead. It tries to optimize small smis by
lea's
> on kSmiConstantRegister, and otherwise defaults to loading it verbatim like
> this.

Used LoadSmiConstant

http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:2254: Smi::FromInt(StackFrame::ARGUMENTS_ADAPTOR));
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> You can use Cmp(const Operand* dst, Handle<Object> source) here. If you don't
> want to make a handle, consider renaming this to just Cmp(..Operand.., Smi*).
> The "SmiX" operations all work on operands that are both smis.

Renamed to Cmp

http://codereview.chromium.org/6682026/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:4562: __ nop();
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Why the nop?

Done.

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

http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc#newcode2249
src/x64/codegen-x64.cc:2249: __ cmpq(left_side.reg(), right_side.reg());
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Why not SmiCompare? You are assuming the layout of smis outside of Smi code,
> please try to avoid that (if it really seems necessary, maybe it's reason to
> introduce another Smi-macro). Only use cmpq for pointer-equality test, not any
> of greater/less than.

Done.

http://codereview.chromium.org/6682026/diff/1/src/x64/codegen-x64.cc#newcode6810
src/x64/codegen-x64.cc:6810: deferred->Branch(below_equal);
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Do use SmiCompare!

Done.

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

http://codereview.chromium.org/6682026/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3035: __ cmpq(temp, index_2);
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Do use SmiCompare.

Done.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (left):

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:841: cmpq(dst, src);
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Don't remove the SmiCompare function. Do use it when you compare smis for
> anything but equality!
> In general, don't make assumptions (in genrated code) about the representation
> of smis outside of the MacroAssembler smi code.

Done.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:865: }
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Adding asserts: Good!

Done.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:893: cmpq(dst, kScratchRegister);
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> This is wasteful, since it doesn't use the kSmiConstantRegister for the values
> where it can be used.
> It's better to use:
>   Register const_reg = GetSmiConstant(kScratchRegister, src);
>   cmpq(dst, const_reg);
> as in SmiCompare(Register,Smi*).

Done.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:281: // Simple comparison of Smis.
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Actually, I believe we try to write "smi" as lower-case when referring to the
> concept, not the Class.

Done.

http://codereview.chromium.org/6682026/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:288: void SmiCompareWithObject(const Operand& dst,
Smi* src);
On 2011/03/14 08:51:01, Lasse Reichstein wrote:
> Rename them to Cmp and group them with the Cmp functions instead.

Done.

[Lasse Reichstein, 2011-03-15 09:07:01]

LGTM with a few fixes.

http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:6017: // Must be null (deleted entry).
How about adding some debug code to test that it is null?

http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:6022: // Check that the candidate is a non-external
ascii string.  The instance
ASCII is an acronym.

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

http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:7209: 
Add to the comment that we know the length is a smi because we tested the array
to have fast elements above.

http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:7209: 
This code now clobbers scratch1 which is assumed to be the elements fixed-array
below.

http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3052: __ CmpObjectType(object, JS_OBJECT_TYPE,
temp);
JS_ARRAY_TYPE

http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h#newc...
src/x64/assembler-x64.h:659: // Instructions to load a 64-bit immediate into a
register.
Good change.
Never name something "new X". It gets old fast :)

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:127: // catch stores of smis and stores into
young gen.
gen -> generation.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:156: // catch stores of smis and stores into
young gen.
ditto.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:843: AbortIfNotSmi(smi2);
Good stuff!

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:855: Cmp(dst, src);
... apart from the optimizations that Cmp also does.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:897: // The Operand cannot use the smi register,
since we may use the scratch
Notice that smi_reg is either kScratchRegister or kSmiConstantRegister in this
case.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object);
Negate condition?

http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assem...
File test/cctest/test-macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assem...
test/cctest/test-macro-assembler-x64.cc:223: __ cmpq(rcx, rcx);
Really should be SmiCompare in this function.

[Lasse Reichstein, 2011-03-15 09:08:19]

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object);
Ignore me, this is correct.

[Erik Corry, 2011-03-15 10:00:50]

http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:6017: // Must be null (deleted entry).
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> How about adding some debug code to test that it is null?

Done.

http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:6022: // Check that the candidate is a non-external
ascii string.  The instance
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> ASCII is an acronym.

Done.

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

http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:7209: 
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> This code now clobbers scratch1 which is assumed to be the elements
fixed-array
> below.

Nice catch.  The tests missed this because this whole file is dead code in the
Cranshaft VM.

http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:7209: 
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Add to the comment that we know the length is a smi because we tested the
array
> to have fast elements above.

Done.

http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3052: __ CmpObjectType(object, JS_OBJECT_TYPE,
temp);
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> JS_ARRAY_TYPE

Done.

http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h#newc...
src/x64/assembler-x64.h:659: // Instructions to load a 64-bit immediate into a
register.
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Good change.
> Never name something "new X". It gets old fast :)

yup.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:127: // catch stores of smis and stores into
young gen.
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> gen -> generation.

Done.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:156: // catch stores of smis and stores into
young gen.
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> ditto.

Done.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:843: AbortIfNotSmi(smi2);
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Good stuff!

yup

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:855: Cmp(dst, src);
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> ... apart from the optimizations that Cmp also does.

Comment deleted.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:897: // The Operand cannot use the smi register,
since we may use the scratch
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Notice that smi_reg is either kScratchRegister or kSmiConstantRegister in this
> case.

Comment updated.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object);
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Negate condition?

Done.

http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object);
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Negate condition?

Not done.

http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assem...
File test/cctest/test-macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assem...
test/cctest/test-macro-assembler-x64.cc:223: __ cmpq(rcx, rcx);
On 2011/03/15 09:07:01, Lasse Reichstein wrote:
> Really should be SmiCompare in this function.

Since we are only testing for equality cmpq seems fine.

[Lasse Reichstein, 2011-03-15 12:29:23]

http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assem...
File test/cctest/test-macro-assembler-x64.cc (right):

http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assem...
test/cctest/test-macro-assembler-x64.cc:223: __ cmpq(rcx, rcx);
But we are testing SmiCompare :)