Re: [PATCH] [PATCH] disas/riscv Fix ctzw disassemble

[Ivan Klokov]

Hello, Palmer!

Thanks for your reviewing

I'm sorry, I sent V2 patch, but forgot to add the appropriate tag.

Please see 
https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg05278.html

It was also reviewed by Daniel Henrique Barboza and weiwei

02.03.2023 3:32, Palmer Dabbelt пишет:
> On Fri, 17 Feb 2023 07:45:14 PST (-0800), dbarboza@ventanamicro.com 
> wrote:
> > On 2/17/23 12:14, Ivan Klokov wrote:
> > > Due to typo in opcode list, ctzw is disassembled as clzw instruction.
> >
> > The code was added by 02c1b569a15b4b06a so I believe a "Fixes:" tag 
> > is in
> > order:
> >
> > Fixes: 02c1b569a15b ("disas/riscv: Add Zb[abcs] instructions")
> >
> > > Signed-off-by: Ivan Klokov <ivan.klokov@syntacore.com>
> > > ---
> > >   disas/riscv.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > index ddda687c13..d0639cd047 100644
> > > --- a/disas/riscv.c
> > > +++ b/disas/riscv.c
> > > @@ -1644,7 +1644,7 @@ const rv_opcode_data opcode_data[] = {
> > >       { "minu", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> > >       { "max", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> > >       { "maxu", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> > > -    { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> > > +    { "ctzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> > >       { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> >
> >
> > Does the order matter here? This patch is putting ctzw before clzw, 
> > but 20 lines
> > or so before we have "clz" after "ctz".
>
> IIUC the ordering does matter: the values in rv_op_* need to match the 
> index of opcode_data[].  decode_inst_opcode() fills out rv_op_*, and 
> then the various decode bits (with format_inst() being the most 
> relevant as it looks at the name field).
>
> So unless I'm missing something, the correct patch should look like
>
>    diff --git a/disas/riscv.c b/disas/riscv.c
>    index ddda687c13..54455aaaa8 100644
>    --- a/disas/riscv.c
>    +++ b/disas/riscv.c
>    @@ -1645,7 +1645,7 @@ const rv_opcode_data opcode_data[] = {
>         { "max", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
>         { "maxu", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
>         { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
>    -    { "clzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
>    +    { "ctzw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
>         { "cpopw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
>         { "slli.uw", rv_codec_i_sh5, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },
>         { "add.uw", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
>
> The threading seems to have gotten a little screwed up with the v2 so 
> sorry if
> I missed something, but I didn't see one with the ordering changed.  I 
> stuck
> what I think is a correct patch over at
> <https://github.com/qemu/qemu/commit/09da30795bcca53447a6f6f9dde4aa91a48f8a01>, 
>
> LMK if that's OK (or just send a v3).
>
> > If the order doesn't matter I think it would be nice to put ctzw 
> > after clzw.
> >
> >
> >
> > Thanks,
> >
> >
> > Daniel
> >
> > >       { "cpopw", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 },
> > >       { "slli.uw", rv_codec_i_sh5, rv_fmt_rd_rs1_imm, NULL, 0, 0, 0 },