[webkit-reviews] review granted: [Bug 22867] Add support to X86Assembler emitting instructions that access all 16 registers on x86-64. : [Attachment 26028] The patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 22867: Add support to X86Assembler emitting instructions that access all 16
registers on x86-64.
https://bugs.webkit.org/show_bug.cgi?id=22867

Attachment 26028: The patch
https://bugs.webkit.org/attachment.cgi?id=26028&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
> +	   (JSC::X86Assembler::X86InstructionFormater::prefix):

"Formater" is a misspelling for "formatter." You should do a global
find/replace to change to X86InstructionFormatter, m_formatter, formatter, etc.


> +    // FIXME: this won't work on 64-bit!
> +    // need to make those 'xchgl_rr's be 'xchgq_rr's (we don't care about
the top bits of the shift value,
> +    // but ecx might have contained a pointer!
>      void lshift32(RegisterID shift_amount, RegisterID dest)
>      {
>	   // On x86 we can only shift by ecx; if asked to shift by another
register we'll

Yikes?

> -    void subl_mr(int offset, RegisterID base, RegisterID dst)
> +    void cmpl_im_force32(int imm, int offset, RegisterID base)

Can we just call this cmpl_i32m, since the normal operation is now called
cmpl_im?

> +
> +#define REX(r, x, b) (PRE_REX | ((r>>3)<<2) | ((x>>3)<<1) | (b>>3))
> +#if PLATFORM(X86_64)
> +#define EMIT_REX_W(r, x, b) m_buffer.putByteUnchecked(REX(r, x, b) | 8)
> +#define EMIT_REX_IF(c, r, x, b) if (c) m_buffer.putByteUnchecked(REX(r, x,
b))
> +#else
> +#define EMIT_REX_IF(c, r, x, b)
> +#endif
> +#define IS_REG_R8_UP(r) (r >= X86::r8)
> +#define IS_REG_SIL_UP(r) (r >= X86::esp)
> +#define EMIT_REX_IF_ANY_ARE_R8_UP(r, x, b)
EMIT_REX_IF(IS_REG_R8_UP(r)|IS_REG_R8_UP(x)|IS_REG_R8_UP(b), r, x, b)

These should be inline functions. It's OK to have an empty inline function in
the non-64 case.

> +	   // Immediates:
> +	   //
> +	   // An immedaite should be appended where appropriate after an op has
been emitted.
> +	   // The writes are unchecked since the opcode formaters above will
have ensured space.
> +
> +	   void instructionImmediate8(int imm)
> +	   {
> +	       m_buffer.putByteUnchecked(imm);
> +	   }
> +
> +	   void instructionImmediate32(int imm)
> +	   {
> +	       m_buffer.putIntUnchecked(imm);
> +	   }
> +
> +	   JmpSrc instructionRel32()
> +	   {
> +	       m_buffer.putIntUnchecked(0);
> +	       return JmpSrc(m_buffer.size());
> +	   }

I think you can remove the "instruction" prefix from these function names.
Implicitly, all Formatter operations deal with instructions.


> +#define MODRM(type, reg, rm) ((type << 6) | ((reg & 7) << 3) | (rm & 7))
> +#define SIB(type, reg, rm) MODRM(type, reg, rm)

These should be inline functions.

What's up with lshift32?

r=me other than that.