Add immediate operands and arithmetic operations to the x64 assembler.

Add immediate operands and arithmetic operations to the x64 assembler.

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

[Lasse Reichstein, 2009-05-27 15:05:33]

LGTM, with comments.

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

http://codereview.chromium.org/115816/diff/1/5#newcode49
Line 49: *reinterpret_cast<uint32_t*>(pc_) = x;
You could use
  Memory::uint32_at(pc_) = x;
instead. I find it more readable, and it's even a little shorter.

http://codereview.chromium.org/115816/diff/1/5#newcode55
Line 55: *reinterpret_cast<uint64_t*>(pc_) = x;
And you might even add a Memory::uint64_at in 64-bit mode.

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

http://codereview.chromium.org/115816/diff/1/4#newcode214
Line 214: explicit Immediate(int32_t value) : value_(value) {}
You gave gutted the Immediate class to just be synonymous with int32_t, with no
extra functions, so you might as well drop it completely.

http://codereview.chromium.org/115816/diff/1/4#newcode418
Line 418: // All 64-bit immediates must have a relocation mode.
I can see they all have that now. Why must they? I.e., give the reason.
Otherwise someone might just add one without a Mode and remove the comment.

http://codereview.chromium.org/115816/diff/1/4#newcode445
Line 445: void arithmetic_op(byte opcode, Register dst, Register src);
Could you move these to be private. They seem to be only used internally.

http://codereview.chromium.org/115816/diff/1/4#newcode744
Line 744: void emit(Immediate x) { emitl(x.value_); }
No "inline"?

http://codereview.chromium.org/115816/diff/1/4#newcode749
Line 749: // REX.W is set.
The last two lines here seems like implementation detail. 
It's ok to have it with the implementation (if it's not obvious from the code),
but here it's unnecessary.

[Lasse Reichstein, 2009-05-27 15:25:38]

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

http://codereview.chromium.org/115816/diff/1/4#newcode418
Line 418: // All 64-bit immediates must have a relocation mode.
And if they don't *have* to have a reloc-mode, make it default to NONE.

[William Hesse, 2009-05-28 09:17:40]

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

http://codereview.chromium.org/115816/diff/1/5#newcode49
Line 49: *reinterpret_cast<uint32_t*>(pc_) = x;
On 2009/05/27 15:05:34, Lasse Reichstein wrote:
> You could use
>   Memory::uint32_at(pc_) = x;
> instead. I find it more readable, and it's even a little shorter.

Done.

http://codereview.chromium.org/115816/diff/1/5#newcode55
Line 55: *reinterpret_cast<uint64_t*>(pc_) = x;
On 2009/05/27 15:05:34, Lasse Reichstein wrote:
> And you might even add a Memory::uint64_at in 64-bit mode.

Done.

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

http://codereview.chromium.org/115816/diff/1/4#newcode418
Line 418: // All 64-bit immediates must have a relocation mode.
On 2009/05/27 15:05:34, Lasse Reichstein wrote:
> I can see they all have that now. Why must they? I.e., give the reason.
> Otherwise someone might just add one without a Mode and remove the comment.

For clarity, the user of the assembler should explicitly say what relocation
mode applies to each 64-bit immediate in the code.

http://codereview.chromium.org/115816/diff/1/4#newcode418
Line 418: // All 64-bit immediates must have a relocation mode.
On 2009/05/27 15:25:38, Lasse Reichstein wrote:
> And if they don't *have* to have a reloc-mode, make it default to NONE.

NONE is not a good default.  It is no likelier to be the correct relocation mode
than any other.

http://codereview.chromium.org/115816/diff/1/4#newcode445
Line 445: void arithmetic_op(byte opcode, Register dst, Register src);
On 2009/05/27 15:05:34, Lasse Reichstein wrote:
> Could you move these to be private. They seem to be only used internally.

Done.

http://codereview.chromium.org/115816/diff/1/4#newcode744
Line 744: void emit(Immediate x) { emitl(x.value_); }
On 2009/05/27 15:05:34, Lasse Reichstein wrote:
> No "inline"?
> 

No, everywhere in our codebase, inline functions defined in the class
declaration are not explicitly declared inline.

http://codereview.chromium.org/115816/diff/1/4#newcode749
Line 749: // REX.W is set.
On 2009/05/27 15:05:34, Lasse Reichstein wrote:
> The last two lines here seems like implementation detail. 
> It's ok to have it with the implementation (if it's not obvious from the
code),
> but here it's unnecessary.

It is not implementation detail, but defines the effect of the function.