Re: [PATCH 3/7] disas/riscv.c: Support disas for Zcm* extensions

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 5/18/23 23:19, Weiwei Li wrote:
> Support disas for Zcmt* instructions only when related extensions
> are supported.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   disas/riscv.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/disas/riscv.c b/disas/riscv.c
> index 729ab684da..9e01810eef 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa)
>                   op = rv_op_c_sqsp;
>               } else {
>                   op = rv_op_c_fsdsp;
> -                if (((inst >> 12) & 0b01)) {
> +                if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) {
>                       switch ((inst >> 8) & 0b01111) {
>                       case 8:
>                           if (((inst >> 4) & 0b01111) >= 4) {
> @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa)
>                   } else {
>                       switch ((inst >> 10) & 0b011) {
>                       case 0:
> -                        if (((inst >> 2) & 0xFF) >= 32) {
> -                            op = rv_op_cm_jalt;
> -                        } else {
> -                            op = rv_op_cm_jt;
> +                        if (dec->cfg->ext_zcmt) {
> +                            if (((inst >> 2) & 0xFF) >= 32) {
> +                                op = rv_op_cm_jalt;
> +                            } else {
> +                                op = rv_op_cm_jt;
> +                            }

In this code, since you're not doing anything if dec->cfg->ext_zcmt is not set,
you could also break earlier and avoid changing the other lines:


>                       case 0:
> +                     if (!dec->cfg->ext_zcmt) {
> +                         break;
> +                     }
>                       if (((inst >> 2) & 0xFF) >= 32) {
>                           op = rv_op_cm_jalt;
>                       } else {
>                           op = rv_op_cm_jt;
>                       }
>                       break;

     >                           }
>                           break;
>                       case 3:
> -                        switch ((inst >> 5) & 0b011) {
> -                        case 1: op = rv_op_cm_mvsa01; break;
> -                        case 3: op = rv_op_cm_mva01s; break;
> +                        if (dec->cfg->ext_zcmp) {
> +                            switch ((inst >> 5) & 0b011) {
> +                            case 1: op = rv_op_cm_mvsa01; break;
> +                            case 3: op = rv_op_cm_mva01s; break;
> +                            }


Same thing here.


These are minor stylistic comments.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>                           }
>                           break;
>                       }