| Message ID | 20200521094413.10425-31-zhiwei_liu@c-sky.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: support vector extension v0.7.1 | expand |
On Thu, May 21, 2020 at 3:45 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > For scalar float instruction, round mode is encoded in instruction, > so fp_status is updating dynamiclly. > > For vector float instruction, round mode is always frm, so > update fp_status when frm changes is enough. > > Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/csr.c | 7 +++++++ > target/riscv/fpu_helper.c | 19 ++++++++++++++----- > target/riscv/internals.h | 3 +++ > 3 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d71c49dfff..438093152b 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -22,6 +22,7 @@ > #include "cpu.h" > #include "qemu/main-loop.h" > #include "exec/exec-all.h" > +#include "internals.h" > > /* CSR function table */ > static riscv_csr_operations csr_ops[]; > @@ -174,6 +175,9 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val) > env->mstatus |= MSTATUS_FS; > #endif > env->frm = val & (FSR_RD >> FSR_RD_SHIFT); > + if (!riscv_cpu_set_rounding_mode(env, env->frm)) { > + return -1; > + } > return 0; > } > > @@ -207,6 +211,9 @@ static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val) > env->vxsat = (val & FSR_VXSAT) >> FSR_VXSAT_SHIFT; > } > riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT); > + if (!riscv_cpu_set_rounding_mode(env, env->frm)) { > + return -1; > + } > return 0; > } > > diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c > index 0b79562a69..262610e837 100644 > --- a/target/riscv/fpu_helper.c > +++ b/target/riscv/fpu_helper.c > @@ -50,13 +50,10 @@ void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong hard) > set_float_exception_flags(soft, &env->fp_status); > } > > -void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) > +bool riscv_cpu_set_rounding_mode(CPURISCVState *env, uint32_t rm) > { > int softrm; > > - if (rm == 7) { > - rm = env->frm; > - } > switch (rm) { > case 0: > softrm = float_round_nearest_even; > @@ -74,10 +71,22 @@ void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) > softrm = float_round_ties_away; > break; > default: > - riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); > + return false; > } > > set_float_rounding_mode(softrm, &env->fp_status); > + return true; > +} > + > +void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) > +{ > + if (rm == 7) { > + rm = env->frm; > + } > + > + if (!riscv_cpu_set_rounding_mode(env, rm)) { > + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); > + } > } > > uint64_t helper_fmadd_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2, > diff --git a/target/riscv/internals.h b/target/riscv/internals.h > index f699d80c41..52f6af2513 100644 > --- a/target/riscv/internals.h > +++ b/target/riscv/internals.h > @@ -27,4 +27,7 @@ FIELD(VDATA, VM, 8, 1) > FIELD(VDATA, LMUL, 9, 2) > FIELD(VDATA, NF, 11, 4) > FIELD(VDATA, WD, 11, 1) > + > +/* set float rounding mode */ > +bool riscv_cpu_set_rounding_mode(CPURISCVState *env, uint32_t rm); > #endif > -- > 2.23.0 > >
On 5/21/20 2:43 AM, LIU Zhiwei wrote: > @@ -174,6 +175,9 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val) > env->mstatus |= MSTATUS_FS; > #endif > env->frm = val & (FSR_RD >> FSR_RD_SHIFT); > + if (!riscv_cpu_set_rounding_mode(env, env->frm)) { > + return -1; > + } This will raise an exception immediately in helper_csrrw(). According to Section 8.2, the no exception should occur until the next fp operation that uses the invalid frm. You're doing this just fine in helper_set_rounding_mode(), which is sufficient for scalar fp ops. Without looking forward to later patches, I suppose we'll need to do something else for vector fp ops. r~
On 2020/6/3 12:27, Richard Henderson wrote: > On 5/21/20 2:43 AM, LIU Zhiwei wrote: >> @@ -174,6 +175,9 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val) >> env->mstatus |= MSTATUS_FS; >> #endif >> env->frm = val & (FSR_RD >> FSR_RD_SHIFT); >> + if (!riscv_cpu_set_rounding_mode(env, env->frm)) { >> + return -1; >> + } > This will raise an exception immediately in helper_csrrw(). > > According to Section 8.2, the no exception should occur until the next fp > operation that uses the invalid frm. > > You're doing this just fine in helper_set_rounding_mode(), which is sufficient > for scalar fp ops. Without looking forward to later patches, I suppose we'll > need to do something else for vector fp ops. Hi Richard, I think you are right. Maybe I should transmit frm to ctx->frm, and check ctx->frm in vector fp ops. We can set ctx->frm = env->frm instead of ctx->frm = -1 in riscv_tr_init_disas_context. And remove the sentence ctx->frm = rm; from gen_set_rm. Is it right? Zhiwei > > r~
On 6/2/20 10:46 PM, LIU Zhiwei wrote: > I think you are right. Maybe I should transmit frm to ctx->frm, and check > ctx->frm in vector fp ops. > > We can set ctx->frm = env->frm instead of ctx->frm = -1 in > riscv_tr_init_disas_context. > And remove the sentence ctx->frm = rm; from gen_set_rm. > > Is it right? If we record frm in tb_flags, then we also have to reset env->fp_status.rounding_mode for scalar fp insns which encode a rm != 7. Depending on the exact mix of fp insns, that could result in more changes to rounding_mode than we do presently. This is something that you'd want to look at closely to make sure you're not making scalar code worse. I think the easiest solution in the short term is to have the translation of any fp vector insn call gen_set_rm(ctx, 7), so that we are certain to install frm as rounding_mode. This will happen at most once per translation block, assuming no scalar insns that would also require changes to rounding_mode. You can work with the risc-v folk to examine frm handling more generally separate from this vector work. r~
On 2020/6/5 4:15, Richard Henderson wrote: > On 6/2/20 10:46 PM, LIU Zhiwei wrote: >> I think you are right. Maybe I should transmit frm to ctx->frm, and check >> ctx->frm in vector fp ops. >> >> We can set ctx->frm = env->frm instead of ctx->frm = -1 in >> riscv_tr_init_disas_context. >> And remove the sentence ctx->frm = rm; from gen_set_rm. >> >> Is it right? > If we record frm in tb_flags, then we also have to reset > env->fp_status.rounding_mode for scalar fp insns which encode a rm != 7. > Depending on the exact mix of fp insns, that could result in more changes to > rounding_mode than we do presently. This is something that you'd want to look > at closely to make sure you're not making scalar code worse. Hi Richard, I don't think so. It will occupy three bits in tb_flags. Another reason is the scalar float codes have fixed rounding mode. The frm field in tb_flags is useless without vector extension. If we really have to put it in the tb_flags, one bit INVALID_FRM is enough. The scalar code will still be the same. The vector code will check the INVALID_FRM. > I think the easiest solution in the short term is to have the translation of > any fp vector insn call gen_set_rm(ctx, 7), so that we are certain to install > frm as rounding_mode. This will happen at most once per translation block, > assuming no scalar insns that would also require changes to rounding_mode. Yes. Only some csr insns can change the rounding mode. And they will exit the tb immediately. So no scalar insns will require changes within a translation block. I think there is a error in gen_set_rm static void gen_set_rm(DisasContext *ctx, int rm) { TCGv_i32 t0; if (ctx->frm == rm) { return; } ctx->frm = rm; t0 = tcg_const_i32(rm); gen_helper_set_rounding_mode(cpu_env, t0); tcg_temp_free_i32(t0); } I don't know why updating ctx->frm in this function. > > You can work with the risc-v folk to examine frm handling more generally > separate from this vector work. OK. I will send a separate RFC patch to handle frm. Then merge it to the vector patch set. Best Regards, Zhiwei > > r~
On 6/4/20 7:50 PM, LIU Zhiwei wrote: > So no scalar insns will require changes within a translation block. Not true -- scalar insns can encode rm into the instruction. > I think there is a error in gen_set_rm > > static void gen_set_rm(DisasContext *ctx, int rm) > { > TCGv_i32 t0; > > if (ctx->frm == rm) { > return; > } > ctx->frm = rm; > t0 = tcg_const_i32(rm); > gen_helper_set_rounding_mode(cpu_env, t0); > tcg_temp_free_i32(t0); > } > > I don't know why updating ctx->frm in this function. This is a cache of the current rm, as most recently stored in env->fp_status.rounding_mode. So if we have fadd.s ft0, ft0, ft0, rtz fadd.s ft0, ft0, ft0, rtz fadd.s ft0, ft0, ft0, rtz we will only switch to round_to_zero once. r~
On 2020/6/5 11:30, Richard Henderson wrote: > On 6/4/20 7:50 PM, LIU Zhiwei wrote: >> So no scalar insns will require changes within a translation block. > Not true -- scalar insns can encode rm into the instruction. > >> I think there is a error in gen_set_rm >> >> static void gen_set_rm(DisasContext *ctx, int rm) >> { >> TCGv_i32 t0; >> >> if (ctx->frm == rm) { >> return; >> } >> ctx->frm = rm; >> t0 = tcg_const_i32(rm); >> gen_helper_set_rounding_mode(cpu_env, t0); >> tcg_temp_free_i32(t0); >> } >> >> I don't know why updating ctx->frm in this function. > This is a cache of the current rm, as most recently stored in > env->fp_status.rounding_mode. > > So if we have > > fadd.s ft0, ft0, ft0, rtz > fadd.s ft0, ft0, ft0, rtz > fadd.s ft0, ft0, ft0, rtz > > we will only switch to round_to_zero once. Get it, thanks. Maybe I should only gen_set_rm(ctx, 7) for each vector float insn. And the csr write method for frm or fcsr will not change. So I will remove this patch in the next patch set. Zhiwei > > r~
On 6/7/20 7:39 PM, LIU Zhiwei wrote: > Maybe I should only gen_set_rm(ctx, 7) for each vector float insn. > And the csr write method for frm or fcsr will not change. > > So I will remove this patch in the next patch set. Sounds perfect, thanks. r~
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index d71c49dfff..438093152b 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -22,6 +22,7 @@ #include "cpu.h" #include "qemu/main-loop.h" #include "exec/exec-all.h" +#include "internals.h" /* CSR function table */ static riscv_csr_operations csr_ops[]; @@ -174,6 +175,9 @@ static int write_frm(CPURISCVState *env, int csrno, target_ulong val) env->mstatus |= MSTATUS_FS; #endif env->frm = val & (FSR_RD >> FSR_RD_SHIFT); + if (!riscv_cpu_set_rounding_mode(env, env->frm)) { + return -1; + } return 0; } @@ -207,6 +211,9 @@ static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val) env->vxsat = (val & FSR_VXSAT) >> FSR_VXSAT_SHIFT; } riscv_cpu_set_fflags(env, (val & FSR_AEXC) >> FSR_AEXC_SHIFT); + if (!riscv_cpu_set_rounding_mode(env, env->frm)) { + return -1; + } return 0; } diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c index 0b79562a69..262610e837 100644 --- a/target/riscv/fpu_helper.c +++ b/target/riscv/fpu_helper.c @@ -50,13 +50,10 @@ void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong hard) set_float_exception_flags(soft, &env->fp_status); } -void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) +bool riscv_cpu_set_rounding_mode(CPURISCVState *env, uint32_t rm) { int softrm; - if (rm == 7) { - rm = env->frm; - } switch (rm) { case 0: softrm = float_round_nearest_even; @@ -74,10 +71,22 @@ void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) softrm = float_round_ties_away; break; default: - riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); + return false; } set_float_rounding_mode(softrm, &env->fp_status); + return true; +} + +void helper_set_rounding_mode(CPURISCVState *env, uint32_t rm) +{ + if (rm == 7) { + rm = env->frm; + } + + if (!riscv_cpu_set_rounding_mode(env, rm)) { + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC()); + } } uint64_t helper_fmadd_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2, diff --git a/target/riscv/internals.h b/target/riscv/internals.h index f699d80c41..52f6af2513 100644 --- a/target/riscv/internals.h +++ b/target/riscv/internals.h @@ -27,4 +27,7 @@ FIELD(VDATA, VM, 8, 1) FIELD(VDATA, LMUL, 9, 2) FIELD(VDATA, NF, 11, 4) FIELD(VDATA, WD, 11, 1) + +/* set float rounding mode */ +bool riscv_cpu_set_rounding_mode(CPURISCVState *env, uint32_t rm); #endif
For scalar float instruction, round mode is encoded in instruction, so fp_status is updating dynamiclly. For vector float instruction, round mode is always frm, so update fp_status when frm changes is enough. Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com> --- target/riscv/csr.c | 7 +++++++ target/riscv/fpu_helper.c | 19 ++++++++++++++----- target/riscv/internals.h | 3 +++ 3 files changed, 24 insertions(+), 5 deletions(-)