X64: Added jmp and call and nop(n) to X64 assembler.

X64: Added jmp and call and nop(n) to X64 assembler.

[Lasse Reichstein, 2009-05-29 10:43:23]

Small review.

[William Hesse, 2009-05-29 11:15:00]

http://codereview.chromium.org/115920/diff/1/2
File src/ia32/assembler-ia32.cc (right):

http://codereview.chromium.org/115920/diff/1/2#newcode1491
Line 1491: emit_rex_64(adr);
I don't think this belongs in the ia32 compiler.

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

http://codereview.chromium.org/115920/diff/1/3#newcode76
Line 76: emit(0x48 | (reg.code() >> 3));
I already have these in my changelist.  I implement them by inline calling
emit_rex_64(rax, reg);.  I think this is simpler.  Why the b?

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

http://codereview.chromium.org/115920/diff/1/4#newcode387
Line 387: emit(0xd0 | (adr.code() & 0x07));
Good.

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

http://codereview.chromium.org/115920/diff/1/5#newcode657
Line 657: // void call(const Operand& adr);
I would wait to declare this until you need it.

http://codereview.chromium.org/115920/diff/1/5#newcode665
Line 665: // void call(Handle<Code> code, RelocInfo::Mode rmode);
I would not create this function.  When will you have a safe use for it?

http://codereview.chromium.org/115920/diff/1/5#newcode672
Line 672: // void jmp(const Operand& adr);
I wouldn't include unless needed.

http://codereview.chromium.org/115920/diff/1/5#newcode679
Line 679: // void jmp(Address entry, RelocInfo::Mode rmode);
I don't think these should exist.  Where is a safe use of them?

http://codereview.chromium.org/115920/diff/1/5#newcode839
Line 839: void emit_rex_64(Register rm_reg) { emit_rex_64(rax, rm_reg); }
This is the one you are replacing with emit_rex_64b.  Why?

http://codereview.chromium.org/115920/diff/1/5#newcode853
Line 853: 
Why is the separate implementation of this function better?

[Kevin Millikin (Chromium), 2009-05-29 11:44:37]

Fast progress on the assembler.  Some drive-by comments.

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

http://codereview.chromium.org/115920/diff/1/4#newcode600
Line 600: void Assembler::nop(int n) {
It is unclear how you are implemenenting nops of various lenghts.  Please put in
comments the instructions that are actually emitted as comments and/or cite the
processor manual.

I disagree in principle with having the assembler do much more than emit exactly
the instruction specified.  Nop(int n) or EmitNop(int n) should arguabley be in
the macro assembler since we have it.

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

http://codereview.chromium.org/115920/diff/1/5#newcode231
Line 231: times_8 = 3,
The x64 assembler implementation should follow the style guide (can you think of
a good reason it shouldn't?).

If you're worried about making it hard to cut and paste x86 code, then changes
could be backported to the x86 assembler.

This should be TIMES_8 or (preferred) kTimes8.

http://codereview.chromium.org/115920/diff/1/5#newcode657
Line 657: // void call(const Operand& adr);
I agree with Bill.  It will be (obviously) implemented if needed, so the comment
seems to have the force of a cautionary warning against implementing this when
*not* needed.

I'm not sure why we need to worry about that, or call out this specific
signature as something not to implement.

[Lasse Reichstein, 2009-05-29 12:27:14]

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

http://codereview.chromium.org/115920/diff/1/3#newcode76
Line 76: emit(0x48 | (reg.code() >> 3));
I agree that there should be only one implementation.
I would prefer it to be the one taking a numeric argument, since I don't like
converting a number to a Register when the number has nothing to do with
registers.

The "b" is because it's not obvious from the arguments that the register's top
bit goes to REX.B. It could just as well have gone to REX.R.

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

http://codereview.chromium.org/115920/diff/1/4#newcode600
Line 600: void Assembler::nop(int n) {
Will refer to manual (these opcodes are taken verbatim from there).

I'm split on moving this to the macro assembler, since I wouldn't want to have
emits directly in there.
We have other cases where there is logic dispatching on the arguments. I think
this is not too far away from that.

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

http://codereview.chromium.org/115920/diff/1/5#newcode231
Line 231: times_8 = 3,
This is a good time to break with history (before any code is written using it).
I'll go for kTimes8.

http://codereview.chromium.org/115920/diff/1/5#newcode657
Line 657: // void call(const Operand& adr);
Agree. It's purely documentation now, saying that this is possible but we can't
be bothered to do it yet. No need for that. Will remove.

http://codereview.chromium.org/115920/diff/1/5#newcode665
Line 665: // void call(Handle<Code> code, RelocInfo::Mode rmode);
Never with the current memory model.
If we ever create a specialized compiler for a smaller memory model, it might be
relevant, but we should add it at that time, now now.

http://codereview.chromium.org/115920/diff/1/5#newcode839
Line 839: void emit_rex_64(Register rm_reg) { emit_rex_64(rax, rm_reg); }
Only because it didn't give a problem when merging.
I put the "b" after emit_rex_64b to show that it puts the register bit in REX.B.

It turns out there will be no equivalent operation for REX.R, so the extra
specification is irrelevant. I'll merge the two functions.

[William Hesse, 2009-05-29 13:08:00]

LGTM.  Comments are optional changes.

http://codereview.chromium.org/115920/diff/1008/1010
File src/x64/assembler-x64-inl.h (right):

http://codereview.chromium.org/115920/diff/1008/1010#newcode77
Line 77: emit(0x48 | (rm_reg.code() >> 3));
There was never a good reason for putting these in -inl-h, so these and the two
above should be in the .h file.
But it is ok to leave them here too.

http://codereview.chromium.org/115920/diff/1008/1011
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/115920/diff/1008/1011#newcode275
Line 275: ASSERT_EQ(rm & 0x07, rm);
I think that rm should be anded with 0x7 always, so that it never generates a
bad modRM byte.  This only works in debug builds.  Then you wouldn't need it in
the register version.

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

http://codereview.chromium.org/115920/diff/1008/1012#newcode868
Line 868: emit_operand(reg.code() & 0x07, adr);
I would really like to put the & 0x7 into the base function.  I think
emit_operand(int, Operand) should be able to take a register code as its int
argument.

[Lasse Reichstein, 2009-05-29 13:13:29]

http://codereview.chromium.org/115920/diff/1008/1011
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/115920/diff/1008/1011#newcode275
Line 275: ASSERT_EQ(rm & 0x07, rm);
I'd rather fail than silently correct errors. The caller should know that the
argument is valid.

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

http://codereview.chromium.org/115920/diff/1008/1012#newcode868
Line 868: emit_operand(reg.code() & 0x07, adr);
There is no need for a register code argument when we have the version that
takes a register. The int version takes the value of the register field of the
ModR/M byte, and only 3-bit uints are allowed (could use that function, though).