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

[gaosong]

Hi Richard,

On 2021/11/15 下午4:42, Richard Henderson wrote:

> On
>       11/15/21 4:59 AM, gaosong wrote:
>       
> > '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?
> >         
> >       
>       
>       Yes.
>       
>       
>       I'll note that regnames[] is defined in target/loongarch/cpu.c,
>       which is not available when we want to use this disassembler for
>       tcg/loongarch64/.  I think it would be easier to print this as
>       
>       
>           "r%d", a->rd
>       
>       
>       so that you do not need to rely on the external strings.
>       
>       
>       I also think you should print signed numbers, "%d", because
>       0xfffffff8 (truncated to 32 bits) is not really the correct
>       representation of -8 for a 64-bit operand.
>       
>       
>       
> > 1. We print sa in disassembly...
> >         
> >         2. We use sa on gen_alsl_* not (sa2+1).
> >         
> >         3. bytepick_w use the same print functions.
> >         
> >         Is my understanding right?
> >         
> >       
>       
>       Yes, that is the issue I am describing.
>       
>       
>

I see that  insns.decode format is not very consistent with other architectures, such ARM/RISCV

I'll correct it , like this:

# Fields
#
%sa2p1     15:2         !function=plus_1

#
# Argument sets
#
&r_i          rd imm
&rrr          rd rj rk
&rr_i         rd rj imm
&rrr_sa     rd rj rk sa

#
# Formats
#
@fmt_rrr             .... ........ ..... rk:5 rj:5 rd:5          &rrr
@fmt_r_i20                        .... ... imm:s20 rd:5    &r_i
@fmt_rr_i12               .... ...... imm:s12 rj:5 rd:5   &rr_i
@fmt_rr_ui12               .... ...... imm:12 rj:5 rd:5   &rr_i
@fmt_rr_i16                   .... .. imm:s16 rj:5 rd:5   &rr_i
@fmt_rrr_sa2p1      .... ........ ... .. rk:5 rj:5 rd:5     &rrr_sa  sa=%sa2p1

#
# Fixed point arithmetic operation instruction
#
add_w            0000 00000001 00000 ..... ..... .....    @fmt_rrr
add_d            0000 00000001 00001 ..... ..... .....    @fmt_rrr
sub_w            0000 00000001 00010 ..... ..... .....    @fmt_rrr
sub_d            0000 00000001 00011 ..... ..... .....    @fmt_rrr
slt              0000 00000001 00100 ..... ..... .....       @fmt_rrr
sltu             0000 00000001 00101 ..... ..... .....      @fmt_rrr
slti             0000 001000 ............ ..... .....               @fmt_rr_i12

and trans_xxx.c.inc

static bool gen_rrr(DisasContext *ctx, arg_rrr *a, ...) {}
static bool gen_rr_i12(DisasContext *ctx, arg_rr_i *a, ) {}
static bool gen_rrr_sa2p1(DisasContext *ctx, arg_rrr_sa *a, ...) {}
...

Richard, is that OK?

Thanks,
Song Gao