Re: [PATCH v4 04/21] target/loongarch: Add fixed point arithmetic instruction translation

[Song Gao]

Hi, Richard,  

Thank you for your comments!

On 09/04/2021 07:04 PM, Richard Henderson wrote:
> On 9/2/21 2:40 PM, Song Gao wrote:
>> +static bool gen_r2_si12(DisasContext *ctx, arg_fmt_rdrjsi12 *a,
>> +                        DisasExtend src_ext, DisasExtend dst_ext,
>> +                        void (*func)(TCGv, TCGv, TCGv))
>> +{
>> +    ctx->dst_ext = dst_ext;
>> +    TCGv dest = gpr_dst(ctx, a->rd);
>> +    TCGv src1 = gpr_src(ctx, a->rj, src_ext);
>> +    TCGv src2 = tcg_constant_tl(a->si12);
> 
> Prefer to put declarations before statements, as per old C90 still.
> 
OK.
>> +    func(dest, src1, src2);
>> +
>> +    if (ctx->dst_ext) {
> 
> Given that func does not receive ctx, why store dst_ext into ctx and then 
> read it back? It seems like you should just use the parameter directly.
> 
OK， ctx->dst_ext will be deleted.
>> +static bool gen_pc(DisasContext *ctx, arg_fmt_rdsi20 *a,
>> +                   void (*func)(DisasContext *ctx, TCGv, target_long))
>> +{
>> +    TCGv dest = gpr_dst(ctx, a->rd);
>> +
>> +    func(ctx, dest, a->si20);
>> +    return true;
>> +}
> 
> Perhaps clearer with:
> 
>   target_long (*func)(target_long pc, target_long si20)
> 
>   target_long addr = func(ctx->base.pc_next, a->si20);> OK， ctx->dst_ext will 
> be deleted.
>   TCGv dest = gpr_dst(ctx, a->rd);
> 
>   tcg_gen_movi_tl(dest, addr);
>   return true;
> 
> 
Surely.
> 
> 
>> +static bool gen_mulh(DisasContext *ctx, arg_add_w *a,
>> +                     void(*func)(TCGv_i32, TCGv_i32, TCGv_i32, TCGv_i32))
>> +{
>> +    TCGv dest = gpr_dst(ctx, a->rd);
>> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
>> +    TCGv src2 = gpr_src(ctx, a->rk, EXT_NONE);
>> +    TCGv_i32 discard = tcg_temp_new_i32();
>> +    TCGv_i32 t0 = tcg_temp_new_i32();
>> +    TCGv_i32 t1 = tcg_temp_new_i32();
>> +
>> +    tcg_gen_trunc_tl_i32(t0, src1);
>> +    tcg_gen_trunc_tl_i32(t1, src2);
>> +    func(discard, t0, t0, t1);
>> +    tcg_gen_ext_i32_tl(dest, t0);
>> +
>> +    tcg_temp_free_i32(discard);
>> +    tcg_temp_free_i32(t0);
>> +    tcg_temp_free_i32(t1);
>> +    return true;
>> +}
> 
> You should be able to use gen_r3 for these.
> 
> 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);
> }
> 
> static void gen_mulh_wu(TCGv dest, TCGv src1, TCGv src2)
> {
>     tcg_gen_mul_i64(dest, src1, src2);
>     tcg_gen_sari_i64(dest, dest, 32);
> }
> 
> static void gen_mulh_d(TCGv dest, TCGv src1, TCGv src2)
> {
>     TCGv discard = tcg_temp_new();
>     tcg_gen_muls2_tl(discard, dest, src1, src2);
>     tcg_temp_free(discard);
> }
> 
> static void gen_mulh_du(TCGv dest, TCGv src1, TCGv src2)
> {
>     TCGv discard = tcg_temp_new();
>     tcg_gen_mulu2_tl(discard, dest, src1, src2);
>     tcg_temp_free(discard);
> }
> 
> TRANS(mulh_w, gen_r3, EXT_SIGN, EXT_SIGN, EXT_NONE, gen_mulh_w)
> TRANS(mulh_wu, gen_r3, EXT_ZERO, EXT_ZERO, EXT_NONE, gen_mulh_wu)
> TRANS(mulh_d, gen_r3, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_d)
> TRANS(mulh_du, gen_r3, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_du)
> 
>> +static bool gen_mulw_d(DisasContext *ctx, arg_add_w *a,
>> +                     void(*func)(TCGv_i64, TCGv))
>> +{
>> +    TCGv dest = gpr_dst(ctx, a->rd);
>> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
>> +    TCGv src2 = gpr_src(ctx, a->rk, EXT_NONE);
>> +
>> +    func(src1, src1);
>> +    func(src2, src2);
>> +    tcg_gen_mul_i64(dest, src1, src2);
>> +    return true;
>> +}
> 
> The func parameter here serves the same purpose as DisasExtend, so again you 
> can use gen_r3:
> 
> TRANS(mulw_d_w, gen_r3, EXT_SIGN, EXT_SIGN, EXT_NONE, tcg_gen_mul_tl)
> TRANS(mulw_d_wu, gen_r3, EXT_ZERO, EXT_ZERO, EXT_NONE, tcg_gen_mul_tl)
> 
> 
OK. 
> 
>> +
>> +static bool gen_div_w(DisasContext *ctx, arg_fmt_rdrjrk *a,
>> +                      DisasExtend src_ext, DisasExtend dst_ext,
>> +                      void(*func)(TCGv, TCGv, TCGv))
>> +{
>> +    ctx->dst_ext = dst_ext;
>> +    TCGv dest = gpr_dst(ctx, a->rd);
>> +    TCGv src1 = gpr_src(ctx, a->rj, src_ext);
>> +    TCGv src2 = gpr_src(ctx, a->rk, src_ext);
>> +    TCGv t2 = tcg_temp_new();
>> +    TCGv t3 = tcg_temp_new();
>> +
>> +    tcg_gen_setcondi_tl(TCG_COND_EQ, t2, src1, INT_MIN);
>> +    tcg_gen_setcondi_tl(TCG_COND_EQ, t3, src2, -1);
>> +    tcg_gen_and_tl(t2, t2, t3);
>> +    tcg_gen_setcondi_tl(TCG_COND_EQ, t3, src2, 0);
>> +    tcg_gen_or_tl(t2, t2, t3);
>> +    tcg_gen_movi_tl(t3, 0);
>> +    tcg_gen_movcond_tl(TCG_COND_NE, src2, t2, t3, t2, src2);
> 
> Bug, writing back to src2.
> OK.
>> +    func(dest, src1, src2);
> 
> You can split this out differently so that you can use gen_r3.
> 
> static TCGv prep_divisor_d(TCGv src1, TCGv src2)
> {
>     TCGv t0 = temp_new();
>     TCGv t1 = tcg_temp_new();
>     TCGv t2 = tcg_temp_new();
>     TCGv zero = tcg_constant_tl(0);
> 
>     /*
>      * If min / -1, set the divisor to 1.
>      * This avoids potential host overflow trap and produces min.
>      * If x / 0, set the divisor to 1.
>      * This avoids potential host overflow trap;
>      * the required result is undefined.
>      */
>     tcg_gen_setcondi_tl(TCG_COND_EQ, t0, src1, INT64_MIN);
>     tcg_gen_setcondi_tl(TCG_COND_EQ, t1, src2, -1);
>     tcg_gen_setcondi_tl(TCG_COND_EQ, t2, src2, 0);
>     tcg_gen_and_tl(t0, t0, t1);
>     tcg_gen_or_tl(t0, t0, t2);
>     tcg_gen_movcond_tl(TCG_COND_NE, t0, t0, zero, t0, src2);
> 
>     tcg_temp_free(t1);
>     tcg_temp_free(t2);
>     return t0;
> }
> 
> static TCGv prep_divisor_du(TCGv src2)
> {
>     TCGv t0 = temp_new();
>     TCGv zero = tcg_constant_tl(0);
>     TCGv one = tcg_constant_tl(1);
> 
>     /*
>      * If x / 0, set the divisor to 1.
>      * This avoids potential host overflow trap;
>      * the required result is undefined.
>      */
>     tcg_gen_movcond_tl(TCG_COND_EQ, t0, src2, zero, one, src2);
>     return t0;
> }
> 
> static void gen_div_d(TCGv dest, TCGv src1, TCGv src2)
> {
>     src2 = prep_divisor_d(src1, src2);
>     tcg_gen_div_tl(dest, src1, src2);
> }
> 
> static void gen_rem_d(TCGv dest, TCGv src1, TCGv src2)
> {
>     src2 = prep_divisor_d(src1, src2);
>     tcg_gen_rem_tl(dest, src1, src2);
> }
> 
> static void gen_div_du(TCGv dest, TCGv src1, TCGv src2)
> {
>     src2 = prep_divisor_du(src2);
>     tcg_gen_divu_tl(dest, src1, src2);
> }
> 
> static void gen_rem_du(TCGv dest, TCGv src1, TCGv src2)
> {
>     src2 = prep_divisor_du(src2);
>     tcg_gen_remu_tl(dest, src1, src2);
> }
> 
> static void gen_div_w(TCGv dest, TCGv src1, TCGv src2)
> {
>     /* We need not check for integer overflow for div_w. */
>     src2 = prep_divisor_du(src2);
>     tcg_gen_div_tl(dest, src1, src2);
> }
> 
> static void gen_rem_w(TCGv dest, TCGv src1, TCGv src2)
> {
>     /* We need not check for integer overflow for rem_w. */
>     src2 = prep_divisor_du(src2);
>     tcg_gen_rem_tl(dest, src1, src2);
> }
> 
> TRANS(div_w, gen_r3, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_div_w)
> TRANS(mod_w, gen_r3, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_rem_w)
> TRANS(div_wu, gen_r3, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_div_du)
> TRANS(mod_wu, gen_r3, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_div_du)
> TRANS(div_d, gen_r3, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_d)
> TRANS(mod_d, gen_r3, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_d)
> TRANS(div_du, gen_r3, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_du)
> TRANS(mod_du, gen_r3, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_du)
> 

Nice.

>> --- a/target/loongarch/internals.h
>> +++ b/target/loongarch/internals.h
>> @@ -9,7 +9,6 @@
>>   #ifndef LOONGARCH_INTERNALS_H
>>   #define LOONGARCH_INTERNALS_H
>>   -
>>   void loongarch_translate_init(void);
>>     void loongarch_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> 
> Fold this back to a previous patch.
>
OK
>> -static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
>> -{
>> -    return true;
>> -}
> 
> Yes indeed, fold this patch to a previous patch.
> 
OK.

Song Gao
Thanks.
> 
> r~