Re: [Qemu-devel] [PATCH 0/9] tcg: reorganize bswap* functions

[Paul Brook]

> This patch series tries to reorganize a bit the bswap* TCG functions.

In principle this looks ok, however several implementation issues:

> +/* Note: we assume the six high bytes are set to zero */
> +static inline void tcg_gen_bswap16_i64(TCGv_i64 ret, TCGv_i64 arg)
> +{
> +    tcg_gen_bswap16_i32(TCGV_LOW(ret), TCGV_LOW(arg));
> +}
> +
> +/* Note: we assume the four high bytes are set to zero */
> +static inline void tcg_gen_bswap32_i64(TCGv_i64 ret, TCGv_i64 arg)
> +{
> +    tcg_gen_bswap32_i32(TCGV_LOW(ret), TCGV_LOW(arg));
> +}

I think we want to preserve the zero extension of the value, i.e. you want 
something along the lines of:

if (!TCGV_EQUAL_I64(ret, arg))
  tcg_gen_movi_i32(TCGV_HIGH(ret), 0);

- I'm not sure whether it's more efficient to do movi(ret, 0) or mov(ret, 
arg). With a bit of luck they'll both be optimized away most of the time.

> +/* Note: we assume the four high bytes are set to zero */
> +static inline void tcg_gen_bswap32_i64(TCGv_i64 ret, TCGv_i64 arg)
> +{
>...
> +    tcg_gen_shli_i64(t0, arg, 24);

Likewise this is missing a mask operation. ext32u_i64(t0, t0) is probably the 
most efficient way).

> +++ b/tcg/i386/tcg-target.c
> +    case INDEX_op_bswap16_i32:
> +        tcg_out8(s, 0x66);
> +        tcg_out_modrm(s, 0xc1, SHIFT_ROL, args[0]);
> +        tcg_out8(s, 8);

> +    { INDEX_op_bswap16_i32, { "r", "0" } },

This is wrong. The r/m field of the modrm byte can only address the first 4 
registers (AL-DL). Values 4-7 address te second bytes of these registers 
(a.k.a. AH-DH). I suspect you need to use the "q" constraint.

> +++ b/tcg/x86_64/tcg-target.c
> +    case INDEX_op_bswap16_i32:
> +    case INDEX_op_bswap16_i64:
> +        tcg_out8(s, 0x66);
> +        tcg_out_modrm(s, 0xc1, SHIFT_ROL, args[0]);
> +        tcg_out8(s, 8);
> +        break;

You need to use tcg_out_opc here to get REX prefixes. You need P_REXB to avoid 
the legacy encoding issues mentioned above, and the high bit of the r/m field 
also goes in the REX byte.

Paul