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.klo...@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)