Bug 22867 – Add support to X86Assembler emitting instructions that access all 16 registers on x86-64.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

Add a new formating class, that is reponsible for both emitting the opcode bytes and the ModRm  bytes of an instruction in a single call; this can insert the REX byte as necessary before the opcode, but has access to the register numbers to build the REX.

[Gavin Barraclough]

Created attachment 26028 [details]
The patch

[Geoffrey Garen]

Comment on attachment 26028 [details]
The patch

> +        (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.

[Gavin Barraclough]

cmpl_im_force32 function name will change in a later patch, as agreed on irc.

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/assembler/AssemblerBuffer.h
Sending        JavaScriptCore/assembler/MacroAssembler.h
Sending        JavaScriptCore/assembler/X86Assembler.h
Sending        JavaScriptCore/jit/JIT.cpp
Sending        JavaScriptCore/jit/JITArithmetic.cpp
Sending        JavaScriptCore/jit/JITCall.cpp
Sending        JavaScriptCore/jit/JITPropertyAccess.cpp
Transmitting file data ........
Committed revision 39316.