* [PATCH v3 1/4] RISC-V: add vector extension field in CPURISCVState
2020-01-03 3:33 [PATCH v3 0/4] RISC-V: support vector extension part 1 LIU Zhiwei
@ 2020-01-03 3:33 ` LIU Zhiwei
2020-01-03 23:05 ` Richard Henderson
2020-01-03 3:33 ` [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line LIU Zhiwei
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-03 3:33 UTC (permalink / raw)
To: alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768, LIU Zhiwei
The 32 vector registers will be viewed as a continuous memory block.
It avoids the convension between element index and (regno,offset).
Thus elements can be directly accessed by offset from the first vector
base address.
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
target/riscv/cpu.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0adb307f32..af66674461 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -93,9 +93,23 @@ typedef struct CPURISCVState CPURISCVState;
#include "pmp.h"
+#define RV_VLEN_MAX 4096
+
struct CPURISCVState {
target_ulong gpr[32];
uint64_t fpr[32]; /* assume both F and D extensions */
+
+ /* vector coprocessor state. */
+ struct {
+ uint64_t vreg[32 * RV_VLEN_MAX / 64];
+ target_ulong vxrm;
+ target_ulong vxsat;
+ target_ulong vl;
+ target_ulong vstart;
+ target_ulong vtype;
+ } vext;
+
+ bool foflag;
target_ulong pc;
target_ulong load_res;
target_ulong load_val;
--
2.23.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/4] RISC-V: add vector extension field in CPURISCVState
2020-01-03 3:33 ` [PATCH v3 1/4] RISC-V: add vector extension field in CPURISCVState LIU Zhiwei
@ 2020-01-03 23:05 ` Richard Henderson
0 siblings, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-01-03 23:05 UTC (permalink / raw)
To: LIU Zhiwei, alistair23, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 1/3/20 2:33 PM, LIU Zhiwei wrote:
> The 32 vector registers will be viewed as a continuous memory block.
> It avoids the convension between element index and (regno,offset).
> Thus elements can be directly accessed by offset from the first vector
> base address.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
> target/riscv/cpu.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0adb307f32..af66674461 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -93,9 +93,23 @@ typedef struct CPURISCVState CPURISCVState;
>
> #include "pmp.h"
>
> +#define RV_VLEN_MAX 4096
> +
> struct CPURISCVState {
> target_ulong gpr[32];
> uint64_t fpr[32]; /* assume both F and D extensions */
> +
> + /* vector coprocessor state. */
> + struct {
> + uint64_t vreg[32 * RV_VLEN_MAX / 64];
Missing alignment.
> + target_ulong vxrm;
> + target_ulong vxsat;
> + target_ulong vl;
> + target_ulong vstart;
> + target_ulong vtype;
> + } vext;
> +
> + bool foflag;
Remove this. As discussed before, you don't need it. Faulting behaviour
should be handled with the proper interfaces.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line
2020-01-03 3:33 [PATCH v3 0/4] RISC-V: support vector extension part 1 LIU Zhiwei
2020-01-03 3:33 ` [PATCH v3 1/4] RISC-V: add vector extension field in CPURISCVState LIU Zhiwei
@ 2020-01-03 3:33 ` LIU Zhiwei
2020-01-03 23:08 ` Richard Henderson
2020-01-06 21:48 ` Jim Wilson
2020-01-03 3:33 ` [PATCH v3 3/4] RISC-V: support vector extension csr LIU Zhiwei
2020-01-03 3:33 ` [PATCH v3 4/4] RISC-V: add vector extension configure instruction LIU Zhiwei
3 siblings, 2 replies; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-03 3:33 UTC (permalink / raw)
To: alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768, LIU Zhiwei
Vector extension is default on only for "any" cpu. It can be turned
on by command line "-cpu rv64,vlen=128,elen=64,vext_spec=v0.7.1".
vlen is the vector register length, default value is 128 bit.
elen is the max operator size in bits, default value is 64 bit.
vext_spec is the vector specification version, default value is v0.7.1.
Thest properties and cpu can be specified with other values.
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
target/riscv/cpu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
target/riscv/cpu.h | 8 ++++++++
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f8d07bd20a..c2370a0a57 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -94,6 +94,11 @@ static void set_priv_version(CPURISCVState *env, int priv_ver)
env->priv_ver = priv_ver;
}
+static void set_vext_version(CPURISCVState *env, int vext_ver)
+{
+ env->vext_ver = vext_ver;
+}
+
static void set_feature(CPURISCVState *env, int feature)
{
env->features |= (1ULL << feature);
@@ -109,7 +114,7 @@ static void set_resetvec(CPURISCVState *env, int resetvec)
static void riscv_any_cpu_init(Object *obj)
{
CPURISCVState *env = &RISCV_CPU(obj)->env;
- set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+ set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV);
set_priv_version(env, PRIV_VERSION_1_11_0);
set_resetvec(env, DEFAULT_RSTVEC);
}
@@ -317,6 +322,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
CPURISCVState *env = &cpu->env;
RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
int priv_version = PRIV_VERSION_1_11_0;
+ int vext_version = VEXT_VERSION_0_07_1;
target_ulong target_misa = 0;
Error *local_err = NULL;
@@ -340,8 +346,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
return;
}
}
-
+ if (cpu->cfg.vext_spec) {
+ if (!g_strcmp0(cpu->cfg.vext_spec, "v0.7.1")) {
+ vext_version = VEXT_VERSION_0_07_1;
+ } else {
+ error_setg(errp,
+ "Unsupported vector spec version '%s'",
+ cpu->cfg.vext_spec);
+ return;
+ }
+ }
set_priv_version(env, priv_version);
+ set_vext_version(env, vext_version);
set_resetvec(env, DEFAULT_RSTVEC);
if (cpu->cfg.mmu) {
@@ -406,6 +422,24 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
if (cpu->cfg.ext_u) {
target_misa |= RVU;
}
+ if (cpu->cfg.ext_v) {
+ target_misa |= RVV;
+ if (!is_power_of_2(cpu->cfg.vlen)) {
+ error_setg(errp,
+ "Vector extension VLEN must be power of 2");
+ return;
+ }
+ if (cpu->cfg.vlen > RV_VLEN_MAX) {
+ error_setg(errp,
+ "Vector extension VLEN must <= %d", RV_VLEN_MAX);
+ return;
+ }
+ if (!is_power_of_2(cpu->cfg.elen)) {
+ error_setg(errp,
+ "Vector extension ELEN must be power of 2");
+ return;
+ }
+ }
set_misa(env, RVXLEN | target_misa);
}
@@ -441,10 +475,14 @@ static Property riscv_cpu_properties[] = {
DEFINE_PROP_BOOL("c", RISCVCPU, cfg.ext_c, true),
DEFINE_PROP_BOOL("s", RISCVCPU, cfg.ext_s, true),
DEFINE_PROP_BOOL("u", RISCVCPU, cfg.ext_u, true),
+ DEFINE_PROP_BOOL("v", RISCVCPU, cfg.ext_v, false),
DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true),
DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
+ DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
+ DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
+ DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
DEFINE_PROP_END_OF_LIST(),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index af66674461..d0b106583a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -64,6 +64,7 @@
#define RVA RV('A')
#define RVF RV('F')
#define RVD RV('D')
+#define RVV RV('V')
#define RVC RV('C')
#define RVS RV('S')
#define RVU RV('U')
@@ -82,6 +83,8 @@ enum {
#define PRIV_VERSION_1_10_0 0x00011000
#define PRIV_VERSION_1_11_0 0x00011100
+#define VEXT_VERSION_0_07_1 0x00000071
+
#define TRANSLATE_PMP_FAIL 2
#define TRANSLATE_FAIL 1
#define TRANSLATE_SUCCESS 0
@@ -119,6 +122,7 @@ struct CPURISCVState {
target_ulong badaddr;
target_ulong priv_ver;
+ target_ulong vext_ver;
target_ulong misa;
target_ulong misa_mask;
@@ -236,12 +240,16 @@ typedef struct RISCVCPU {
bool ext_c;
bool ext_s;
bool ext_u;
+ bool ext_v;
bool ext_counters;
bool ext_ifencei;
bool ext_icsr;
char *priv_spec;
char *user_spec;
+ char *vext_spec;
+ uint16_t vlen;
+ uint16_t elen;
bool mmu;
bool pmp;
} cfg;
--
2.23.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line
2020-01-03 3:33 ` [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line LIU Zhiwei
@ 2020-01-03 23:08 ` Richard Henderson
2020-01-06 21:48 ` Jim Wilson
1 sibling, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-01-03 23:08 UTC (permalink / raw)
To: LIU Zhiwei, alistair23, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 1/3/20 2:33 PM, LIU Zhiwei wrote:
> + if (cpu->cfg.ext_v) {
> + target_misa |= RVV;
> + if (!is_power_of_2(cpu->cfg.vlen)) {
> + error_setg(errp,
> + "Vector extension VLEN must be power of 2");
> + return;
> + }
> + if (cpu->cfg.vlen > RV_VLEN_MAX) {
> + error_setg(errp,
> + "Vector extension VLEN must <= %d", RV_VLEN_MAX);
> + return;
> + }
> + if (!is_power_of_2(cpu->cfg.elen)) {
> + error_setg(errp,
> + "Vector extension ELEN must be power of 2");
> + return;
> + }
Missing maximum on elen.
Missing minimum on vlen, which, as I discussed earlier, should be 128 to avoid
asserts in tcg-op-gvec.c.
> #define PRIV_VERSION_1_10_0 0x00011000
> #define PRIV_VERSION_1_11_0 0x00011100
>
> +#define VEXT_VERSION_0_07_1 0x00000071
To match the other version number formats, surely this should be 0x00000701.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line
2020-01-03 3:33 ` [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line LIU Zhiwei
2020-01-03 23:08 ` Richard Henderson
@ 2020-01-06 21:48 ` Jim Wilson
2020-01-07 1:42 ` LIU Zhiwei
1 sibling, 1 reply; 14+ messages in thread
From: Jim Wilson @ 2020-01-06 21:48 UTC (permalink / raw)
To: LIU Zhiwei, alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 1/2/20 7:33 PM, LIU Zhiwei wrote:
> + if (cpu->cfg.vlen > RV_VLEN_MAX) {
> + error_setg(errp,
> + "Vector extension VLEN must <= %d", RV_VLEN_MAX);
> + return;
There is no architectural maximum for VLEN. This is simply an
implementation choice so you can use static arrays instead of malloc. I
think this error should be reworded to something like "Vector extension
implementation only supports VLEN <= %d."
The other errors here are for architecture requirements and are OK.
Jim
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line
2020-01-06 21:48 ` Jim Wilson
@ 2020-01-07 1:42 ` LIU Zhiwei
0 siblings, 0 replies; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-07 1:42 UTC (permalink / raw)
To: Jim Wilson, alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
[-- Attachment #1: Type: text/plain, Size: 726 bytes --]
On 2020/1/7 5:48, Jim Wilson wrote:
> On 1/2/20 7:33 PM, LIU Zhiwei wrote:
>> + if (cpu->cfg.vlen > RV_VLEN_MAX) {
>> + error_setg(errp,
>> + "Vector extension VLEN must <= %d",
>> RV_VLEN_MAX);
>> + return;
>
> There is no architectural maximum for VLEN. This is simply an
> implementation choice so you can use static arrays instead of malloc.
> I think this error should be reworded to something like "Vector
> extension implementation only supports VLEN <= %d."
Thanks. It's good to reduce ambiguous.
Zhiwei
> The other errors here are for architecture requirements and are OK.
>
> Jim
[-- Attachment #2: Type: text/html, Size: 2017 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 3/4] RISC-V: support vector extension csr
2020-01-03 3:33 [PATCH v3 0/4] RISC-V: support vector extension part 1 LIU Zhiwei
2020-01-03 3:33 ` [PATCH v3 1/4] RISC-V: add vector extension field in CPURISCVState LIU Zhiwei
2020-01-03 3:33 ` [PATCH v3 2/4] RISC-V: configure and turn on vector extension from command line LIU Zhiwei
@ 2020-01-03 3:33 ` LIU Zhiwei
2020-01-03 23:14 ` Richard Henderson
2020-01-06 22:00 ` Jim Wilson
2020-01-03 3:33 ` [PATCH v3 4/4] RISC-V: add vector extension configure instruction LIU Zhiwei
3 siblings, 2 replies; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-03 3:33 UTC (permalink / raw)
To: alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768, LIU Zhiwei
Until v0.7.1 specification, vector status is still not defined for
mstatus.
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
target/riscv/cpu_bits.h | 15 +++++++
target/riscv/csr.c | 92 +++++++++++++++++++++++++++++------------
2 files changed, 80 insertions(+), 27 deletions(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 11f971ad5d..9eb43ecc1e 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -29,6 +29,14 @@
#define FSR_NXA (FPEXC_NX << FSR_AEXC_SHIFT)
#define FSR_AEXC (FSR_NVA | FSR_OFA | FSR_UFA | FSR_DZA | FSR_NXA)
+/* Vector Fixed-Point round model */
+#define FSR_VXRM_SHIFT 9
+#define FSR_VXRM (0x3 << FSR_VXRM_SHIFT)
+
+/* Vector Fixed-Point saturation flag */
+#define FSR_VXSAT_SHIFT 8
+#define FSR_VXSAT (0x1 << FSR_VXSAT_SHIFT)
+
/* Control and Status Registers */
/* User Trap Setup */
@@ -48,6 +56,13 @@
#define CSR_FRM 0x002
#define CSR_FCSR 0x003
+/* User Vector CSRs */
+#define CSR_VSTART 0x008
+#define CSR_VXSAT 0x009
+#define CSR_VXRM 0x00a
+#define CSR_VL 0xc20
+#define CSR_VTYPE 0xc21
+
/* User Timers and Counters */
#define CSR_CYCLE 0xc00
#define CSR_TIME 0xc01
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..506ad7b590 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -53,6 +53,11 @@ static int fs(CPURISCVState *env, int csrno)
return 0;
}
+static int vs(CPURISCVState *env, int csrno)
+{
+ return 0;
+}
+
static int ctr(CPURISCVState *env, int csrno)
{
#if !defined(CONFIG_USER_ONLY)
@@ -107,11 +112,6 @@ static int pmp(CPURISCVState *env, int csrno)
/* User Floating-Point CSRs */
static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
{
-#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
- return -1;
- }
-#endif
*val = riscv_cpu_get_fflags(env);
return 0;
}
@@ -119,9 +119,6 @@ static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
- return -1;
- }
env->mstatus |= MSTATUS_FS;
#endif
riscv_cpu_set_fflags(env, val & (FSR_AEXC >> FSR_AEXC_SHIFT));
@@ -130,11 +127,6 @@ static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
{
-#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
- return -1;
- }
-#endif
*val = env->frm;
return 0;
}
@@ -142,9 +134,6 @@ static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
- return -1;
- }
env->mstatus |= MSTATUS_FS;
#endif
env->frm = val & (FSR_RD >> FSR_RD_SHIFT);
@@ -153,29 +142,73 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
{
-#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
- return -1;
- }
-#endif
- *val = (riscv_cpu_get_fflags(env) << FSR_AEXC_SHIFT)
- | (env->frm << FSR_RD_SHIFT);
+ *val = (env->vext.vxrm << FSR_VXRM_SHIFT)
+ | (env->vext.vxsat << FSR_VXSAT_SHIFT)
+ | (riscv_cpu_get_fflags(env) << FSR_AEXC_SHIFT)
+ | (env->frm << FSR_RD_SHIFT);
return 0;
}
static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
{
#if !defined(CONFIG_USER_ONLY)
- if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
- return -1;
- }
env->mstatus |= MSTATUS_FS;
#endif
env->frm = (val & FSR_RD) >> FSR_RD_SHIFT;
+ env->vext.vxrm = (val & FSR_VXRM) >> FSR_VXRM_SHIFT;
+ env->vext.vxsat = (val & FSR_VXSAT) >> FSR_VXSAT_SHIFT;
riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT);
return 0;
}
+static int read_vtype(CPURISCVState *env, int csrno, target_ulong *val)
+{
+ *val = env->vext.vtype;
+ return 0;
+}
+
+static int read_vl(CPURISCVState *env, int csrno, target_ulong *val)
+{
+ *val = env->vext.vl;
+ return 0;
+}
+
+static int read_vxrm(CPURISCVState *env, int csrno, target_ulong *val)
+{
+ *val = env->vext.vxrm;
+ return 0;
+}
+
+static int read_vxsat(CPURISCVState *env, int csrno, target_ulong *val)
+{
+ *val = env->vext.vxsat;
+ return 0;
+}
+
+static int read_vstart(CPURISCVState *env, int csrno, target_ulong *val)
+{
+ *val = env->vext.vstart;
+ return 0;
+}
+
+static int write_vxrm(CPURISCVState *env, int csrno, target_ulong val)
+{
+ env->vext.vxrm = val;
+ return 0;
+}
+
+static int write_vxsat(CPURISCVState *env, int csrno, target_ulong val)
+{
+ env->vext.vxsat = val;
+ return 0;
+}
+
+static int write_vstart(CPURISCVState *env, int csrno, target_ulong val)
+{
+ env->vext.vstart = val;
+ return 0;
+}
+
/* User Timers and Counters */
static int read_instret(CPURISCVState *env, int csrno, target_ulong *val)
{
@@ -873,7 +906,12 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
[CSR_FFLAGS] = { fs, read_fflags, write_fflags },
[CSR_FRM] = { fs, read_frm, write_frm },
[CSR_FCSR] = { fs, read_fcsr, write_fcsr },
-
+ /* Vector CSRs */
+ [CSR_VSTART] = { vs, read_vstart, write_vstart },
+ [CSR_VXSAT] = { vs, read_vxsat, write_vxsat },
+ [CSR_VXRM] = { vs, read_vxrm, write_vxrm },
+ [CSR_VL] = { vs, read_vl },
+ [CSR_VTYPE] = { vs, read_vtype },
/* User Timers and Counters */
[CSR_CYCLE] = { ctr, read_instret },
[CSR_INSTRET] = { ctr, read_instret },
--
2.23.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/4] RISC-V: support vector extension csr
2020-01-03 3:33 ` [PATCH v3 3/4] RISC-V: support vector extension csr LIU Zhiwei
@ 2020-01-03 23:14 ` Richard Henderson
2020-01-06 22:00 ` Jim Wilson
1 sibling, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-01-03 23:14 UTC (permalink / raw)
To: LIU Zhiwei, alistair23, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 1/3/20 2:33 PM, LIU Zhiwei wrote:
> Until v0.7.1 specification, vector status is still not defined for
> mstatus.
Using "until" imples that v0.7.1 *does* define the vector status.
Better said as "The v0.7.1 specification does not define vector status within
mstatus. A future revision will define the privileged portion of the vector
status."
Although lack of a privileged spec does suggest to me that perhaps we ought to
support the vector extension only for CONFIG_USER_ONLY for now.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] RISC-V: support vector extension csr
2020-01-03 3:33 ` [PATCH v3 3/4] RISC-V: support vector extension csr LIU Zhiwei
2020-01-03 23:14 ` Richard Henderson
@ 2020-01-06 22:00 ` Jim Wilson
2020-01-07 1:34 ` LIU Zhiwei
1 sibling, 1 reply; 14+ messages in thread
From: Jim Wilson @ 2020-01-06 22:00 UTC (permalink / raw)
To: LIU Zhiwei, alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 1/2/20 7:33 PM, LIU Zhiwei wrote:
> Until v0.7.1 specification, vector status is still not defined for
> mstatus.
The v0.8 spec does define a VS bit in mstatus.
> @@ -107,11 +112,6 @@ static int pmp(CPURISCVState *env, int csrno)
> /* User Floating-Point CSRs */
> static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
> {
> -#if !defined(CONFIG_USER_ONLY)
> - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
> - return -1;
> - }
> -#endif
> *val = riscv_cpu_get_fflags(env);
> return 0;
> }
This allows reads of fflags when it doesn't exist, and hence does not
make much sense. Instead of removing the code, you should add a check
for the vector extension, since the vector extension requires that fcsr
exist even if the base architecture doesn't include FP support. Ideally
this should use the VS bit, but if you don't have it then you can just
check to see if the vector extension was enabled as a command line option.
While the vector spec says that fcsr must exist, it doesn't specify that
the FP fields in fcsr are necessarily readable or writable when there is
no FP. It also doesn't specify whether the other FP related shadows of
fcsr exist, like fflags. This appears to have been left unspecified. I
don't think that you should be making fflags reads and writes work for a
target with vector but without float. I think it would make more sense
to have fcsr behave 3 different ways depending on whether we have only
F, only V, or both F and V. And then we can support reads and writes of
only the valid fields.
Jim
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 3/4] RISC-V: support vector extension csr
2020-01-06 22:00 ` Jim Wilson
@ 2020-01-07 1:34 ` LIU Zhiwei
0 siblings, 0 replies; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-07 1:34 UTC (permalink / raw)
To: Jim Wilson, alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 2020/1/7 6:00, Jim Wilson wrote:
> On 1/2/20 7:33 PM, LIU Zhiwei wrote:
>> Until v0.7.1 specification, vector status is still not defined for
>> mstatus.
>
> The v0.8 spec does define a VS bit in mstatus.
>
Yes, I will also support v0.8 spec after the v0.7.1 spec.
>> @@ -107,11 +112,6 @@ static int pmp(CPURISCVState *env, int csrno)
>> /* User Floating-Point CSRs */
>> static int read_fflags(CPURISCVState *env, int csrno, target_ulong
>> *val)
>> {
>> -#if !defined(CONFIG_USER_ONLY)
>> - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
>> - return -1;
>> - }
>> -#endif
>> *val = riscv_cpu_get_fflags(env);
>> return 0;
>> }
>
> This allows reads of fflags when it doesn't exist, and hence does not
> make much sense. Instead of removing the code, you should add a check
> for the vector extension, since the vector extension requires that
> fcsr exist even if the base architecture doesn't include FP support.
> Ideally this should use the VS bit, but if you don't have it then you
> can just check to see if the vector extension was enabled as a command
> line option.
>
I' sorry that there is some ambiguous here. The reason to remove these
code is that they are redundant, and has nothing to do with the vector
extension. I just delete them by hand.
As you can see, all float csr has a predicate function.
static int fs(CPURISCVState *env, int csrno)
{
#if !defined(CONFIG_USER_ONLY)
if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
return -1;
}
#endif
return 0;
}
The read or write function must be called after the predicate return 0.
So no need to check (!env->debugger && !(env->mstatus & MSTATUS_FS) again.
> While the vector spec says that fcsr must exist, it doesn't specify
> that the FP fields in fcsr are necessarily readable or writable when
> there is no FP. It also doesn't specify whether the other FP related
> shadows of fcsr exist, like fflags. This appears to have been left
> unspecified. I don't think that you should be making fflags reads and
> writes work for a target with vector but without float. I think it
> would make more sense to have fcsr behave 3 different ways depending
> on whether we have only F, only V, or both F and V. And then we can
> support reads and writes of only the valid fields.
>
Thanks. Maybe I should just only loose the check condition for fcsr.
Best Regards,
Zhiwei
> Jim
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 4/4] RISC-V: add vector extension configure instruction
2020-01-03 3:33 [PATCH v3 0/4] RISC-V: support vector extension part 1 LIU Zhiwei
` (2 preceding siblings ...)
2020-01-03 3:33 ` [PATCH v3 3/4] RISC-V: support vector extension csr LIU Zhiwei
@ 2020-01-03 3:33 ` LIU Zhiwei
2020-01-03 23:41 ` Richard Henderson
3 siblings, 1 reply; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-03 3:33 UTC (permalink / raw)
To: alistair23, richard.henderson, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768, LIU Zhiwei
vsetvl and vsetvli are two configure instructions for vl, vtype. TB flags
should update after configure instructions. The (ill, lmul, sew ) of vtype
and the bit of (VSTART == 0 && VL == VLMAX) will be placed within tb_flags.
Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
target/riscv/Makefile.objs | 2 +-
target/riscv/cpu.c | 1 +
target/riscv/cpu.h | 55 ++++++++++++++++++++-----
target/riscv/helper.h | 2 +
target/riscv/insn32.decode | 5 +++
target/riscv/insn_trans/trans_rvv.inc.c | 52 +++++++++++++++++++++++
target/riscv/translate.c | 17 +++++++-
target/riscv/vector_helper.c | 51 +++++++++++++++++++++++
8 files changed, 172 insertions(+), 13 deletions(-)
create mode 100644 target/riscv/insn_trans/trans_rvv.inc.c
create mode 100644 target/riscv/vector_helper.c
diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
index b1c79bc1d1..d577cef9e0 100644
--- a/target/riscv/Makefile.objs
+++ b/target/riscv/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o gdbstub.o pmp.o
+obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o vector_helper.o gdbstub.o pmp.o
DECODETREE = $(SRC_PATH)/scripts/decodetree.py
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c2370a0a57..3ff7b50bff 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -347,6 +347,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
}
}
if (cpu->cfg.vext_spec) {
+ env->vext.vtype = ~((target_ulong)-1 >> 1);
if (!g_strcmp0(cpu->cfg.vext_spec, "v0.7.1")) {
vext_version = VEXT_VERSION_0_07_1;
} else {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index d0b106583a..152a96f1fa 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -23,6 +23,7 @@
#include "qom/cpu.h"
#include "exec/cpu-defs.h"
#include "fpu/softfloat.h"
+#include "hw/registerfields.h"
#define TCG_GUEST_DEFAULT_MO 0
@@ -98,6 +99,20 @@ typedef struct CPURISCVState CPURISCVState;
#define RV_VLEN_MAX 4096
+struct VTYPE {
+#ifdef HOST_WORDS_BIGENDIAN
+ target_ulong vill:1;
+ target_ulong reserved:sizeof(target_ulong) * 8 - 7;
+ target_ulong sew:3;
+ target_ulong lmul:2;
+#else
+ target_ulong lmul:2;
+ target_ulong sew:3;
+ target_ulong reserved:sizeof(target_ulong) * 8 - 7;
+ target_ulong vill:1;
+#endif
+};
+
struct CPURISCVState {
target_ulong gpr[32];
uint64_t fpr[32]; /* assume both F and D extensions */
@@ -309,19 +324,44 @@ void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
-#define TB_FLAGS_MMU_MASK 3
-#define TB_FLAGS_MSTATUS_FS MSTATUS_FS
+typedef CPURISCVState CPUArchState;
+typedef RISCVCPU ArchCPU;
+#include "exec/cpu-all.h"
+
+FIELD(TB_FLAGS, MMU, 0, 2)
+FIELD(TB_FLAGS, FS, 13, 2)
+FIELD(TB_FLAGS, VL_EQ_VLMAX, 16, 1)
+FIELD(TB_FLAGS, LMUL, 17, 2)
+FIELD(TB_FLAGS, SEW, 19, 3)
+FIELD(TB_FLAGS, VILL, 22, 1)
static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
- target_ulong *cs_base, uint32_t *flags)
+ target_ulong *cs_base, uint32_t *pflags)
{
+ RISCVCPU *cpu = env_archcpu(env);
+ struct VTYPE *vtype = (struct VTYPE *)&env->vext.vtype;
+ uint32_t vlmax;
+ uint8_t vl_eq_vlmax;
+ uint32_t flags = 0;
+
*pc = env->pc;
*cs_base = 0;
+ vlmax = (1 << vtype->lmul) * cpu->cfg.vlen / (8 * (1 << vtype->sew));
+ vl_eq_vlmax = (env->vext.vstart == 0) && (vlmax == env->vext.vl);
+
+ flags = FIELD_DP32(flags, TB_FLAGS, VILL, vtype->vill);
+ flags = FIELD_DP32(flags, TB_FLAGS, SEW, vtype->sew);
+ flags = FIELD_DP32(flags, TB_FLAGS, LMUL, vtype->lmul);
+ flags = FIELD_DP32(flags, TB_FLAGS, VL_EQ_VLMAX, vl_eq_vlmax);
+
#ifdef CONFIG_USER_ONLY
- *flags = TB_FLAGS_MSTATUS_FS;
+ flags = FIELD_DP32(flags, TB_FLAGS, FS, MSTATUS_FS);
#else
- *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
+ flags = FIELD_DP32(flags, TB_FLAGS, MMU, cpu_mmu_index(env, 0));
+ flags = FIELD_DP32(flags, TB_FLAGS, FS, (env->mstatus & MSTATUS_FS));
#endif
+ *pflags = flags;
+ *cs_base = 0;
}
int riscv_csrrw(CPURISCVState *env, int csrno, target_ulong *ret_value,
@@ -362,9 +402,4 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
-typedef CPURISCVState CPUArchState;
-typedef RISCVCPU ArchCPU;
-
-#include "exec/cpu-all.h"
-
#endif /* RISCV_CPU_H */
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index debb22a480..000b5aa3d1 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -76,3 +76,5 @@ DEF_HELPER_2(mret, tl, env, tl)
DEF_HELPER_1(wfi, void, env)
DEF_HELPER_1(tlb_flush, void, env)
#endif
+/* Vector functions */
+DEF_HELPER_3(vector_vsetvli, tl, env, tl, tl)
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
index 77f794ed70..5dc009c3cd 100644
--- a/target/riscv/insn32.decode
+++ b/target/riscv/insn32.decode
@@ -62,6 +62,7 @@
@r_rm ....... ..... ..... ... ..... ....... %rs2 %rs1 %rm %rd
@r2_rm ....... ..... ..... ... ..... ....... %rs1 %rm %rd
@r2 ....... ..... ..... ... ..... ....... %rs1 %rd
+@r2_zimm . zimm:11 ..... ... ..... ....... %rs1 %rd
@sfence_vma ....... ..... ..... ... ..... ....... %rs2 %rs1
@sfence_vm ....... ..... ..... ... ..... ....... %rs1
@@ -203,3 +204,7 @@ fcvt_w_d 1100001 00000 ..... ... ..... 1010011 @r2_rm
fcvt_wu_d 1100001 00001 ..... ... ..... 1010011 @r2_rm
fcvt_d_w 1101001 00000 ..... ... ..... 1010011 @r2_rm
fcvt_d_wu 1101001 00001 ..... ... ..... 1010011 @r2_rm
+
+# *** RV32V Extension ***
+vsetvli 0 ........... ..... 111 ..... 1010111 @r2_zimm
+vsetvl 1000000 ..... ..... 111 ..... 1010111 @r
diff --git a/target/riscv/insn_trans/trans_rvv.inc.c b/target/riscv/insn_trans/trans_rvv.inc.c
new file mode 100644
index 0000000000..5d80144502
--- /dev/null
+++ b/target/riscv/insn_trans/trans_rvv.inc.c
@@ -0,0 +1,52 @@
+/*
+ * RISC-V translation routines for the RVV Standard Extension.
+ *
+ * Copyright (c) 2019 C-SKY Limited. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+static bool trans_vsetvl(DisasContext *ctx, arg_vsetvl * a)
+{
+ TCGv s1, s2, d;
+ d = tcg_temp_new();
+ s1 = tcg_temp_new();
+ s2 = tcg_temp_new();
+ gen_get_gpr(s1, a->rs1);
+ gen_get_gpr(s2, a->rs2);
+ gen_helper_vector_vsetvli(d, cpu_env, s1, s2);
+ tcg_gen_st_tl(d, cpu_env, offsetof(CPURISCVState, vext.vl));
+ exit_tb(ctx);
+ ctx->base.is_jmp = DISAS_NORETURN;
+ tcg_temp_free(s1);
+ tcg_temp_free(s2);
+ tcg_temp_free(d);
+ return true;
+}
+
+static bool trans_vsetvli(DisasContext *ctx, arg_vsetvli * a)
+{
+ TCGv s1, s2, d;
+ d = tcg_temp_new();
+ s1 = tcg_temp_new();
+ s2 = tcg_const_tl(a->zimm);
+ gen_get_gpr(s1, a->rs1);
+ gen_helper_vector_vsetvli(d, cpu_env, s1, s2);
+ tcg_gen_st_tl(d, cpu_env, offsetof(CPURISCVState, vext.vl));
+ exit_tb(ctx);
+ ctx->base.is_jmp = DISAS_NORETURN;
+ tcg_temp_free(s1);
+ tcg_temp_free(s2);
+ tcg_temp_free(d);
+ return true;
+}
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 8d6ab73258..beb283b735 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -55,6 +55,12 @@ typedef struct DisasContext {
to reset this known value. */
int frm;
bool ext_ifencei;
+ /* vector extension */
+ bool vill;
+ uint8_t lmul;
+ uint8_t sew;
+ uint16_t vlen;
+ bool vl_eq_vlmax;
} DisasContext;
#ifdef TARGET_RISCV64
@@ -706,6 +712,7 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
#include "insn_trans/trans_rva.inc.c"
#include "insn_trans/trans_rvf.inc.c"
#include "insn_trans/trans_rvd.inc.c"
+#include "insn_trans/trans_rvv.inc.c"
#include "insn_trans/trans_privileged.inc.c"
/*
@@ -754,14 +761,20 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
DisasContext *ctx = container_of(dcbase, DisasContext, base);
CPURISCVState *env = cs->env_ptr;
RISCVCPU *cpu = RISCV_CPU(cs);
+ uint32_t tb_flags = ctx->base.tb->flags;
ctx->pc_succ_insn = ctx->base.pc_first;
- ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;
- ctx->mstatus_fs = ctx->base.tb->flags & TB_FLAGS_MSTATUS_FS;
+ ctx->mem_idx = FIELD_EX32(tb_flags, TB_FLAGS, MMU);
+ ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS);
ctx->priv_ver = env->priv_ver;
ctx->misa = env->misa;
ctx->frm = -1; /* unknown rounding mode */
ctx->ext_ifencei = cpu->cfg.ext_ifencei;
+ ctx->vlen = cpu->cfg.vlen;
+ ctx->vill = FIELD_EX32(tb_flags, TB_FLAGS, VILL);
+ ctx->sew = FIELD_EX32(tb_flags, TB_FLAGS, SEW);
+ ctx->lmul = FIELD_EX32(tb_flags, TB_FLAGS, LMUL);
+ ctx->vl_eq_vlmax = FIELD_EX32(tb_flags, TB_FLAGS, VL_EQ_VLMAX);
}
static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
new file mode 100644
index 0000000000..4e394207ce
--- /dev/null
+++ b/target/riscv/vector_helper.c
@@ -0,0 +1,51 @@
+/*
+ * RISC-V Vectore Extension Helpers for QEMU.
+ *
+ * Copyright (c) 2019 C-SKY Limited. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+#include <math.h>
+
+#define VECTOR_HELPER(name) HELPER(glue(vector_, name))
+
+target_ulong VECTOR_HELPER(vsetvli)(CPURISCVState *env, target_ulong s1,
+ target_ulong s2)
+{
+ int vlmax, vl;
+ RISCVCPU *cpu = env_archcpu(env);
+ struct VTYPE *vtype = (struct VTYPE *)&s2;
+
+ if (vtype->sew > cpu->cfg.elen) { /* only set vill bit. */
+ env->vext.vtype = ~((target_ulong)-1 >> 1);
+ return 0;
+ }
+
+ vlmax = (1 << vtype->lmul) * cpu->cfg.vlen / (8 * (1 << vtype->sew));
+ if (s1 == 0) {
+ vl = vlmax;
+ } else if (s1 <= vlmax) {
+ vl = s1;
+ } else {
+ vl = vlmax;
+ }
+ env->vext.vl = vl;
+ env->vext.vtype = s2;
+ env->vext.vstart = 0;
+ return vl;
+}
--
2.23.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/4] RISC-V: add vector extension configure instruction
2020-01-03 3:33 ` [PATCH v3 4/4] RISC-V: add vector extension configure instruction LIU Zhiwei
@ 2020-01-03 23:41 ` Richard Henderson
2020-01-07 2:11 ` LIU Zhiwei
0 siblings, 1 reply; 14+ messages in thread
From: Richard Henderson @ 2020-01-03 23:41 UTC (permalink / raw)
To: LIU Zhiwei, alistair23, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
On 1/3/20 2:33 PM, LIU Zhiwei wrote:
> vsetvl and vsetvli are two configure instructions for vl, vtype. TB flags
> should update after configure instructions. The (ill, lmul, sew ) of vtype
> and the bit of (VSTART == 0 && VL == VLMAX) will be placed within tb_flags.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
> target/riscv/Makefile.objs | 2 +-
> target/riscv/cpu.c | 1 +
> target/riscv/cpu.h | 55 ++++++++++++++++++++-----
> target/riscv/helper.h | 2 +
> target/riscv/insn32.decode | 5 +++
> target/riscv/insn_trans/trans_rvv.inc.c | 52 +++++++++++++++++++++++
> target/riscv/translate.c | 17 +++++++-
> target/riscv/vector_helper.c | 51 +++++++++++++++++++++++
> 8 files changed, 172 insertions(+), 13 deletions(-)
> create mode 100644 target/riscv/insn_trans/trans_rvv.inc.c
> create mode 100644 target/riscv/vector_helper.c
>
> diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
> index b1c79bc1d1..d577cef9e0 100644
> --- a/target/riscv/Makefile.objs
> +++ b/target/riscv/Makefile.objs
> @@ -1,4 +1,4 @@
> -obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o gdbstub.o pmp.o
> +obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o vector_helper.o gdbstub.o pmp.o
>
> DECODETREE = $(SRC_PATH)/scripts/decodetree.py
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c2370a0a57..3ff7b50bff 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -347,6 +347,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> }
> }
> if (cpu->cfg.vext_spec) {
> + env->vext.vtype = ~((target_ulong)-1 >> 1);
Better as FIELD_DP64(0, VTYPE, VILL, 1),
> +struct VTYPE {
> +#ifdef HOST_WORDS_BIGENDIAN
> + target_ulong vill:1;
> + target_ulong reserved:sizeof(target_ulong) * 8 - 7;
> + target_ulong sew:3;
> + target_ulong lmul:2;
> +#else
> + target_ulong lmul:2;
> + target_ulong sew:3;
> + target_ulong reserved:sizeof(target_ulong) * 8 - 7;
> + target_ulong vill:1;
> +#endif
> +};
Do not use bit fields to describe target register layout.
Use FIELD().
> -#define TB_FLAGS_MMU_MASK 3
> -#define TB_FLAGS_MSTATUS_FS MSTATUS_FS
> +typedef CPURISCVState CPUArchState;
> +typedef RISCVCPU ArchCPU;
> +#include "exec/cpu-all.h"
> +
> +FIELD(TB_FLAGS, MMU, 0, 2)
> +FIELD(TB_FLAGS, FS, 13, 2)
The change to use FIELD for MMU and FS should be made separately from adding
the vector state.
> +FIELD(TB_FLAGS, VL_EQ_VLMAX, 16, 1)
> +FIELD(TB_FLAGS, LMUL, 17, 2)
> +FIELD(TB_FLAGS, SEW, 19, 3)
> +FIELD(TB_FLAGS, VILL, 22, 1)
Why are you leaving holes in TB_FLAGS? I know why the original hole was there,
since it corresponded to simple masks on other registers.
> + vlmax = (1 << vtype->lmul) * cpu->cfg.vlen / (8 * (1 << vtype->sew));
Wow, this can be simplified a lot.
(1 << LMUL) * VLEN / (8 * (1 << SEW))
= (VLEN << LMUL) / (8 << SEW)
= (VLEN << LMUL) >> (SEW + 3)
= VLEN >> (SEW + 3 - LMUL)
> + vl_eq_vlmax = (env->vext.vstart == 0) && (vlmax == env->vext.vl);
> +
> + flags = FIELD_DP32(flags, TB_FLAGS, VILL, vtype->vill);
> + flags = FIELD_DP32(flags, TB_FLAGS, SEW, vtype->sew);
> + flags = FIELD_DP32(flags, TB_FLAGS, LMUL, vtype->lmul);
> + flags = FIELD_DP32(flags, TB_FLAGS, VL_EQ_VLMAX, vl_eq_vlmax);
I wonder if perhaps this all ought to be nested under
if (env->misa & RVV) {
...
} else {
flag = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
}
so that, for the normal case when RVV is disabled, we don't bother computing
all of those bits.
> +static bool trans_vsetvl(DisasContext *ctx, arg_vsetvl * a)
> +{
> + TCGv s1, s2, d;
> + d = tcg_temp_new();
> + s1 = tcg_temp_new();
> + s2 = tcg_temp_new();
> + gen_get_gpr(s1, a->rs1);
> + gen_get_gpr(s2, a->rs2);
> + gen_helper_vector_vsetvli(d, cpu_env, s1, s2);
> + tcg_gen_st_tl(d, cpu_env, offsetof(CPURISCVState, vext.vl));
Why are you performing the store to vl inline, as opposed to within the helper
funtion?
> + exit_tb(ctx);
A normal exit is correct for vsetvl, because the new state is variable.
> +static bool trans_vsetvli(DisasContext *ctx, arg_vsetvli * a)
> +{
> + TCGv s1, s2, d;
> + d = tcg_temp_new();
> + s1 = tcg_temp_new();
> + s2 = tcg_const_tl(a->zimm);
> + gen_get_gpr(s1, a->rs1);
> + gen_helper_vector_vsetvli(d, cpu_env, s1, s2);
> + tcg_gen_st_tl(d, cpu_env, offsetof(CPURISCVState, vext.vl));
> + exit_tb(ctx);
You could use
gen_goto_tb(ctx, 0, ctx->base.pc_next)
here, because the new state is unknown but constant. It will be the same every
time the instruction is executed, and thus can compute the new state only once,
saving that computation in the link to the next tb.
> +target_ulong VECTOR_HELPER(vsetvli)(CPURISCVState *env, target_ulong s1,
> + target_ulong s2)
> +{
> + int vlmax, vl;
> + RISCVCPU *cpu = env_archcpu(env);
> + struct VTYPE *vtype = (struct VTYPE *)&s2;
FIELD_EX64 for all uses of VTYPE.
> +
> + if (vtype->sew > cpu->cfg.elen) { /* only set vill bit. */
> + env->vext.vtype = ~((target_ulong)-1 >> 1);
FIELD_DP64.
> + vlmax = (1 << vtype->lmul) * cpu->cfg.vlen / (8 * (1 << vtype->sew));
Same simplification as before. Perhaps extract this to an inline function for
clarity, documenting the algebraic simplification only once.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 4/4] RISC-V: add vector extension configure instruction
2020-01-03 23:41 ` Richard Henderson
@ 2020-01-07 2:11 ` LIU Zhiwei
0 siblings, 0 replies; 14+ messages in thread
From: LIU Zhiwei @ 2020-01-07 2:11 UTC (permalink / raw)
To: Richard Henderson, alistair23, chihmin.chao, palmer
Cc: wenmeng_zhang, qemu-riscv, qemu-devel, wxy194768
[-- Attachment #1: Type: text/plain, Size: 5934 bytes --]
Hi Richard,
Thanks for the comments of the part 1. It's really very helpful.
I accept most of the comments.
On 2020/1/4 7:41, Richard Henderson wrote:
> On 1/3/20 2:33 PM, LIU Zhiwei wrote:
>> vsetvl and vsetvli are two configure instructions for vl, vtype. TB flags
>> should update after configure instructions. The (ill, lmul, sew ) of vtype
>> and the bit of (VSTART == 0 && VL == VLMAX) will be placed within tb_flags.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>> target/riscv/Makefile.objs | 2 +-
>> target/riscv/cpu.c | 1 +
>> target/riscv/cpu.h | 55 ++++++++++++++++++++-----
>> target/riscv/helper.h | 2 +
>> target/riscv/insn32.decode | 5 +++
>> target/riscv/insn_trans/trans_rvv.inc.c | 52 +++++++++++++++++++++++
>> target/riscv/translate.c | 17 +++++++-
>> target/riscv/vector_helper.c | 51 +++++++++++++++++++++++
>> 8 files changed, 172 insertions(+), 13 deletions(-)
>> create mode 100644 target/riscv/insn_trans/trans_rvv.inc.c
>> create mode 100644 target/riscv/vector_helper.c
>>
>> diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
>> index b1c79bc1d1..d577cef9e0 100644
>> --- a/target/riscv/Makefile.objs
>> +++ b/target/riscv/Makefile.objs
>> @@ -1,4 +1,4 @@
>> -obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o gdbstub.o pmp.o
>> +obj-y += translate.o op_helper.o cpu_helper.o cpu.o csr.o fpu_helper.o vector_helper.o gdbstub.o pmp.o
>>
>> DECODETREE = $(SRC_PATH)/scripts/decodetree.py
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index c2370a0a57..3ff7b50bff 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -347,6 +347,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>> }
>> }
>> if (cpu->cfg.vext_spec) {
>> + env->vext.vtype = ~((target_ulong)-1 >> 1);
> Better as FIELD_DP64(0, VTYPE, VILL, 1),
>
>
>> +struct VTYPE {
>> +#ifdef HOST_WORDS_BIGENDIAN
>> + target_ulong vill:1;
>> + target_ulong reserved:sizeof(target_ulong) * 8 - 7;
>> + target_ulong sew:3;
>> + target_ulong lmul:2;
>> +#else
>> + target_ulong lmul:2;
>> + target_ulong sew:3;
>> + target_ulong reserved:sizeof(target_ulong) * 8 - 7;
>> + target_ulong vill:1;
>> +#endif
>> +};
> Do not use bit fields to describe target register layout.
> Use FIELD().
OK. I think there is no need to the handle endianess here. FIELD() is good.
>> -#define TB_FLAGS_MMU_MASK 3
>> -#define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>> +typedef CPURISCVState CPUArchState;
>> +typedef RISCVCPU ArchCPU;
>> +#include "exec/cpu-all.h"
>> +
>> +FIELD(TB_FLAGS, MMU, 0, 2)
>> +FIELD(TB_FLAGS, FS, 13, 2)
> The change to use FIELD for MMU and FS should be made separately from adding
> the vector state.
>
>> +FIELD(TB_FLAGS, VL_EQ_VLMAX, 16, 1)
>> +FIELD(TB_FLAGS, LMUL, 17, 2)
>> +FIELD(TB_FLAGS, SEW, 19, 3)
>> +FIELD(TB_FLAGS, VILL, 22, 1)
> Why are you leaving holes in TB_FLAGS? I know why the original hole was there,
> since it corresponded to simple masks on other registers.
>
>> + vlmax = (1 << vtype->lmul) * cpu->cfg.vlen / (8 * (1 << vtype->sew));
> Wow, this can be simplified a lot.
>
> (1 << LMUL) * VLEN / (8 * (1 << SEW))
> = (VLEN << LMUL) / (8 << SEW)
> = (VLEN << LMUL) >> (SEW + 3)
> = VLEN >> (SEW + 3 - LMUL)
>
Good.
>> + vl_eq_vlmax = (env->vext.vstart == 0) && (vlmax == env->vext.vl);
>> +
>> + flags = FIELD_DP32(flags, TB_FLAGS, VILL, vtype->vill);
>> + flags = FIELD_DP32(flags, TB_FLAGS, SEW, vtype->sew);
>> + flags = FIELD_DP32(flags, TB_FLAGS, LMUL, vtype->lmul);
>> + flags = FIELD_DP32(flags, TB_FLAGS, VL_EQ_VLMAX, vl_eq_vlmax);
> I wonder if perhaps this all ought to be nested under
>
> if (env->misa & RVV) {
> ...
> } else {
> flag = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
> }
>
> so that, for the normal case when RVV is disabled, we don't bother computing
> all of those bits.
>
>> +static bool trans_vsetvl(DisasContext *ctx, arg_vsetvl * a)
>> +{
>> + TCGv s1, s2, d;
>> + d = tcg_temp_new();
>> + s1 = tcg_temp_new();
>> + s2 = tcg_temp_new();
>> + gen_get_gpr(s1, a->rs1);
>> + gen_get_gpr(s2, a->rs2);
>> + gen_helper_vector_vsetvli(d, cpu_env, s1, s2);
>> + tcg_gen_st_tl(d, cpu_env, offsetof(CPURISCVState, vext.vl));
> Why are you performing the store to vl inline, as opposed to within the helper
> funtion?
>
>> + exit_tb(ctx);
> A normal exit is correct for vsetvl, because the new state is variable.
>
>> +static bool trans_vsetvli(DisasContext *ctx, arg_vsetvli * a)
>> +{
>> + TCGv s1, s2, d;
>> + d = tcg_temp_new();
>> + s1 = tcg_temp_new();
>> + s2 = tcg_const_tl(a->zimm);
>> + gen_get_gpr(s1, a->rs1);
>> + gen_helper_vector_vsetvli(d, cpu_env, s1, s2);
>> + tcg_gen_st_tl(d, cpu_env, offsetof(CPURISCVState, vext.vl));
>> + exit_tb(ctx);
> You could use
>
> gen_goto_tb(ctx, 0, ctx->base.pc_next)
>
> here, because the new state is unknown but constant. It will be the same every
> time the instruction is executed, and thus can compute the new state only once,
> saving that computation in the link to the next tb.
>
>> +target_ulong VECTOR_HELPER(vsetvli)(CPURISCVState *env, target_ulong s1,
>> + target_ulong s2)
>> +{
>> + int vlmax, vl;
>> + RISCVCPU *cpu = env_archcpu(env);
>> + struct VTYPE *vtype = (struct VTYPE *)&s2;
> FIELD_EX64 for all uses of VTYPE.
>
>> +
>> + if (vtype->sew > cpu->cfg.elen) { /* only set vill bit. */
>> + env->vext.vtype = ~((target_ulong)-1 >> 1);
> FIELD_DP64.
>
>> + vlmax = (1 << vtype->lmul) * cpu->cfg.vlen / (8 * (1 << vtype->sew));
> Same simplification as before. Perhaps extract this to an inline function for
> clarity, documenting the algebraic simplification only once.
>
> r~
Best Regards,
Zhiwei
[-- Attachment #2: Type: text/html, Size: 9195 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread