Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
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~
Re: [PATCH v8 30/62] target/riscv: Update fp_status when float rounding mode changes
On Thu, May 21, 2020 at 3:45 AM LIU Zhiwei 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
Reviewed-by: Alistair Francis
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
>
>
