Re: [PATCH 1/1] target/riscv: Clear vstart_qe_zero flag

[Daniel Henrique Barboza]

On 12/14/23 08:18, Ivan Klokov wrote:
> The vstart_qe_zero flag is set at the beginning of the translation
> phase from the env->vstart variable. During the execution phase, some
> instructions may change env->vstart, but the flag remains the same as
> at the start of the block. With some combinations of instructions this
> causes an illegal instruction exception. This patch simultaneously
> updates flag and env->vstart and to avoid inconsistency.
>
> Signed-off-by: Ivan Klokov <ivan.klokov@syntacore.com>
> ---
>   target/riscv/insn_trans/trans_rvbf16.c.inc |  6 +-
>   target/riscv/insn_trans/trans_rvv.c.inc    | 88 +++++++++++-----------
>   target/riscv/insn_trans/trans_rvvk.c.inc   | 12 +--
>   target/riscv/translate.c                   | 12 ++-
>   4 files changed, 64 insertions(+), 54 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvbf16.c.inc 
> b/target/riscv/insn_trans/trans_rvbf16.c.inc
> index 4e39c00884..2867bbc2bb 100644
> --- a/target/riscv/insn_trans/trans_rvbf16.c.inc
> +++ b/target/riscv/insn_trans/trans_rvbf16.c.inc
> @@ -86,7 +86,7 @@ static bool trans_vfncvtbf16_f_f_w(DisasContext *ctx, 
> arg_vfncvtbf16_f_f_w *a)
>                              ctx->cfg_ptr->vlen / 8,
>                              ctx->cfg_ptr->vlen / 8, data,
>                              gen_helper_vfncvtbf16_f_f_w);
> -        mark_vs_dirty(ctx);
> +        finalize_rvv_inst(ctx);

"finalize_rvv_inst()" is not the best of names in this case. Why aren't we
"finalizing" all RVV instructions?

mark_vs_dirty() is already doing a lot of ctx updates. I would just put any 
updates
to ctx->vstart_qe_zero inside of it.


> [...]


> -        mark_vs_dirty(s);
> +        finalize_rrv_inst(s);
>           gen_set_label(over);
>           return true;
>       }
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index f0be79bb16..d4147e2dd7 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -670,8 +670,18 @@ static void mark_vs_dirty(DisasContext *ctx)
>           }
>       }
>   }
> +static void set_vstart_eq_zero(DisasContext *ctx)
> +{
> +    ctx->vstart_eq_zero = true;
> +}
> +
> +static void finalize_rvv_inst(DisasContext *ctx)
> +{
> +    mark_vs_dirty(ctx);
> +    set_vstart_eq_zero(ctx);
> +}

Did you check if every time we're marking VS dirty we're setting env->vstart = 
0?
I was taking a looking into it and so far I'm not to make this assumption.

IMO you'll need to read the current vstart value (probably via cpu_vstart) to 
see if
ctx->vstart_eq_zero is true or not. If it's false you can also mark 
ctx->vl_eq_vmax
as false since it depends on ctx->vstart_eq_zero.


Thanks,


Daniel


>   #else
> -static inline void mark_vs_dirty(DisasContext *ctx) { }
> +static inline void finalize_rvv_inst(DisasContext *ctx){ }
>   #endif
>   
>   static void gen_set_rm(DisasContext *ctx, int rm)