| Message ID | 20230217151459.54649-1-ivan.klokov@syntacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | disas/riscv Fix ctzw disassemble | expand |
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". 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 },
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 },
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 }, >
On Fri, 17 Feb 2023 07:14:59 PST (-0800), ivan.klokov@syntacore.com wrote: > Due to typo in opcode list, ctzw is disassembled as clzw instruction. > > 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 }, > { "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 }, Thanks, this is queue up on riscv-to-apply.next -- I think I managed to get all the reviews and such from the mailing list, it got a bit confused. Here's what I've got: commit 270629024df1f9f4e704ce8325f958858c5cbff7 gpg: Signature made Sun 05 Mar 2023 12:43:52 PM PST gpg: using RSA key 2B3C3747446843B24A943A7A2E1319F35FBB1889 gpg: issuer "palmer@dabbelt.com" gpg: Good signature from "Palmer Dabbelt <palmer@dabbelt.com>" [ultimate] gpg: aka "Palmer Dabbelt <palmer@rivosinc.com>" [ultimate] Author: Ivan Klokov <ivan.klokov@syntacore.com> Date: Fri Feb 17 18:14:59 2023 +0300 disas/riscv Fix ctzw disassemble Due to typo in opcode list, ctzw is disassembled as clzw instruction. Signed-off-by: Ivan Klokov <ivan.klokov@syntacore.com> Fixes: 02c1b569a15b ("disas/riscv: Add Zb[abcs] instructions") Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Message-ID: <20230217151459.54649-1-ivan.klokov@syntacore.com> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> 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 },
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 }, { "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 },
Due to typo in opcode list, ctzw is disassembled as clzw instruction. Signed-off-by: Ivan Klokov <ivan.klokov@syntacore.com> --- disas/riscv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)