Re: [PULL 13/43] target/arm: Fix SVE2 WHILEWR/WHILERW zero diff boundary case

On 8/21/26 14:00, Peter Maydell wrote:
> From: Richard Henderson <[email protected]>
> 
> The trans_WHILE_ptr function incorrectly handles the case where the
> address difference divided by ESIZE results in zero. This happens when
> the address difference is less than ESIZE but greater than zero.
> 
> Fix by dropping direct comparisons of op0 vs op1, and instead
> testing the scaled diff vs 0.  Merge with the bounding to the
> maximum vector length via wrapping arithmetic.
> 
> Cc: [email protected]
> Fixes: 14f6dad168e ("target/arm: Implement SVE2 WHILERW, WHILEWR")
> Reported-by: YanjunYang <[email protected]>
> Signed-off-by: Richard Henderson <[email protected]>
> Reviewed-by: Peter Maydell <[email protected]>
> Message-id: [email protected]
> Signed-off-by: Peter Maydell <[email protected]>
> ---
>   target/arm/tcg/translate-sve.c | 38 +++++++++++++++++++++-------------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c
> index fc4cc8c479f..0becef16a94 100644
> --- a/target/arm/tcg/translate-sve.c
> +++ b/target/arm/tcg/translate-sve.c
> @@ -3697,7 +3697,7 @@ TRANS_FEAT(WHILE_gt_cnt4, aa64_sme2_or_sve2p1, do_WHILE,
>   
>   static bool trans_WHILE_ptr(DisasContext *s, arg_WHILE_ptr *a)
>   {
> -    TCGv_i64 op0, op1, diff, t1, tmax;
> +    TCGv_i64 op0, op1, diff, t1;
>       TCGv_i32 t2;
>       TCGv_ptr ptr;
>       unsigned vsz = vec_full_reg_size(s);
> @@ -3713,7 +3713,6 @@ static bool trans_WHILE_ptr(DisasContext *s, arg_WHILE_ptr *a)
>       op0 = read_cpu_reg(s, a->rn, 1);
>       op1 = read_cpu_reg(s, a->rm, 1);
>   
> -    tmax = tcg_constant_i64(vsz >> a->esz);
>       diff = tcg_temp_new_i64();
>   
>       if (a->rw) {
> @@ -3723,25 +3722,36 @@ static bool trans_WHILE_ptr(DisasContext *s, arg_WHILE_ptr *a)
>           tcg_gen_sub_i64(diff, op0, op1);
>           tcg_gen_sub_i64(t1, op1, op0);
>           tcg_gen_movcond_i64(TCG_COND_GEU, diff, op0, op1, diff, t1);
> -        /* Divide, rounding down, by ESIZE.  */
> -        tcg_gen_shri_i64(diff, diff, a->esz);
> -        /* If op1 == op0, diff == 0, and the condition is always true. */
> -        tcg_gen_movcond_i64(TCG_COND_EQ, diff, op0, op1, tmax, diff);
>       } else {
>           /* WHILEWR */
> -        tcg_gen_sub_i64(diff, op1, op0);
> -        /* Divide, rounding down, by ESIZE.  */
> -        tcg_gen_shri_i64(diff, diff, a->esz);
> -        /* If op0 >= op1, diff <= 0, the condition is always true. */
> -        tcg_gen_movcond_i64(TCG_COND_GEU, diff, op0, op1, tmax, diff);
> +        /* Saturating subtraction maps diff <= 0 to diff == 0. */
> +        tcg_gen_ussub_i64(diff, op1, op0);
>       }
>   
> -    /* Bound to the maximum.  */
> -    tcg_gen_umin_i64(diff, diff, tmax);
> +    /* Divide, rounding down, by ESIZE.  */
> +    tcg_gen_shri_i64(diff, diff, a->esz);

While picking this one up for 10.0.x stable series, I come across an
interesting case.  This very code were changed in v10.0.0-1961-gf96fd13c6ea0
("target/arm: Move scale by esz into helper_sve_while*"), and is now being
dropped entirely.

The back-port of this patch to 10.0.x is below, and is also available at
https://gitlab.com/mjt0k/qemu/-/commit/f25f16d5c5d364306ee88438704a9054855c12b0
(staging-10.0 branch).  It should be okay, but I appreciate an additional
look there, just in case :)

(Yes, I picked up the testcase too, and it passes).

Thanks,

/mjt

commit f25f16d5c5d364306ee88438704a9054855c12b0
Author: Richard Henderson <[email protected]>
Date:   Tue Aug 11 12:15:39 2026 -0700

     target/arm: Fix SVE2 WHILEWR/WHILERW zero diff boundary case

     The trans_WHILE_ptr function incorrectly handles the case where the
     address difference divided by ESIZE results in zero. This happens when
     the address difference is less than ESIZE but greater than zero.

     Fix by dropping direct comparisons of op0 vs op1, and instead
     testing the scaled diff vs 0.  Merge with the bounding to the
     maximum vector length via wrapping arithmetic.

     Cc: [email protected]
     Fixes: 14f6dad168e ("target/arm: Implement SVE2 WHILERW, WHILEWR")
     Reported-by: YanjunYang <[email protected]>
     Signed-off-by: Richard Henderson <[email protected]>
     Reviewed-by: Peter Maydell <[email protected]>
     Message-id: [email protected]
     Signed-off-by: Peter Maydell <[email protected]>
     (cherry picked from commit bab972a2b0e34b889fbe8deb1e41cbea47e797f2)
     (Mjt: back-port to 10.0.x across missing v10.0.0-1961-gf96fd13c6ea0
      "target/arm: Move scale by esz into helper_sve_while*")
     Signed-off-by: Michael Tokarev <[email protected]>

diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c
index 1d5b06e3917..e7d1f8d5b12 100644
--- a/target/arm/tcg/translate-sve.c
+++ b/target/arm/tcg/translate-sve.c
@@ -3218,7 +3218,7 @@ static bool trans_WHILE(DisasContext *s, arg_WHILE *a)

  static bool trans_WHILE_ptr(DisasContext *s, arg_WHILE_ptr *a)
  {
-    TCGv_i64 op0, op1, diff, t1, tmax;
+    TCGv_i64 op0, op1, diff, t1;
      TCGv_i32 t2;
      TCGv_ptr ptr;
      unsigned vsz = vec_full_reg_size(s);
@@ -3234,7 +3234,6 @@ static bool trans_WHILE_ptr(DisasContext *s, arg_WHILE_ptr *a)
      op0 = read_cpu_reg(s, a->rn, 1);
      op1 = read_cpu_reg(s, a->rm, 1);

-    tmax = tcg_constant_i64(vsz);
      diff = tcg_temp_new_i64();

      if (a->rw) {
@@ -3244,25 +3243,36 @@ static bool trans_WHILE_ptr(DisasContext *s, arg_WHILE_ptr *a)
          tcg_gen_sub_i64(diff, op0, op1);
          tcg_gen_sub_i64(t1, op1, op0);
          tcg_gen_movcond_i64(TCG_COND_GEU, diff, op0, op1, diff, t1);
-        /* Round down to a multiple of ESIZE.  */
-        tcg_gen_andi_i64(diff, diff, -1 << a->esz);
-        /* If op1 == op0, diff == 0, and the condition is always true. */
-        tcg_gen_movcond_i64(TCG_COND_EQ, diff, op0, op1, tmax, diff);
      } else {
          /* WHILEWR */
-        tcg_gen_sub_i64(diff, op1, op0);
-        /* Round down to a multiple of ESIZE.  */
-        tcg_gen_andi_i64(diff, diff, -1 << a->esz);
-        /* If op0 >= op1, diff <= 0, the condition is always true. */
-        tcg_gen_movcond_i64(TCG_COND_GEU, diff, op0, op1, tmax, diff);
+        /* Saturating subtraction maps diff <= 0 to diff == 0. */
+        tcg_gen_ussub_i64(diff, op1, op0);
      }

-    /* Bound to the maximum.  */
-    tcg_gen_umin_i64(diff, diff, tmax);
+    /* Divide, rounding down, by ESIZE.  */
+    tcg_gen_shri_i64(diff, diff, a->esz);

-    /* Since we're bounded, pass as a 32-bit type.  */
+    /*
+     * If diff == 0, the condition is always true.  Also, bound to max.
+     * Simplify
+     *    diff = diff ? diff : max;
+     *    diff = umin(diff, max);
+     * via
+     *    diff -= 1;
+     *    diff = umin(diff, max - 1);
+     *    diff += 1;
+     * via 0 - 1 == UINT64_MAX.
+     */
+    tcg_gen_addi_i64(diff, diff, -1);
+    tcg_gen_umin_i64(diff, diff, tcg_constant_i64((vsz >> a->esz) - 1));
+
+    /*
+     * Since we're bounded, pass as a 32-bit type.
+     * Sink the diff += 1 from above into the 32-bit type.
+     */
      t2 = tcg_temp_new_i32();
      tcg_gen_extrl_i64_i32(t2, diff);
+    tcg_gen_addi_i32(t2, t2, 1);

      desc = FIELD_DP32(desc, PREDDESC, OPRSZ, vsz / 8);
      desc = FIELD_DP32(desc, PREDDESC, ESZ, a->esz);