X64 Crankshaft: Add bit operations and shifts to x64 crankshaft.

X64 Crankshaft: Add bit operations and shifts to x64 crankshaft.

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

[Lasse Reichstein, 2011-02-04 09:51:24]

LGTM

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

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:679: __ testl(ToRegister(left),
Immediate(0x80000000));
Seems wasteful to compare to a 4-byte literal.
Just do 
  testl(ToRegister(left), ToRegister(left));
  DeoptimizeIf(negative, ...)

Maybe cache the result of ToRegister(left), instead of calling it multiple
times.

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:701: __ testl(ToRegister(left),
Immediate(0x80000000));
Test to self and deopt if negative here too.

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

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-x64.cc#newcode762
src/x64/lithium-x64.cc:762: LOperand* left = UseFixed(instr->left(), rdx);
Why fixed registers? Is it to match the calling convention of the binary op
stub?

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-x64.cc#newcode799
src/x64/lithium-x64.cc:799: // by 0 and the result cannot be truncated to int32.
It's a little tricky to have a constant value that happens to be zero when the
operand isn't constant. The "constant_value" variable should be meaningless in
that case.

How about a binary flag that says 
 bool can_deopt = (op == Token::SHR);
and inside if (right_value->IsConstant), set it to false if the constant is
non-zero.

[William Hesse, 2011-02-04 14:37:12]

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

http://codereview.chromium.org/6246099/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:263: }
Indentation fixed.

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:679: __ testl(ToRegister(left),
Immediate(0x80000000));
On 2011/02/04 09:51:25, Lasse Reichstein wrote:
> Seems wasteful to compare to a 4-byte literal.
> Just do 
>   testl(ToRegister(left), ToRegister(left));
>   DeoptimizeIf(negative, ...)
> 
> Maybe cache the result of ToRegister(left), instead of calling it multiple
> times.

Done.

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:701: __ testl(ToRegister(left),
Immediate(0x80000000));
On 2011/02/04 09:51:25, Lasse Reichstein wrote:
> Test to self and deopt if negative here too.

Done.

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

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-x64.cc#newcode762
src/x64/lithium-x64.cc:762: LOperand* left = UseFixed(instr->left(), rdx);
Yes.
On 2011/02/04 09:51:25, Lasse Reichstein wrote:
> Why fixed registers? Is it to match the calling convention of the binary op
> stub?

http://codereview.chromium.org/6246099/diff/5/src/x64/lithium-x64.cc#newcode799
src/x64/lithium-x64.cc:799: // by 0 and the result cannot be truncated to int32.
This is the way it is done on all the other platforms.
On 2011/02/04 09:51:25, Lasse Reichstein wrote:
> It's a little tricky to have a constant value that happens to be zero when the
> operand isn't constant. The "constant_value" variable should be meaningless in
> that case.
> 
> How about a binary flag that says 
>  bool can_deopt = (op == Token::SHR);
> and inside if (right_value->IsConstant), set it to false if the constant is
> non-zero.