Re: [PATCH] target/riscv: fix right shifts shamt value for rv128c
Le 09/07/2022 à 10:52, Weiwei Li a écrit :
在 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
---
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_cb12:s1 5:2 2:1 10:2 3:2 !function=ex_shift_1
%imm_cj12: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.
Ooups! I'll do that.
@@ -82,9 +83,9 @@
@c_addi16sp ... . . . .. imm=%imm_addi16sp rs1=2 rd=2
@c_shift... . .. ... . .. \
- rd=%rs1_3 rs1=%rs1_3 shamt=%shimm_6bit
+ rd=%rs1_3 rs1=%rs1_3 shamt=%shrimm_6bit
@c_shift2 ... . .. ... . .. \
- rd=%rd rs1=%rd shamt=%shimm_6bit
+ rd=%rd rs1=%rd shamt=%shlimm_6bit
@c_andi ... . .. ... . .. 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(, isa);
-decode_inst_operands();
+decode_inst_operands(, isa);
decode_inst_decompress(, isa);
decode_inst_lift_pseudo();
format_inst(buf, buflen, 16, );
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;
}
Indeed! Thanks for pointing that mistake out.
Also 0 becoming 64 is a rv128 only thing, and a shamt of zero should be a
hint in rv64/rv32, so I believe the immediat should not be changed.
Re: [PATCH] target/riscv: fix right shifts shamt value for rv128c
在 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
---
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_cb12:s1 5:2 2:1 10:2 3:2 !function=ex_shift_1
%imm_cj12: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 ... . . . .. imm=%imm_addi16sp rs1=2 rd=2
@c_shift... . .. ... . .. \
- rd=%rs1_3 rs1=%rs1_3 shamt=%shimm_6bit
+ rd=%rs1_3 rs1=%rs1_3 shamt=%shrimm_6bit
@c_shift2 ... . .. ... . .. \
- rd=%rd rs1=%rd shamt=%shimm_6bit
+ rd=%rd rs1=%rd shamt=%shlimm_6bit
@c_andi ... . .. ... . .. 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(, isa);
-decode_inst_operands();
+decode_inst_operands(, isa);
decode_inst_decompress(, isa);
decode_inst_lift_pseudo();
format_inst(buf, buflen, 16, );
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"
[PATCH] target/riscv: fix right shifts shamt value for rv128c
For rv128c right shifts, the 6-bit shamt is sign extended to 7 bits.
Signed-off-by: Frédéric Pétrot
---
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_cb12:s1 5:2 2:1 10:2 3:2 !function=ex_shift_1
%imm_cj12: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
@@ -82,9 +83,9 @@
@c_addi16sp ... . . . .. imm=%imm_addi16sp rs1=2 rd=2
@c_shift... . .. ... . .. \
- rd=%rs1_3 rs1=%rs1_3 shamt=%shimm_6bit
+ rd=%rs1_3 rs1=%rs1_3 shamt=%shrimm_6bit
@c_shift2 ... . .. ... . .. \
- rd=%rd rs1=%rd shamt=%shimm_6bit
+ rd=%rd rs1=%rd shamt=%shlimm_6bit
@c_andi ... . .. ... . .. 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(, isa);
-decode_inst_operands();
+decode_inst_operands(, isa);
decode_inst_decompress(, isa);
decode_inst_lift_pseudo();
format_inst(buf, buflen, 16, );
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;
+}
+
/* Include the auto-generated decoder for 32 bit insn */
#include "decode-insn32.c.inc"
--
2.36.1
