RE: [PATCH v2 16/21] Hexagon (target/hexagon) add A4_addp_c/A4_subp_c

[Taylor Simpson]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, April 6, 2021 4:12 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; ale@rev.ng; Brian Cain <bcain@quicinc.com>
> Subject: Re: [PATCH v2 16/21] Hexagon (target/hexagon) add
> A4_addp_c/A4_subp_c
>
> On 3/31/21 8:53 PM, Taylor Simpson wrote:
> > +#define fGEN_TCG_A4_addp_c(SHORTCODE) \
> > +    do { \
> > +        TCGv_i64 carry = tcg_temp_new_i64(); \
> > +        TCGv_i64 zero = tcg_const_i64(0); \
> > +        tcg_gen_extu_i32_i64(carry, PxV); \
> > +        tcg_gen_andi_i64(carry, carry, 1); \
> > +        tcg_gen_add2_i64(RddV, carry, RssV, zero, carry, zero); \
> > +        tcg_gen_add2_i64(RddV, carry, RddV, carry, RttV, zero); \
> > +        tcg_gen_extrl_i64_i32(PxV, carry); \
>
> Note that this is already a single bit, always.

Yes, we just need an _i32 instead of an _i64.

> > +static TCGv gen_8bitsof(TCGv result, TCGv value)
> > +{
> > +    TCGv zero = tcg_const_tl(0);
> > +    TCGv ones = tcg_const_tl(0xff);
> > +    tcg_gen_movcond_tl(TCG_COND_NE, result, value, zero, ones, zero);
> > +    tcg_temp_free(zero);
> > +    tcg_temp_free(ones);
> > +
> > +    return result;
>
> There's little point in a conditional move.
> Just multiply by 0xff.
>
> Unless you had another non-boolean use for gen_8bitsof?

Yes, there are instances of that.

> Anyway, I guess it's all sane,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!