1. Patchew
  2. QEMU
  3. View series

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

Ivan Klokov posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230217151459.54649-1-ivan.klokov@syntacore.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>
There is a newer version of this series
disas/riscv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] [PATCH] disas/riscv Fix ctzw disassemble
Posted by Ivan Klokov 1 year, 2 months ago 
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 },
-- 
2.34.1
Re: [PATCH] [PATCH] disas/riscv Fix ctzw disassemble
Posted by Palmer Dabbelt 1 year, 2 months ago 
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 },
Re: [PATCH] [PATCH] disas/riscv Fix ctzw disassemble
Posted by Daniel Henrique Barboza 1 year, 2 months ago 

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 },
Re: [PATCH] [PATCH] disas/riscv Fix ctzw disassemble
Posted by Palmer Dabbelt 1 year, 2 months ago 
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 },
Re: [PATCH] [PATCH] disas/riscv Fix ctzw disassemble
Posted by Ivan Klokov 1 year, 2 months ago 
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 },
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.