Re: [PATCH] target/riscv: fix right shifts shamt value for rv128c

[Weiwei Li]

在 2022/7/8 下午11:00, Frédéric Pétrot 写道:

> For rv128c right shifts, the 6-bit shamt is sign extended to 7 bits.
>
> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> ---
>  target/riscv/insn16.decode |  7 ++++---
>  disas/riscv.c              | 27 +++++++++++++++++++++------
>  target/riscv/translate.c   | 12 +++++++++++-
>  3 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/insn16.decode b/target/riscv/insn16.decode
> index 02c8f61b48..ea3c5a0411 100644
> --- a/target/riscv/insn16.decode
> +++ b/target/riscv/insn16.decode
> @@ -31,7 +31,8 @@
>  %imm_cb        12:s1 5:2 2:1 10:2 3:2 !function=ex_shift_1
>  %imm_cj        12:s1 8:1 9:2 6:1 7:1 2:1 11:1 3:3 !function=ex_shift_1
>  
> -%shimm_6bit   12:1 2:5               !function=ex_rvc_shifti
> +%shlimm_6bit   12:1 2:5              !function=ex_rvc_shiftli
> +%shrimm_6bit   12:1 2:5              !function=ex_rvc_shiftri
>  %uimm_6bit_lq 2:4 12:1 6:1           !function=ex_shift_4
>  %uimm_6bit_ld 2:3 12:1 5:2           !function=ex_shift_3
>  %uimm_6bit_lw 2:2 12:1 4:3           !function=ex_shift_2
>

It's better to maintain the alignment here.

> @@ -82,9 +83,9 @@
>  @c_addi16sp     ... .  ..... ..... .. &i imm=%imm_addi16sp rs1=2 rd=2
>  
>  @c_shift        ... . .. ... ..... .. \
> -                &shift rd=%rs1_3 rs1=%rs1_3 shamt=%shimm_6bit
> +                &shift rd=%rs1_3 rs1=%rs1_3 shamt=%shrimm_6bit
>  @c_shift2       ... . .. ... ..... .. \
> -                &shift rd=%rd rs1=%rd shamt=%shimm_6bit
> +                &shift rd=%rd rs1=%rd shamt=%shlimm_6bit
>  
>  @c_andi         ... . .. ... ..... .. &i imm=%imm_ci rs1=%rs1_3 rd=%rs1_3
>  
> diff --git a/disas/riscv.c b/disas/riscv.c
> index 7af6afc8fa..489c2ae5e8 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -2402,10 +2402,25 @@ static int32_t operand_sbimm12(rv_inst inst)
>          ((inst << 56) >> 63) << 11;
>  }
>  
> -static uint32_t operand_cimmsh6(rv_inst inst)
> +static uint32_t operand_cimmshl6(rv_inst inst, rv_isa isa)
>  {
> -    return ((inst << 51) >> 63) << 5 |
> +    int imm = ((inst << 51) >> 63) << 5 |
>          (inst << 57) >> 59;
> +    if (isa == rv128) {
> +        imm = imm ? imm : 64;
> +    }
> +    return imm;
> +}
> +
> +static uint32_t operand_cimmshr6(rv_inst inst, rv_isa isa)
> +{
> +    int imm = ((inst << 51) >> 63) << 5 |
> +        (inst << 57) >> 59;
> +    if (isa == rv128) {
> +        imm = imm | (imm & 32) << 1;
> +        imm = imm ? imm : 64;
> +    }
> +    return imm;
>  }
>  
>  static int32_t operand_cimmi(rv_inst inst)
> @@ -2529,7 +2544,7 @@ static uint32_t operand_rnum(rv_inst inst)
>  
>  /* decode operands */
>  
> -static void decode_inst_operands(rv_decode *dec)
> +static void decode_inst_operands(rv_decode *dec, rv_isa isa)
>  {
>      rv_inst inst = dec->inst;
>      dec->codec = opcode_data[dec->op].codec;
> @@ -2652,7 +2667,7 @@ static void decode_inst_operands(rv_decode *dec)
>      case rv_codec_cb_sh6:
>          dec->rd = dec->rs1 = operand_crs1rdq(inst) + 8;
>          dec->rs2 = rv_ireg_zero;
> -        dec->imm = operand_cimmsh6(inst);
> +        dec->imm = operand_cimmshr6(inst, isa);
>          break;
>      case rv_codec_ci:
>          dec->rd = dec->rs1 = operand_crs1rd(inst);
> @@ -2667,7 +2682,7 @@ static void decode_inst_operands(rv_decode *dec)
>      case rv_codec_ci_sh6:
>          dec->rd = dec->rs1 = operand_crs1rd(inst);
>          dec->rs2 = rv_ireg_zero;
> -        dec->imm = operand_cimmsh6(inst);
> +        dec->imm = operand_cimmshl6(inst, isa);
>          break;
>      case rv_codec_ci_16sp:
>          dec->rd = rv_ireg_sp;
> @@ -3193,7 +3208,7 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst)
>      dec.pc = pc;
>      dec.inst = inst;
>      decode_inst_opcode(&dec, isa);
> -    decode_inst_operands(&dec);
> +    decode_inst_operands(&dec, isa);
>      decode_inst_decompress(&dec, isa);
>      decode_inst_lift_pseudo(&dec);
>      format_inst(buf, buflen, 16, &dec);
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 63b04e8a94..af3a2cd68c 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -705,12 +705,22 @@ static int ex_rvc_register(DisasContext *ctx, int reg)
>      return 8 + reg;
>  }
>  
> -static int ex_rvc_shifti(DisasContext *ctx, int imm)
> +static int ex_rvc_shiftli(DisasContext *ctx, int imm)
>  {
>      /* For RV128 a shamt of 0 means a shift by 64. */
>      return imm ? imm : 64;
>  }
>  
> +static int ex_rvc_shiftri(DisasContext *ctx, int imm)
> +{
> +    /*
> +     * For RV128 a shamt of 0 means a shift by 64, furthermore, for right
> +     * shifts, the shamt is sign-extended.
> +     */
> +    imm = imm | (imm & 32) << 1;
> +    return imm ? imm : 64;
> +}
> +
>

This calculation didn't add limitation for only working in RV128, and may trigger fault in RV32/RV64,

such as following check in gen_shift_imm_fn:

if (a->shamt >= max_len) {

return false;

}

Regards,

Weiwei Li

>  /* Include the auto-generated decoder for 32 bit insn */
>  #include "decode-insn32.c.inc"
>  
>