X64 Crankshaft: Added a bunch of operations.

X64 Crankshaft: Added a bunch of operations.

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

[William Hesse, 2011-01-24 16:25:27]

Add the instructions to the disassembler.

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

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode396
src/x64/assembler-x64.h:396: bool depends_on_register(Register reg) const {
Could you call it address_uses_register() ?

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode415
src/x64/assembler-x64.h:415: // Comparison uses REX.B if using the SIB byte,
otherwise not.
instead of "if using the SIB byte", say "if base_code came from the SIB byte". 
Maybe it is simpler if you have only base_code = buf_[0] & 0x7, test against
0x05, then test against 0x04, and finally inside SIB byte test the 4-bit bit
code against 0x05 again.  Then there is no need for the base_bits variable.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode851
src/x64/assembler-x64.h:851: void andl(Register dst, const Operand& src) {
Add to disassembler.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1239: void movdqa(const Operand& dst, XMMRegister src);
Add to disassembler.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1282: // Use either movsd or movlpd.
I commented these, but did not remove them.  We should remove them.

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

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1157: __ movl(result,
Immediate(Heap::kFalseValueRootIndex));
Why not move True and False directly to the result?  That is just
Operand(kRootRegister, some offset), so I don't see why two loads is better than
one.  Are you thinking that you are getting an extra memory load in one of the
two cases?  Then move the branch before the load, and add another branch to
done.  Do you know this is faster than simpler ways?

How do you know that if a load from memory is is immediately killed by another
load, that it is actually carried out?  Maybe it isn't.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1331: Heap::kTrueValueRootIndex));
I think you need to multiply the kTrueValueRootIndex by kPointerSize.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1805: __ cvtlsi2sd(ToDoubleRegister(output),
ToOperand(input));
Operand cannot represent a register, so you need to write two cases: a (memory)
Operand, and a Register.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1848: __ Set(reg, 0);
How can 0 be a valid pointer?  It isn't tagged.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1901: __ Integer32ToSmi(input_reg, input_reg); 
// Retag smi.
Can't we have some way to make this optional, or use kScratchRegister, so that
we don't do unneccesary work (retagging the Smi).

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

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:995: movl(dst, src);
Why is the second choice worst than the first.  Why not just do the second
choice all the time?

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1840: const int kShaddowSpace = 4;
Shadow has one d.  What is shadow space?

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1852: movdqa(Operand(rbp, offset - ((i + 1) *
kDoubleSize)), reg);
How do we know that we are 16-byte aligned here?  I think we are only 8-byte
aligned.  The frame is aligned below.

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1855: subq(rsp, Immediate(arg_stack_space *
kPointerSize));
This code is duplicated.  It seems to run twice- a bad idea.

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1900: movdqa(reg, Operand(rbp, offset - ((i + 1)
* kDoubleSize)));
Is this the right rbp here?

[Rico, 2011-01-25 07:13:28]

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

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode405
src/x64/assembler-x64.h:405: // Index code (including REX.X) of 0x04 (esp) means
no index register.
esp -> rsp

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode406
src/x64/assembler-x64.h:406: if (index_code != 0x04 && index_code == code)
return true;
What about using rsp.code() instead of 0x04

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

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1285: Register temp =
ToRegister(instr->TempAt(0));
No need to the temp register here (and in the lithium-x64 file) - just use r10

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1467: Register temp =
ToRegister(instr->TempAt(0));
Use r10?

[William Hesse, 2011-01-25 09:22:37]

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

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1257: Label* is_not_object,
This function should drop the temp2 argument, use kScratchRegister.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1265: __ Cmp(input, Factory::null_value());
Can we use the root array here?

[Lasse Reichstein, 2011-01-25 10:13:27]

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

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode396
src/x64/assembler-x64.h:396: bool depends_on_register(Register reg) const {
It should probably even be AddressUsesRegister.
And it's a big function. I'll move it to the .cc file.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode405
src/x64/assembler-x64.h:405: // Index code (including REX.X) of 0x04 (esp) means
no index register.
On 2011/01/25 07:13:28, Rico wrote:
> esp -> rsp

Done.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode406
src/x64/assembler-x64.h:406: if (index_code != 0x04 && index_code == code)
return true;
Done, and also rbp.code() instead of 0x05 later.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode415
src/x64/assembler-x64.h:415: // Comparison uses REX.B if using the SIB byte,
otherwise not.
I have rewritten it so the two branches have no shared code. They now differ on
when the REX.B bit is added to the base_code value (before or after testing for
r/m=rbp,mod=0).

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode851
src/x64/assembler-x64.h:851: void andl(Register dst, const Operand& src) {
This one is already there (it's one of the fairly generic "arithmetic"
operations, where we are just adding another case).

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1239: void movdqa(const Operand& dst, XMMRegister src);
On 2011/01/24 16:25:27, William Hesse wrote:
> Add to disassembler.

Done.

http://codereview.chromium.org/6366010/diff/1/src/x64/assembler-x64.h#newcode...
src/x64/assembler-x64.h:1282: // Use either movsd or movlpd.
On 2011/01/24 16:25:27, William Hesse wrote:
> I commented these, but did not remove them.  We should remove them.

Done.

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

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1157: __ movl(result,
Immediate(Heap::kFalseValueRootIndex));
The change to the code relative to ia32 is because the ia32 code does two loads.
In ia32, the loads are immediate. This was the simplest change of that code to
use the root array.

I have rewritten the code to use two labels and one direct root-array load per
branch.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1285: Register temp =
ToRegister(instr->TempAt(0));
Removed, and EmitIsObject rewritten to use only kScratchRegister as temporary.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1331: Heap::kTrueValueRootIndex));
Well spotted.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1467: Register temp =
ToRegister(instr->TempAt(0));
This temp is needed in EmitClassOfTest. It uses the temp register in or through
macros that clobber kScratchRegister.

Remember kScratchRegister is the macro assembler's scratch register. It's not
guaranteed to survive past any macro that doesn't say that it doesn't clobber
it.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1805: __ cvtlsi2sd(ToDoubleRegister(output),
ToOperand(input));
We assert just above that input is not a register (as does the ia32 code).

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1848: __ Set(reg, 0);
By chance, since it's the representation of Smi zero.
I'll make it explicit, and use Move(reg, Smi::fromInt(0));
It'll emit the same code anyway (xorl(reg,reg)).

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1901: __ Integer32ToSmi(input_reg, input_reg); 
// Retag smi.
Using kScratchRegister seems like the easy way. We still have to untag it first.
Done.

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

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:995: movl(dst, src);
The second choice has a following operation that depends on a memory load. The
first choice's movl is independent of other operations and can be hoisted to
before the result is ready in src, leaving only the memory operation.
So, I'm expecting marginally better parallelization.
It might be pure speculation though.

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1840: const int kShaddowSpace = 4;
I believe it's the space reserved on the stack for the register-passed arguments
in the Win64 calling convention.
The name 'shadow space' is used by the MSDN documentation, e.g.,
http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx
The double-d is inexcusable though.

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1852: movdqa(Operand(rbp, offset - ((i + 1) *
kDoubleSize)), reg);
Ack, I don't. We are only 8-byte aligned. Using movdqa is bad. Will change to
movsd.

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1855: subq(rsp, Immediate(arg_stack_space *
kPointerSize));
Argh, removed.

http://codereview.chromium.org/6366010/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:1900: movdqa(reg, Operand(rbp, offset - ((i + 1)
* kDoubleSize)));
It seems so. It's equivalent to the ia32 code.

[Lasse Reichstein, 2011-01-25 10:17:50]

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

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1257: Label* is_not_object,
Dropped both. They seem to be used sequentially, and if using kScratchRegister,
there's no problem with either being input.

http://codereview.chromium.org/6366010/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:1265: __ Cmp(input, Factory::null_value());
Done.

[William Hesse, 2011-01-25 11:23:24]

LGTM.