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. http://codereview.chromium.org/115816 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
