Re: [PATCH 09/13] target/riscv: Adjust vector address with ol

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

On 2021/11/1 下午7:35, Richard Henderson wrote:

> On 11/1/21 6:01 AM, LIU Zhiwei wrote:
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>>   target/riscv/insn_trans/trans_rvv.c.inc |  8 ++++
>>   target/riscv/internals.h                |  1 +
>>   target/riscv/vector_helper.c            | 54 +++++++++++++++++--------
>>   3 files changed, 46 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc 
>> b/target/riscv/insn_trans/trans_rvv.c.inc
>> index ed042f7bb9..5cd9b802df 100644
>> --- a/target/riscv/insn_trans/trans_rvv.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
>> @@ -233,6 +233,7 @@ static bool ld_us_op(DisasContext *s, arg_r2nfvm 
>> *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return ldst_us_trans(a->rd, a->rs1, data, fn, s);
>>   }
>>   @@ -286,6 +287,7 @@ static bool st_us_op(DisasContext *s, 
>> arg_r2nfvm *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return ldst_us_trans(a->rd, a->rs1, data, fn, s);
>>   }
>>   @@ -365,6 +367,7 @@ static bool ld_stride_op(DisasContext *s, 
>> arg_rnfvm *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return ldst_stride_trans(a->rd, a->rs1, a->rs2, data, fn, s);
>>   }
>>   @@ -404,6 +407,7 @@ static bool st_stride_op(DisasContext *s, 
>> arg_rnfvm *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       fn =  fns[seq][s->sew];
>>       if (fn == NULL) {
>>           return false;
>> @@ -490,6 +494,7 @@ static bool ld_index_op(DisasContext *s, 
>> arg_rnfvm *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return ldst_index_trans(a->rd, a->rs1, a->rs2, data, fn, s);
>>   }
>>   @@ -542,6 +547,7 @@ static bool st_index_op(DisasContext *s, 
>> arg_rnfvm *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return ldst_index_trans(a->rd, a->rs1, a->rs2, data, fn, s);
>>   }
>>   @@ -617,6 +623,7 @@ static bool ldff_op(DisasContext *s, arg_r2nfvm 
>> *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, NF, a->nf);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return ldff_trans(a->rd, a->rs1, data, fn, s);
>>   }
>>   @@ -724,6 +731,7 @@ static bool amo_op(DisasContext *s, arg_rwdvm 
>> *a, uint8_t seq)
>>       data = FIELD_DP32(data, VDATA, VM, a->vm);
>>       data = FIELD_DP32(data, VDATA, LMUL, s->lmul);
>>       data = FIELD_DP32(data, VDATA, WD, a->wd);
>> +    data = FIELD_DP32(data, VDATA, OL, s->ol);
>>       return amo_trans(a->rd, a->rs1, a->rs2, data, fn, s);
>>   }
>>   /*
>> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
>> index b15ad394bb..f74b8291e4 100644
>> --- a/target/riscv/internals.h
>> +++ b/target/riscv/internals.h
>> @@ -27,6 +27,7 @@ FIELD(VDATA, VM, 8, 1)
>>   FIELD(VDATA, LMUL, 9, 2)
>>   FIELD(VDATA, NF, 11, 4)
>>   FIELD(VDATA, WD, 11, 1)
>> +FIELD(VDATA, OL, 15, 2)
>>     /* float point classify helpers */
>>   target_ulong fclass_h(uint64_t frs1);
>> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
>> index 535420ee66..451688c328 100644
>> --- a/target/riscv/vector_helper.c
>> +++ b/target/riscv/vector_helper.c
>> @@ -112,6 +112,11 @@ static uint32_t vext_wd(uint32_t desc)
>>       return (simd_data(desc) >> 11) & 0x1;
>>   }
>>   +static inline uint32_t vext_ol(uint32_t desc)
>> +{
>> +    return FIELD_EX32(simd_data(desc), VDATA, OL);
>> +}
>
> XLEN not OLEN.
OK.
>
>> @@ -123,6 +128,14 @@ static inline uint32_t vext_maxsz(uint32_t desc)
>>       return simd_maxsz(desc) << vext_lmul(desc);
>>   }
>>   +static inline target_ulong adjust_addr(target_ulong addr, uint32_t 
>> olen)
>> +{
>> +    if (olen < TARGET_LONG_BITS) {
>> +        addr &= UINT32_MAX;
>> +    }
>> +    return addr;
>> +}
>
> Here's where I'm unsure.  This looks a lot like the changes that are 
> required to support pointer-masking in vectors, which Alexey said he 
> was going to look at.
>
> (1) Do we need to pass anything in VEXT at all?
>     We do have CPURISCVState, so we could just use cpu_get_ml,
Yes, we should use cpu_get_xl.
> which we would also need for env->mmte etc for pointer masking.

Do you mean env->mpmmask and env->mpmbase? I think yes, we should also 
adjust these register behaviors with xlen.

>
> (2) Do we try to streamline the "normal" case with a simple bit in VEXT
>     that indicates if the address needs modification at all?  I.e. the
>     bit is set if UXLEN < TARGET_LONG_BITS or if PM_ENABLED?
>
> (3) Do we try to streamline the computation by passing down composite
>     mask and base parameters.  This way we don't need to do complex
>     examination of ENV to determine execution mode, and instead always
>     compute
>
>        addr = (addr & mask) | base;
>
>     where mask = -1, base = 0 for "normal" addressing, and when
>     UXLEN == 32, mask <= UINT32_MAX.

Do you mean add env->pmmask and env->pmbase?

I can initialize themin riscv_tr_init_disas_context, such as by 
env->xpmmask & UINT32_MAX .

>
> (4) Do we in fact want to pre-compute these into known slots on ENV,
>     so that we don't have to pass these around as separate parameters?
>     We would adjust these values during PM CSR changes and when
>     changing privilege levels.
>
>
> r~