X64 implementation: comparison operations.

X64 implementation: comparison operations.

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

[Mads Ager (chromium), 2009-06-24 13:27:27]

LGTM

http://codereview.chromium.org/146082/diff/1012/17
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/146082/diff/1012/17#newcode1086
Line 1086: const Operand& dst,
Indentation seems off.

http://codereview.chromium.org/146082/diff/1012/17#newcode1089
Line 1089: Register dst,
Identation again.

http://codereview.chromium.org/146082/diff/1012/19
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/146082/diff/1012/19#newcode1630
Line 1630: __ testq(answer.reg(), Immediate(kSmiTagMask));
testl to make it consistent with the rest of the code (after my changes).

http://codereview.chromium.org/146082/diff/1012/19#newcode2470
Line 2470: __ testq(left_side.reg(), Immediate(kSmiTagMask));
testl

http://codereview.chromium.org/146082/diff/1012/19#newcode2516
Line 2516: __ testq(operand.reg(), Immediate(kSmiTagMask));
testl

http://codereview.chromium.org/146082/diff/1012/19#newcode2564
Line 2564: __ testq(temp.reg(), Immediate(kSmiTagMask));
testl

http://codereview.chromium.org/146082/diff/1012/19#newcode3742
Line 3742: __ movl(rax, FieldOperand(rdx, HeapNumber::kValueOffset));
movq?

http://codereview.chromium.org/146082/diff/1012/19#newcode3907
Line 3907: __ movl(kScratchRegister, FieldOperand(object,
HeapObject::kMapOffset));
movq?

[Lasse Reichstein, 2009-06-24 14:29:15]

http://codereview.chromium.org/146082/diff/1012/14
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/146082/diff/1012/14#newcode525
Line 525: // Use 64-bit mode byte registers.
Do you expect this to happen again enough that we should make a function for it?

http://codereview.chromium.org/146082/diff/1012/17
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/146082/diff/1012/17#newcode1086
Line 1086: const Operand& dst,
Indentation is off-by-one (this function signature and next).

http://codereview.chromium.org/146082/diff/1012/19
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/146082/diff/1012/19#newcode1622
Line 1622: Handle<String> check(String::cast(*right->AsLiteral()->handle()));
Is there a reason this is not:
Handle<String> check(Handle<String>::cast(right->AsLiteral()->handle()));
That should avoid creating a new handle location when one already exists.

http://codereview.chromium.org/146082/diff/1012/19#newcode1630
Line 1630: __ testq(answer.reg(), Immediate(kSmiTagMask));
Use testl instead of testq (four more after this).

http://codereview.chromium.org/146082/diff/1012/19#newcode2470
Line 2470: __ testq(left_side.reg(), Immediate(kSmiTagMask));
testl

http://codereview.chromium.org/146082/diff/1012/19#newcode2516
Line 2516: __ testq(operand.reg(), Immediate(kSmiTagMask));
testl

http://codereview.chromium.org/146082/diff/1012/19#newcode2564
Line 2564: __ testq(temp.reg(), Immediate(kSmiTagMask));
... and testl again (there might be more).

http://codereview.chromium.org/146082/diff/1012/19#newcode2573
Line 2573: __ cmpq(answer.reg(), Immediate(0));
Which flags are set differently by testq(reg,reg) and cmpq(reg,0)?

http://codereview.chromium.org/146082/diff/1012/19#newcode3840
Line 3840: // TODO(1243847): Use cmov below once CpuFeatures are properly hooked
up.
cmov is always enabled in X64.

http://codereview.chromium.org/146082/diff/1012/19#newcode3842
Line 3842: // use edx, eax to convert unsigned to signed comparison
edx, rax

[William Hesse, 2009-06-25 15:31:46]

http://codereview.chromium.org/146082/diff/1012/17
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/146082/diff/1012/17#newcode1086
Line 1086: const Operand& dst,
On 2009/06/24 13:27:27, Mads Ager wrote:
> Indentation seems off.

Done.

http://codereview.chromium.org/146082/diff/1012/17#newcode1089
Line 1089: Register dst,
On 2009/06/24 13:27:27, Mads Ager wrote:
> Identation again.

Done.

http://codereview.chromium.org/146082/diff/1012/19
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/146082/diff/1012/19#newcode1630
Line 1630: __ testq(answer.reg(), Immediate(kSmiTagMask));
On 2009/06/24 13:27:27, Mads Ager wrote:
> testl to make it consistent with the rest of the code (after my changes).

Done.  Changed everywhere it occurs.

http://codereview.chromium.org/146082/diff/1012/19#newcode3742
Line 3742: __ movl(rax, FieldOperand(rdx, HeapNumber::kValueOffset));
On 2009/06/24 13:27:27, Mads Ager wrote:
> movq?

Done.

http://codereview.chromium.org/146082/diff/1012/19#newcode3907
Line 3907: __ movl(kScratchRegister, FieldOperand(object,
HeapObject::kMapOffset));
On 2009/06/24 13:27:27, Mads Ager wrote:
> movq?

Done.