Re: [PATCH v4 06/12] target/hexagon: introduce new helper functions

[Richard Henderson]

On 4/15/21 9:34 AM, Alessandro Di Federico wrote:
> +void gen_store32(TCGv vaddr, TCGv src, tcg_target_long width, unsigned slot)
> +{
> +    tcg_gen_mov_tl(hex_store_addr[slot], vaddr);
> +    tcg_gen_movi_tl(hex_store_width[slot], width);
> +    tcg_gen_mov_tl(hex_store_val32[slot], src);
> +}
> +
> +void gen_store1(TCGv_env cpu_env, TCGv vaddr, TCGv src, DisasContext *ctx,
> +                unsigned slot)
> +{
> +    gen_store32(vaddr, src, 1, slot);
> +    ctx->store_width[slot] = 1;
> +}

Why is store_width here and not in gen_store32?
Do you really need so many helpers here, as opposed to making use of MemOp?

> +void gen_sat_i32_ext(TCGv ovfl, TCGv dest, TCGv source, int width)
> +{
> +    gen_sat_i32(dest, source, width);
> +    TCGv zero = tcg_const_i32(0);
> +    TCGv one = tcg_const_i32(1);
> +    tcg_gen_movcond_i32(TCG_COND_NE, ovfl, source, dest, one, zero);

(source != dest ? 1 : 0) -> (source != dest).

Therefore, tcg_gen_setcond_i32.

Or did you intend

ovfl = (source != dest ? 1 : ovfl)?

which is probably still better as

  tcg_gen_setcond_tl(TCG_COND_NE, tmp, source,dest);
  tcg_gen_or_tl(ovfl, ovfl, tmp);

> +void gen_fbrev(TCGv result, TCGv src)
> +{
> +    TCGv lo = tcg_temp_new();
> +    TCGv tmp1 = tcg_temp_new();
> +    TCGv tmp2 = tcg_temp_new();
> +
> +    /* Bit reversal of low 16 bits */
> +    tcg_gen_extract_tl(lo, src, 0, 16);
> +    tcg_gen_andi_tl(tmp1, lo, 0xaaaa);
> +    tcg_gen_shri_tl(tmp1, tmp1, 1);
> +    tcg_gen_andi_tl(tmp2, lo, 0x5555);
> +    tcg_gen_shli_tl(tmp2, tmp2, 1);
> +    tcg_gen_or_tl(lo, tmp1, tmp2);
> +    tcg_gen_andi_tl(tmp1, lo, 0xcccc);
> +    tcg_gen_shri_tl(tmp1, tmp1, 2);
> +    tcg_gen_andi_tl(tmp2, lo, 0x3333);
> +    tcg_gen_shli_tl(tmp2, tmp2, 2);
> +    tcg_gen_or_tl(lo, tmp1, tmp2);
> +    tcg_gen_andi_tl(tmp1, lo, 0xf0f0);
> +    tcg_gen_shri_tl(tmp1, tmp1, 4);
> +    tcg_gen_andi_tl(tmp2, lo, 0x0f0f);
> +    tcg_gen_shli_tl(tmp2, tmp2, 4);
> +    tcg_gen_or_tl(lo, tmp1, tmp2);
> +    tcg_gen_bswap16_tl(lo, lo);
> +
> +    /* Final tweaks */
> +    tcg_gen_deposit_tl(result, src, lo, 0, 16);
> +    tcg_gen_or_tl(result, result, lo);
> +
> +    tcg_temp_free(lo);
> +    tcg_temp_free(tmp1);
> +    tcg_temp_free(tmp2);
> +}

Coordinate with Taylor.
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg10007.html


r~