Re: [PATCH 1/9] target/s390x: Use a single return for helper_divs32/u32

[Ilya Leoshkevich]

On Fri, Oct 21, 2022 at 05:29:58PM +1000, Richard Henderson wrote:
> Pack the quotient and remainder into a single uint64_t.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/helper.h         |  2 +-
>  target/s390x/tcg/int_helper.c | 26 +++++++++++++-------------
>  target/s390x/tcg/translate.c  | 10 ++++++----
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index bf33d86f74..97a9668eef 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -10,7 +10,7 @@ DEF_HELPER_FLAGS_4(clc, TCG_CALL_NO_WG, i32, env, i32, i64, 
> i64)
>  DEF_HELPER_3(mvcl, i32, env, i32, i32)
>  DEF_HELPER_3(clcl, i32, env, i32, i32)
>  DEF_HELPER_FLAGS_4(clm, TCG_CALL_NO_WG, i32, env, i32, i32, i64)
> -DEF_HELPER_FLAGS_3(divs32, TCG_CALL_NO_WG, s64, env, s64, s64)
> +DEF_HELPER_FLAGS_3(divs32, TCG_CALL_NO_WG, i64, env, s64, s64)
>  DEF_HELPER_FLAGS_3(divu32, TCG_CALL_NO_WG, i64, env, i64, i64)
>  DEF_HELPER_FLAGS_3(divs64, TCG_CALL_NO_WG, s64, env, s64, s64)
>  DEF_HELPER_FLAGS_4(divu64, TCG_CALL_NO_WG, i64, env, i64, i64, i64)
> diff --git a/target/s390x/tcg/int_helper.c b/target/s390x/tcg/int_helper.c
> index 954542388a..130b8bd4d2 100644
> --- a/target/s390x/tcg/int_helper.c
> +++ b/target/s390x/tcg/int_helper.c
> @@ -34,45 +34,45 @@
>  #endif
>  
>  /* 64/32 -> 32 signed division */
> -int64_t HELPER(divs32)(CPUS390XState *env, int64_t a, int64_t b64)
> +uint64_t HELPER(divs32)(CPUS390XState *env, int64_t a, int64_t b64)
>  {
> -    int32_t ret, b = b64;
> -    int64_t q;
> +    int32_t b = b64;
> +    int64_t q, r;
>  
>      if (b == 0) {
>          tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
>      }
>  
> -    ret = q = a / b;
> -    env->retxl = a % b;
> +    q = a / b;
> +    r = a % b;
>  
>      /* Catch non-representable quotient.  */
> -    if (ret != q) {
> +    if (q != (int32_t)q) {
>          tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
>      }
>  
> -    return ret;
> +    return deposit64(r, 32, 32, q);
>  }
>  
>  /* 64/32 -> 32 unsigned division */
>  uint64_t HELPER(divu32)(CPUS390XState *env, uint64_t a, uint64_t b64)
>  {
> -    uint32_t ret, b = b64;
> -    uint64_t q;
> +    uint32_t b = b64;
> +    uint64_t q, r;
>  
>      if (b == 0) {
>          tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
>      }
>  
> -    ret = q = a / b;
> -    env->retxl = a % b;
> +    q = a / b;
> +    r = a % b;
>  
>      /* Catch non-representable quotient.  */
> -    if (ret != q) {
> +    if (q != (uint32_t)q) {
>          tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
>      }
>  
> -    return ret;
> +    return deposit64(r, 32, 32, q);
>  }
>  
>  /* 64/64 -> 64 signed division */
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 1d2dddab1c..525317c9df 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2395,15 +2395,17 @@ static DisasJumpType op_diag(DisasContext *s, 
> DisasOps *o)
>  
>  static DisasJumpType op_divs32(DisasContext *s, DisasOps *o)
>  {
> -    gen_helper_divs32(o->out2, cpu_env, o->in1, o->in2);
> -    return_low128(o->out);
> +    gen_helper_divs32(o->out, cpu_env, o->in1, o->in2);
> +    tcg_gen_ext32u_i64(o->out2, o->out);
> +    tcg_gen_shri_i64(o->out, o->out, 32);
>      return DISAS_NEXT;
>  }
>  
>  static DisasJumpType op_divu32(DisasContext *s, DisasOps *o)
>  {
> -    gen_helper_divu32(o->out2, cpu_env, o->in1, o->in2);
> -    return_low128(o->out);
> +    gen_helper_divu32(o->out, cpu_env, o->in1, o->in2);
> +    tcg_gen_ext32u_i64(o->out2, o->out);
> +    tcg_gen_shri_i64(o->out, o->out, 32);
>      return DISAS_NEXT;
>  }
>  
> -- 
> 2.34.1

Hi,

The wasmtime testsuite was failing and bisect pointed to this commit.
Apparently this needs a fixup:


--- a/target/s390x/tcg/int_helper.c
+++ b/target/s390x/tcg/int_helper.c
@@ -51,7 +51,7 @@ uint64_t HELPER(divs32)(CPUS390XState *env, int64_t a, 
int64_t b64)
         tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
     }
 
-    return deposit64(r, 32, 32, q);
+    return deposit64(q, 32, 32, r);
 }
 
 /* 64/32 -> 32 unsigned division */
@@ -72,7 +72,7 @@ uint64_t HELPER(divu32)(CPUS390XState *env, uint64_t a, 
uint64_t b64)
         tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
     }
 
-    return deposit64(r, 32, 32, q);
+    return deposit64(q, 32, 32, r);
 }
 
 /* 64/64 -> 64 signed division */


Currently we return out = r | (q << 32) here.
op_divu32 makes out2 = r, out = q from this.
Finally, r1_P32 stores r1 = q, r1+1 = r.
But DLR wants the opposite:

    The remainder is placed in bit
    positions 32-63 of general register R1, and the quo-
    tient is placed in bit positions 32-63 of general regis-
    ter R1 + 1.

Ditto DR.

Best regards,
Ilya