Re: [Qemu-devel] [PATCH v14 16/33] target-tilegx: Handle most bit manipulation instructions

[Peter Maydell]

On 24 August 2015 at 17:17, Richard Henderson <address@hidden> wrote:
> Omitting crc instructions.

I'm not a fan of commit message bodies that rely on reading
the subject line to make sense (partly because my mail
client doesn't put the subject line very prominently
when reading the email...)

> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target-tilegx/helper.c    | 23 ++++++++++++++++++
>  target-tilegx/helper.h    |  2 ++
>  target-tilegx/translate.c | 60 
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/target-tilegx/helper.c b/target-tilegx/helper.c
> index ea66da0..6aba681 100644
> --- a/target-tilegx/helper.c
> +++ b/target-tilegx/helper.c
> @@ -40,6 +40,29 @@ uint64_t helper_cnttz(uint64_t arg)
>      return ctz64(arg);
>  }
>
> +uint64_t helper_pcnt(uint64_t arg)
> +{
> +    return ctpop64(arg);
> +}
> +
> +uint64_t helper_revbits(uint64_t arg)
> +{
> +    /* Assign the correct byte position.  */
> +    arg = bswap64(arg);
> +
> +    /* Assign the correct nibble position.  */
> +    arg = ((arg & 0xf0f0f0f0f0f0f0f0ULL) >> 4)
> +        | ((arg & 0x0f0f0f0f0f0f0f0fULL) << 4);
> +
> +    /* Assign the correct bit position.  */
> +    arg = ((arg & 0x8888888888888888ULL) >> 3)
> +        | ((arg & 0x4444444444444444ULL) >> 1)
> +        | ((arg & 0x2222222222222222ULL) << 1)
> +        | ((arg & 0x1111111111111111ULL) << 3);
> +
> +    return arg;
> +}

AArch64 has this exact same operation; maybe we should
factor it out into bitops.h ?

> +
>  /*
>   * Functional Description
>   *     uint64_t a = rf[SrcA];
> diff --git a/target-tilegx/helper.h b/target-tilegx/helper.h
> index fd5517e..644d313 100644
> --- a/target-tilegx/helper.h
> +++ b/target-tilegx/helper.h
> @@ -1,4 +1,6 @@
>  DEF_HELPER_2(exception, noreturn, env, i32)
>  DEF_HELPER_FLAGS_1(cntlz, TCG_CALL_NO_RWG_SE, i64, i64)
>  DEF_HELPER_FLAGS_1(cnttz, TCG_CALL_NO_RWG_SE, i64, i64)
> +DEF_HELPER_FLAGS_1(pcnt, TCG_CALL_NO_RWG_SE, i64, i64)
> +DEF_HELPER_FLAGS_1(revbits, TCG_CALL_NO_RWG_SE, i64, i64)
>  DEF_HELPER_FLAGS_3(shufflebytes, TCG_CALL_NO_RWG_SE, i64, i64, i64, i64)
> diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
> index 090c006..82a34e5 100644
> --- a/target-tilegx/translate.c
> +++ b/target-tilegx/translate.c
> @@ -177,6 +177,35 @@ static void gen_saturate_op(TCGv tdest, TCGv tsrca, TCGv 
> tsrcb,
>      tcg_temp_free(t0);
>  }
>
> +static void gen_dblaligni(TCGv tdest, TCGv tsrca, TCGv tsrcb, int shr)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    tcg_gen_shri_tl(t0, tsrcb, shr);
> +    tcg_gen_shli_tl(tdest, tsrca, 64 - shr);
> +    tcg_gen_or_tl(tdest, tdest, t0);
> +
> +    tcg_temp_free(t0);
> +}
> +
> +static void gen_dblalign(TCGv tdest, TCGv tsrcd, TCGv tsrca, TCGv tsrcb)
> +{
> +    TCGv t0 = tcg_temp_new();

This operation seems sufficiently obscure that I think it would be
helpful to have a comment that explains what it is:
    /* Shift the 128 bit value tsrca:tsrcd right by the number
     * of bytes specified by the bottom 3 bits of tsrcb, and
     * set tdest to the low 64 bits of the resulting value.
     */

(I think I've read the ISA correctly...)

Incidentally I deduce that we're not planning to support bigendian
mode?

> +
> +    tcg_gen_andi_tl(t0, tsrcb, 7);
> +    tcg_gen_shli_tl(t0, t0, 3);
> +    tcg_gen_shr_tl(tdest, tsrcd, t0);
> +
> +    /* Rather than creating and invalid shift, 64 - 0, perform the
> +       left shift in two steps via the one's compliment.  */

typos: "an", "complement".

It might be helpful to say what the maths going on here is
more explicitly:
    We want to do "t0 = tsrca << (64 - t0)".
    Twos' complement arithmetic on a 6-bit field tells us
    that 64 - t0 == (t0 ^ 63) + 1. So we can do the shift in
    two parts, neither of which will be 64 (since t0 can't be
    63 here).

Otherwise

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM