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

[gaosong]

Hi Richard,

On 2021/11/12 下午10:05, Richard Henderson wrote:

> On
>         11/12/21 7:53 AM, Song Gao wrote:
>       
>       
> > +#
> >         
> >         +# Fields
> >         
> >         +#
> >         
> >         +%rd      0:5
> >         
> >         +%rj      5:5
> >         
> >         +%rk      10:5
> >         
> >         +%sa2     15:2
> >         
> >         +%si12    10:s12
> >         
> >         +%ui12    10:12
> >         
> >         +%si16    10:s16
> >         
> >         +%si20    5:s20
> >         
> >         
>       
>       You should only create separate field definitions like
>         this when they are complex: e.g. the logical field is disjoint
>         or there's a need for !function.
>       
>       
>       
>       
> > +
> >         
> >         +#
> >         
> >         +# Argument sets
> >         
> >         +#
> >         
> >         +&fmt_rdrjrk         rd rj rk
> >         
> >         +&fmt_rdrjsi12       rd rj si12
> >         
> >         +&fmt_rdrjrksa2      rd rj rk sa2
> >         
> >         +&fmt_rdrjsi16       rd rj si16
> >         
> >         +&fmt_rdrjui12       rd rj ui12
> >         
> >         +&fmt_rdsi20         rd si20
> >         
> >         
>       
>       Some of these should be combined.  The width of the
>         immediate is a detail of the format, not the decoded argument
>         set.  Thus you should have
>       
>       
>       
>       &fmt_rdimm     rd imm
>       
>       &fmt_rdrjimm   rd rj imm
>       
>       &fmt_rdrjrk    rd rj rk
>       
>       &fmt_rdrjrksa  rd rj rk sa
>       
>       
>       
>     
'The width of the immediate is a detail of the format'  means:

&fmt_rdrjimm         rd  rj imm 

@fmt_rdrjimm         .... ...... imm:12  rj:5 rd:5     &fmt_rdrjimm
@fmt_rdrjimm14         .... .... imm:14  rj:5 rd:5     &fmt_rdrjimm 
@fmt_rdrjimm16           .... .. imm:16  rj:5 rd:5     &fmt_rdrjimm

and we print in the disassembly, liks this 

output_rdrjimm(DisasContext *ctx, arg_fmt_rdrjimm * a,  const char *mnemonic)
{
    output(ctx, mnemonic, "%s, %s, 0x%x", regnames[a->rd], regnames[a->rj], a->imm);
}

is that right? 
> > +alsl_w           0000 00000000 010 ..
> >           ..... ..... .....   @fmt_rdrjrksa2
> >         
> >         +alsl_wu          0000 00000000 011 .. ..... .....
> >           .....   @fmt_rdrjrksa2
> >         
> >         +alsl_d           0000 00000010 110 .. ..... .....
> >           .....   @fmt_rdrjrksa2
> >         
> >         
>       
>       The encoding of these insns is that the shift is sa+1.
>       
>       
>       
>       While you compensate for this in gen_alsl_*, we print the
>         "wrong" number in the disassembly.  I think it would be better
>         to do
>       
>       
>       
>       %sa2p1     15:2 !function=plus_1
>       
>       @fmt_rdrjrksa2p1  .... ........ ... .. rk:5 rj:5 rd:5 \
>       
>                         &fmt_rdrjrksa sa=%sa2p1
>       
>       
>       
>     
1. We print sa in disassembly 
output_rdrjrksa(DisasContext *ctx, arg_fmt_rdrjsa* a, const char *memonic)
{
    output(ctx, memonic, "%s, %s, %s, 0x0x", regnames[a->rd], regnames[a->rj], a->sa)
}

2. We use sa on gen_alsl_*  not  (sa2+1). 

3 bytepick_w use the same print functions. but the Field/Argurment/Format is 
%sa2      15:2
&fmt_rdrjrksa   rd rj sa
@fmt_rdrjrk sa2  .... ........ ...  sa:2  rk:5 rj:5 rd:5  &fmt_rdrjrksa

Is my understanding right?

Thanks.
Song Gao