X64 Crank: Implemented DoBranch and all *AndBranch comparisons.

X64 Crank: Implemented DoBranch and all *AndBranch comparisons.

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

[William Hesse, 2011-01-18 14:01:50]

LGTM, with comments.

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

http://codereview.chromium.org/6374002/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:4333: __ sbbl(rax, rcx);  // Subtract one if below
(aka. carry set).
Should be a 64-bit subtraction, to return a 64-bit signed number in rax.

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

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1035: inline Condition
LCodeGen::TokenToCondition(Token::Value op, bool is_unsigned) {
If this is inline, it should be defined in the header file, rather than here,
no?

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1066: __ Cmp(ToRegister(left),
ToHandle(LConstantOperand::cast(right)));
Shouldn't you use Immediate(Int32Value(LConstantOperand::cast(right)))?

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1279: Register temp =
ToRegister(instr->TempAt(0));
Unless we are using instr->temp after this instruction finishes, shouldn't we
use kScratchRegister, and not use a temp in the instruction?

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1344: __ CmpObjectType(temp, JS_FUNCTION_TYPE,
temp2);
We can use kScratchRegister here.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1356: // The class name we are testing against is
a symbol because it's a literal.
Could we assert that this is a symbol?

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1379: temp = temp2;
We probably don't need two temp registers here.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1847: __ Cmp(input,
Handle<Object>(Heap::true_value()));
Can we use the root array here?

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1852: } else if
(type_name->Equals(Heap::undefined_symbol())) {
Root array.

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

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-x64.cc#newcode924
src/x64/lithium-x64.cc:924: return new LCmpIDAndBranch(UseRegisterAtStart(left),
I think Integer32 can be handled by the CmpIAndBranch, not requiring doubles on
x64.

[Lasse Reichstein, 2011-01-19 09:20:35]

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

http://codereview.chromium.org/6374002/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:4333: __ sbbl(rax, rcx);  // Subtract one if below
(aka. carry set).
Done. Well spotted.

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

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1035: inline Condition
LCodeGen::TokenToCondition(Token::Value op, bool is_unsigned) {
It's only used in this file, so it's declared here.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1066: __ Cmp(ToRegister(left),
ToHandle(LConstantOperand::cast(right)));
I probably should. We know that the constant is a 32-bit value at this point. 
Done.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1279: Register temp =
ToRegister(instr->TempAt(0));
It should be safe. We only use it as scratch register for CmpObjectType, and
kScratchRegister can be used there.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1344: __ CmpObjectType(temp, JS_FUNCTION_TYPE,
temp2);
We can, but the current implementation uses the fact that it has two temp
registers to ensure that "temp" isn't the same register as "input".

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1356: // The class name we are testing against is
a symbol because it's a literal.
Done.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1379: temp = temp2;
This is the place where we seem that we do need them - in order to ensure that
"temp" isn't the same register as "input". I don't know if there is another way
to ensure that we have another register.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1847: __ Cmp(input,
Handle<Object>(Heap::true_value()));
Done, both.

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1852: } else if
(type_name->Equals(Heap::undefined_symbol())) {
Done.

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

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-x64.cc#newcode924
src/x64/lithium-x64.cc:924: return new LCmpIDAndBranch(UseRegisterAtStart(left),
I don't believe there is a CmpIAndBranch, only a CmpIDAndBranch. It will do the
right thing if both operands are integers.

[Lasse Reichstein, 2011-01-19 10:14:53]

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

http://codereview.chromium.org/6374002/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1344: __ CmpObjectType(temp, JS_FUNCTION_TYPE,
temp2);
I removed the second temp register, and just stopped claiming (incorrectly) that
the function doesn't clobber input.
The above line would actually clobber input if it is aliased to temp2.