Re: [PATCH v2 12/23] target/loongarch: Scrutinise TCG bitops translation for 32 bit build

[Richard Henderson]

On 12/26/24 13:19, Jiaxun Yang wrote:
> Use tl variant whenever possible.
>
> Silent compiler warnings by performing casting for come consts.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   target/loongarch/tcg/insn_trans/trans_bit.c.inc | 34 ++++++++++++++-----------
>   1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/target/loongarch/tcg/insn_trans/trans_bit.c.inc 
> b/target/loongarch/tcg/insn_trans/trans_bit.c.inc
> index 
> ee5fa003ce06a1910f826c3eb96d1d532c32e02c..a40346a670be31a123848e8ea5f7b94f8372976b
>  100644
> --- a/target/loongarch/tcg/insn_trans/trans_bit.c.inc
> +++ b/target/loongarch/tcg/insn_trans/trans_bit.c.inc
> @@ -18,13 +18,17 @@ static bool gen_rr(DisasContext *ctx, arg_rr *a,
>   
>   static void gen_bytepick_w(TCGv dest, TCGv src1, TCGv src2, target_long sa)
>   {
> +#ifdef TARGET_LOONGARCH64
>       tcg_gen_concat_tl_i64(dest, src1, src2);
>       tcg_gen_sextract_i64(dest, dest, (32 - sa * 8), 32);
> +#else
> +    tcg_gen_extract2_tl(dest, src1, src2, (32 - sa * 8));

This is the kind of case where using _i32 explicitly emphasizes that the specific size is 
required for correctness.  You've already got the ifdef to protect the code anyway.

>   static void gen_bytepick_d(TCGv dest, TCGv src1, TCGv src2, target_long sa)
>   {
> -    tcg_gen_extract2_i64(dest, src1, src2, (64 - sa * 8));
> +    tcg_gen_extract2_tl(dest, src1, src2, (64 - sa * 8));
>   }
>   
>   static bool gen_bstrins(DisasContext *ctx, arg_rr_ms_ls *a,
> @@ -85,7 +89,7 @@ static void gen_cto_w(TCGv dest, TCGv src1)
>   
>   static void gen_clz_d(TCGv dest, TCGv src1)
>   {
> -    tcg_gen_clzi_i64(dest, src1, TARGET_LONG_BITS);
> +    tcg_gen_clzi_tl(dest, src1, TARGET_LONG_BITS);
>   }
>   
>   static void gen_clo_d(TCGv dest, TCGv src1)
> @@ -107,8 +111,8 @@ static void gen_cto_d(TCGv dest, TCGv src1)
>   
>   static void gen_revb_2w(TCGv dest, TCGv src1)
>   {
> -    tcg_gen_bswap64_i64(dest, src1);
> -    tcg_gen_rotri_i64(dest, dest, 32);
> +    tcg_gen_bswap_tl(dest, src1);
> +    tcg_gen_rotri_tl(dest, dest, 32);
>   }
>   
>   static void gen_revb_2h(TCGv dest, TCGv src1)
> @@ -126,7 +130,7 @@ static void gen_revb_2h(TCGv dest, TCGv src1)
>   
>   static void gen_revb_4h(TCGv dest, TCGv src1)
>   {
> -    TCGv mask = tcg_constant_tl(0x00FF00FF00FF00FFULL);
> +    TCGv mask = tcg_constant_tl((target_ulong)0x00FF00FF00FF00FFULL);
>       TCGv t0 = tcg_temp_new();
>       TCGv t1 = tcg_temp_new();
>   
> @@ -139,22 +143,22 @@ static void gen_revb_4h(TCGv dest, TCGv src1)
>   
>   static void gen_revh_2w(TCGv dest, TCGv src1)
>   {
> -    TCGv_i64 t0 = tcg_temp_new_i64();
> -    TCGv_i64 t1 = tcg_temp_new_i64();
> -    TCGv_i64 mask = tcg_constant_i64(0x0000ffff0000ffffull);
> +    TCGv t0 = tcg_temp_new();
> +    TCGv t1 = tcg_temp_new();
> +    TCGv mask = tcg_constant_tl((target_ulong)0x0000ffff0000ffffull);
>   
> -    tcg_gen_shri_i64(t0, src1, 16);
> -    tcg_gen_and_i64(t1, src1, mask);
> -    tcg_gen_and_i64(t0, t0, mask);
> -    tcg_gen_shli_i64(t1, t1, 16);
> -    tcg_gen_or_i64(dest, t1, t0);
> +    tcg_gen_shri_tl(t0, src1, 16);
> +    tcg_gen_and_tl(t1, src1, mask);
> +    tcg_gen_and_tl(t0, t0, mask);
> +    tcg_gen_shli_tl(t1, t1, 16);
> +    tcg_gen_or_tl(dest, t1, t0);
>   }
>   
>   static void gen_revh_d(TCGv dest, TCGv src1)
>   {
>       TCGv t0 = tcg_temp_new();
>       TCGv t1 = tcg_temp_new();
> -    TCGv mask = tcg_constant_tl(0x0000FFFF0000FFFFULL);
> +    TCGv mask = tcg_constant_tl((target_ulong)0x0000FFFF0000FFFFULL);
>   
>       tcg_gen_shri_tl(t1, src1, 16);
>       tcg_gen_and_tl(t1, t1, mask);

While this allows the code to compile, (1) the functions are unused and (2) they do not 
compute the required results.  For me, the latter is concerning.

I'd suggest moving GEN_FALSE_TRANS out of trans_privileged.c.inc, then

#ifdef TARGET_LOONGARCH64
// all gen_foo_d
TRANS(bytepick_d, 64, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_bytepick_d)
// etc
#else
GEN_FALSE_TRANS(bytepick_d)
// etc
#endif


r~