Re: [PATCH v11 04/26] target/loongarch: Add fixed point arithmetic instruction translation

[gaosong]

Hi Richard,

On 2021/11/20 下午3:17, Richard Henderson wrote:

> On
>       11/19/21 7:13 AM, Song Gao wrote:
>       
> > +static bool gen_rrr(DisasContext *ctx,
> >         arg_rrr *a,
> >         
> >         +                    DisasExtend src1_ext, DisasExtend src2_ext,
> >         
> >         +                    DisasExtend dst_ext, void (*func)(TCGv,
> >         TCGv, TCGv))
> >         
> >         +{
> >         
> >         +    TCGv dest = gpr_dst(ctx, a->rd, dst_ext);
> >         
> >         +    TCGv src1 = gpr_src(ctx, a->rj, src1_ext);
> >         
> >         +    TCGv src2 = gpr_src(ctx, a->rk, src2_ext);
> >         
> >         +
> >         
> >         +    func(dest, src1, src2);
> >         
> >         +
> >         
> >         +    /* dst_ext is EXT_NONE and input is dest, We don't run
> >         gen_set_gpr. */
> >         
> >         +    if (dst_ext) {
> >         
> >         +        gen_set_gpr(a->rd, dest, dst_ext);
> >         
> >         +    }
> >         
> >       
>       
>       Why the (incomplete) condition around gen_set_gpr?
>       
>       
>       I think it's a bug to not name EXT_NONE in the test (just because
>       EXT_NONE == 0 now...), but I also think you should not have added
>       the test at all.  We will not generate any code in the end within
>       gen_set_gpr, but it allows the routines to be self-contained.  You
>       shouldn't assume what gpr_dst returned.
>       
>       
>     
You're right, gen_set_gpr not need EXT_NONE at all, and we need not condition around gen_set_gpr. 
I think that if we know the dst_ext is EXT_NONE, we do't need gen_set_gpr.  I'll correct them on v12. 

Thanks
Song Gao
>       
>       r~
>       
>