Re: [PATCH 17/28] tcg: Add flags argument to tcg_gen_bswap16_*, tcg_gen_bswap32_i64

[Peter Maydell]

On Mon, 14 Jun 2021 at 09:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Implement the new semantics in the fallback expansion.
> Change all callers to supply the flags that keep the
> semantics unchanged locally.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index dc65577e2f..3763285bb0 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -1001,20 +1001,35 @@ void tcg_gen_ext16u_i32(TCGv_i32 ret, TCGv_i32 arg)
>      }
>  }
>
> -/* Note: we assume the two high bytes are set to zero */
> -void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg)
> +void tcg_gen_bswap16_i32(TCGv_i32 ret, TCGv_i32 arg, int flags)
>  {
> +    /* Only one extension flag may be present. */
> +    tcg_debug_assert(!(flags & TCG_BSWAP_OS) || !(flags & TCG_BSWAP_OZ));
> +
>      if (TCG_TARGET_HAS_bswap16_i32) {
> -        tcg_gen_op3i_i32(INDEX_op_bswap16_i32, ret, arg,
> -                         TCG_BSWAP_IZ | TCG_BSWAP_OZ);
> +        tcg_gen_op3i_i32(INDEX_op_bswap16_i32, ret, arg, flags);
>      } else {
>          TCGv_i32 t0 = tcg_temp_new_i32();
> +        TCGv_i32 t1 = tcg_temp_new_i32();
>
> -        tcg_gen_ext8u_i32(t0, arg);
> -        tcg_gen_shli_i32(t0, t0, 8);
> -        tcg_gen_shri_i32(ret, arg, 8);
> -        tcg_gen_or_i32(ret, ret, t0);
> +        tcg_gen_shri_i32(t0, arg, 8);
> +        if (!(flags & TCG_BSWAP_IZ)) {
> +            tcg_gen_ext8u_i32(t0, t0);
> +        }
> +
> +        if (flags & TCG_BSWAP_OS) {
> +            tcg_gen_shli_i32(t1, t1, 24);

t1 hasn't been initialized yet. Should this be "tcg_gen_shli_i32(t1, arg, 24)" ?

> +            tcg_gen_sari_i32(t1, t1, 16);
> +        } else if (flags & TCG_BSWAP_OZ) {
> +            tcg_gen_ext8u_i32(t1, arg);
> +            tcg_gen_shli_i32(t1, t1, 8);
> +        } else {
> +            tcg_gen_shli_i32(t1, arg, 8);
> +        }
> +
> +        tcg_gen_or_i32(ret, t0, t1);
>          tcg_temp_free_i32(t0);
> +        tcg_temp_free_i32(t1);
>      }

>      } else {
>          TCGv_i64 t0 = tcg_temp_new_i64();
> +        TCGv_i64 t1 = tcg_temp_new_i64();
>
> -        tcg_gen_ext8u_i64(t0, arg);
> -        tcg_gen_shli_i64(t0, t0, 8);
> -        tcg_gen_shri_i64(ret, arg, 8);
> -        tcg_gen_or_i64(ret, ret, t0);
> +        tcg_gen_shri_i64(t0, arg, 8);
> +        if (!(flags & TCG_BSWAP_IZ)) {
> +            tcg_gen_ext8u_i64(t0, t0);
> +        }
> +
> +        if (flags & TCG_BSWAP_OS) {
> +            tcg_gen_shli_i64(t1, t1, 56);

Similarly here.

> +            tcg_gen_sari_i64(t1, t1, 48);
> +        } else if (flags & TCG_BSWAP_OZ) {
> +            tcg_gen_ext8u_i64(t1, arg);
> +            tcg_gen_shli_i64(t1, t1, 8);
> +        } else {
> +            tcg_gen_shli_i64(t1, arg, 8);
> +        }
> +
> +        tcg_gen_or_i64(ret, t0, t1);
>          tcg_temp_free_i64(t0);
> +        tcg_temp_free_i64(t1);
>      }
>  }

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM