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

[Richard Henderson]

On 12/26/24 13:19, Jiaxun Yang wrote:
> mulh.w and mulh.wu are handled with tcg_gen_muls2_i32 and tcg_gen_mulu2_i32
> to adopt different TARGET_LONG size.
>
> min value of divisor is generated from TARGET_LONG_BITS to adopt different
> long size as well.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   target/loongarch/tcg/insn_trans/trans_arith.c.inc | 25 +++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/target/loongarch/tcg/insn_trans/trans_arith.c.inc 
> b/target/loongarch/tcg/insn_trans/trans_arith.c.inc
> index 
> 2be057e9320a9b722c173b0352e1631543147d68..a2360c5fdd2003ca0e458743348e687987f421d4
>  100644
> --- a/target/loongarch/tcg/insn_trans/trans_arith.c.inc
> +++ b/target/loongarch/tcg/insn_trans/trans_arith.c.inc
> @@ -92,8 +92,24 @@ static void gen_sltu(TCGv dest, TCGv src1, TCGv src2)
>   
>   static void gen_mulh_w(TCGv dest, TCGv src1, TCGv src2)
>   {
> -    tcg_gen_mul_i64(dest, src1, src2);
> -    tcg_gen_sari_i64(dest, dest, 32);
> +#ifdef TARGET_LOONGARCH64
> +    tcg_gen_mul_tl(dest, src1, src2);
> +    tcg_gen_sari_tl(dest, dest, 32);

Leave the _i64.

> +#else
> +    TCGv_i32 discard = tcg_temp_new_i32();
> +    tcg_gen_muls2_i32(discard, dest, src1, src2);
> +#endif
> +}
> +
> +static void gen_mulh_wu(TCGv dest, TCGv src1, TCGv src2)
> +{
> +#ifdef TARGET_LOONGARCH64
> +    /* Signs are handled by the caller's EXT_ZERO */
> +    gen_mulh_w(dest, src1, src2);
> +#else
> +    TCGv_i32 discard = tcg_temp_new_i32();
> +    tcg_gen_mulu2_i32(discard, dest, src1, src2);
> +#endif
>   }

Otherwise, these two are fine.

>   
>   static void gen_mulh_d(TCGv dest, TCGv src1, TCGv src2)
> @@ -113,6 +129,7 @@ static void prep_divisor_d(TCGv ret, TCGv src1, TCGv src2)
>       TCGv t0 = tcg_temp_new();
>       TCGv t1 = tcg_temp_new();
>       TCGv zero = tcg_constant_tl(0);
> +    target_long min = 1ull << (TARGET_LONG_BITS - 1);
>   
>       /*
>        * If min / -1, set the divisor to 1.
> @@ -121,7 +138,7 @@ static void prep_divisor_d(TCGv ret, TCGv src1, TCGv src2)
>        * This avoids potential host overflow trap;
>        * the required result is undefined.
>        */
> -    tcg_gen_setcondi_tl(TCG_COND_EQ, ret, src1, INT64_MIN);
> +    tcg_gen_setcondi_tl(TCG_COND_EQ, ret, src1, min);
>       tcg_gen_setcondi_tl(TCG_COND_EQ, t0, src2, -1);
>       tcg_gen_setcondi_tl(TCG_COND_EQ, t1, src2, 0);
>       tcg_gen_and_tl(ret, ret, t0);

This is ok, but s/prep_divisor_d/prep_divisor_tl/.
Without the rename, this change would appear to affect correctness.

In addition, gen_{div,rem}_w will need to use prep_divisor_tl instead of 
prep_divisor_du.


r~