Re: [PATCH v3 4/4] RISC-V: add vector extension configure instruction

[LIU Zhiwei <zhiwei_liu@c-sky.com>]

[-- 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 --]