Re: [PATCH v3 3/6] tcg/riscv64: Extract elements for vector arithmetic immediates

[Philippe Mathieu-Daudé]

On 29/10/26 19:18, Aurelien Jarno wrote:
On 2026-10-28 15:33, Philippe Mathieu-Daudé wrote:
The .vi emitters for add, reverse sub and saturating ops pass the
whole replicated constant (e.g. 0x0505050505050505) to encode_vi()
instead of a single element. This aborts debug builds on common
vectorized guest code and is undefined behaviour in release builds.

I'll reword as:

  The .vi emitters pass the whole replicated constant (e.g.
  0x0505050505050505) instead of one element. The encoding is still
  correct, because encode_vi() keeps the low 5 bits, but it trips the
  range assert added in 006352d69f14 on common vectorized guest code
  in debug builds. Extract the VECE element with vec_imm(), so the
  assert keeps checking that the emitter agrees with the constraint
  matcher.


Add vec_imm() to extract the VECE element, and use it.

Fixes: 006352d69f14 ("tcg/riscv64: Assert immediate in range ...")
Signed-off-by: Philippe Mathieu-Daudé <phi...@oss.qualcomm.com>
---
  tcg/riscv64/tcg-target.c.inc | 23 +++++++++++++++++------
  1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tcg/riscv64/tcg-target.c.inc b/tcg/riscv64/tcg-target.c.inc
index 0e749ae296b8..a3d995e290b7 100644
--- a/tcg/riscv64/tcg-target.c.inc
+++ b/tcg/riscv64/tcg-target.c.inc
@@ -735,6 +735,12 @@ static void tcg_out_opc_vi_u(TCGContext *s, RISCVInsn opc,
      tcg_out32(s, encode_vi_u(opc, vd, imm, vs2, true));
  }
  
+/* Constants are replicated by VECE; extract one element for .vi forms. */
+static int64_t vec_imm(TCGArg arg, unsigned vece)
+{
+    return sextract64(arg, 0, 8 << vece);
+}
+
  static void tcg_out_opc_vv_vi(TCGContext *s, RISCVInsn o_vv, RISCVInsn o_vi,
                                TCGReg vd, TCGReg vs2, TCGArg vi1, int c_vi1)
  {
@@ -2788,12 +2794,13 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
          break;
      case INDEX_op_add_vec:
          set_vtype_len_sew(s, type, vece);
-        tcg_out_opc_vv_vi(s, OPC_VADD_VV, OPC_VADD_VI, a0, a1, a2, c2);
+        tcg_out_opc_vv_vi(s, OPC_VADD_VV, OPC_VADD_VI, a0, a1,
+                          c2 ? vec_imm(a2, vece) : a2, c2);
          break;
      case INDEX_op_sub_vec:
          set_vtype_len_sew(s, type, vece);
          if (const_args[1]) {
-            tcg_out_opc_vi(s, OPC_VRSUB_VI, a0, a2, a1);
+            tcg_out_opc_vi(s, OPC_VRSUB_VI, a0, a2, vec_imm(a1, vece));
          } else {
              tcg_out_opc_vv(s, OPC_VSUB_VV, a0, a1, a2);
          }
@@ -2824,19 +2831,23 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
          break;
      case INDEX_op_ssadd_vec:
          set_vtype_len_sew(s, type, vece);
-        tcg_out_opc_vv_vi(s, OPC_VSADD_VV, OPC_VSADD_VI, a0, a1, a2, c2);
+        tcg_out_opc_vv_vi(s, OPC_VSADD_VV, OPC_VSADD_VI, a0, a1,
+                          c2 ? vec_imm(a2, vece) : a2, c2);
          break;
      case INDEX_op_sssub_vec:
          set_vtype_len_sew(s, type, vece);
-        tcg_out_opc_vv_vi(s, OPC_VSSUB_VV, OPC_VSSUB_VI, a0, a1, a2, c2);
+        tcg_out_opc_vv_vi(s, OPC_VSSUB_VV, OPC_VSSUB_VI, a0, a1,
+                          c2 ? vec_imm(a2, vece) : a2, c2);
          break;
      case INDEX_op_usadd_vec:
          set_vtype_len_sew(s, type, vece);
-        tcg_out_opc_vv_vi(s, OPC_VSADDU_VV, OPC_VSADDU_VI, a0, a1, a2, c2);
+        tcg_out_opc_vv_vi(s, OPC_VSADDU_VV, OPC_VSADDU_VI, a0, a1,
+                          c2 ? vec_imm(a2, vece) : a2, c2);
          break;
      case INDEX_op_ussub_vec:
          set_vtype_len_sew(s, type, vece);
-        tcg_out_opc_vv_vi(s, OPC_VSSUBU_VV, OPC_VSSUBU_VI, a0, a1, a2, c2);
+        tcg_out_opc_vv_vi(s, OPC_VSSUBU_VV, OPC_VSSUBU_VI, a0, a1,
+                          c2 ? vec_imm(a2, vece) : a2, c2);
          break;
      case INDEX_op_smax_vec:
          set_vtype_len_sew(s, type, vece);

This is technically correct. That said, the goal of all this extra code
is to satisfy the assert added in commit 006352d69f14 ("tcg/riscv64:
Assert immediate in range for {i,s,u,vi}-type insns") not to fix the
code generation which is correct as it only keeps the lower 5 bits of
the immediate.

At this stage we already know that the immediate can be represented as a
duplicated simm5 constant, so just dropping the assert could be a
simpler way to fix the issue.

IIUC that assert was added on purpose, together with the matching ones 
for imm12, simm12 and uimm20. If we drop the vi assert, the emitter
stops checking that it agrees with the matcher; and vec_imm() costs
only five lines. The reworded description should clarify that.