Comments addressed, other changes made, new version uploaded.
http://codereview.chromium.org/113841/diff/1/4 File src/x64/assembler-x64-inl.h (right): http://codereview.chromium.org/113841/diff/1/4#newcode183 Line 183: // SIB byte needed to encode (esp + offset) or (esp + offset) On 2009/05/27 07:17:19, Kevin Millikin wrote: > This comment is somehow wrong (and it's missing a period :) ). Done. http://codereview.chromium.org/113841/diff/1/4#newcode188 Line 188: if (base.is(rsp) || base.is(r12)) { On 2009/05/27 07:17:19, Kevin Millikin wrote: > Since this code is repeated in all three cases, can't you write: > if (base.is(rsp) || base.is(r12)) set_sib(times_1, rsp, base); > if (disp == 0 && ...) { > set_modrm(0, rsp); > } else if (is_int8(disp)) { > set_modrm(1, base); > } else { > set_modrm(2, base); > set_disp32(disp): > } The functions needed to be called in the order: set_modrm, set_sib, set_disp*. I modified set_modrm so it can be called after the other two, and made your suggested change. http://codereview.chromium.org/113841/diff/1/3 File src/x64/assembler-x64.h (right): http://codereview.chromium.org/113841/diff/1/3#newcode258 Line 258: INLINE(explicit Operand(Register base, int32_t disp)); On 2009/05/27 07:17:19, Kevin Millikin wrote: > Why explicit? Good question. I wondered myself. I think it was copied and pasted from the single-argument case. I'll remove it. http://codereview.chromium.org/113841 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
