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

From: Taylor Simpson <tsimpson@quicinc.com>
To: Richard Henderson <richard.henderson@linaro.org>,
	Alessandro Di Federico <ale.qemu@rev.ng>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "babush@rev.ng" <babush@rev.ng>, Brian Cain <bcain@quicinc.com>,
	Alessandro Di Federico <ale@rev.ng>,
	"nizzo@rev.ng" <nizzo@rev.ng>,
	"philmd@redhat.com" <philmd@redhat.com>
Subject: RE: [PATCH v4 06/12] target/hexagon: introduce new helper functions
Date: Mon, 19 Apr 2021 15:00:17 +0000	[thread overview]
Message-ID: <BYAPR02MB4886545B4B093744B7B4885DDE499@BYAPR02MB4886.namprd02.prod.outlook.com> (raw)
In-Reply-To: <b36337c6-f6d0-45b1-be11-b998d8579c4c@linaro.org>

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Sunday, April 18, 2021 4:31 PM
> To: Alessandro Di Federico <ale.qemu@rev.ng>; qemu-devel@nongnu.org
> Cc: Taylor Simpson <tsimpson@quicinc.com>; Brian Cain
> <bcain@quicinc.com>; babush@rev.ng; nizzo@rev.ng; philmd@redhat.com;
> Alessandro Di Federico <ale@rev.ng>
> Subject: Re: [PATCH v4 06/12] target/hexagon: introduce new helper
> functions
> 
> 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?

These are included in this patch
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01355.html
which hasn't been merged yet.

> 
> > +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

Once this patch series is merged, many load/store instructions will have manual overrides.  I can easily provide overrides for the remainder.  Then, we could skip them in the idef-parser.  At a minimum, you should skip the ones that are part of that series
- circular addressing					https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01355.html
- bit reverse addressing					https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01354.html
- load and unpack bytes					https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01353.html				
- load into shifted register				https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01359.html

Alessandro, what do you think?

Thanks,
Taylor