[PATCH 0/2] target/hppa: Use tcg_constant_*

[PATCH 0/2] target/hppa: Use tcg_constant_*

[Richard Henderson]

Replace use of tcg_const_*, which makes a copy into a temp
which must be freed, with direct use of the constant.

r~

Richard Henderson (2):
  target/hppa: Use tcg_constant_*
  target/hppa: Clean up DisasCond

 target/hppa/translate.c | 99 +++++++++++------------------------------
 1 file changed, 27 insertions(+), 72 deletions(-)

-- 
2.25.1

[PATCH 1/2] target/hppa: Use tcg_constant_*

[Richard Henderson]

Replace uses of tcg_const_* with the allocate and free
close together.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/hppa/translate.c | 56 +++++++++++++----------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 64af1e0d5c..d17568880f 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -145,6 +145,7 @@
 #define tcg_gen_sextract_reg tcg_gen_sextract_i64
 #define tcg_const_reg        tcg_const_i64
 #define tcg_const_local_reg  tcg_const_local_i64
+#define tcg_constant_reg     tcg_constant_i64
 #define tcg_gen_movcond_reg  tcg_gen_movcond_i64
 #define tcg_gen_add2_reg     tcg_gen_add2_i64
 #define tcg_gen_sub2_reg     tcg_gen_sub2_i64
@@ -239,6 +240,7 @@
 #define tcg_gen_sextract_reg tcg_gen_sextract_i32
 #define tcg_const_reg        tcg_const_i32
 #define tcg_const_local_reg  tcg_const_local_i32
+#define tcg_constant_reg     tcg_constant_i32
 #define tcg_gen_movcond_reg  tcg_gen_movcond_i32
 #define tcg_gen_add2_reg     tcg_gen_add2_i32
 #define tcg_gen_sub2_reg     tcg_gen_sub2_i32
@@ -772,9 +774,7 @@ static inline target_ureg iaoq_dest(DisasContext *ctx, target_sreg disp)
 
 static void gen_excp_1(int exception)
 {
-    TCGv_i32 t = tcg_const_i32(exception);
-    gen_helper_excp(cpu_env, t);
-    tcg_temp_free_i32(t);
+    gen_helper_excp(cpu_env, tcg_constant_i32(exception));
 }
 
 static void gen_excp(DisasContext *ctx, int exception)
@@ -788,12 +788,9 @@ static void gen_excp(DisasContext *ctx, int exception)
 
 static bool gen_excp_iir(DisasContext *ctx, int exc)
 {
-    TCGv_reg tmp;
-
     nullify_over(ctx);
-    tmp = tcg_const_reg(ctx->insn);
-    tcg_gen_st_reg(tmp, cpu_env, offsetof(CPUHPPAState, cr[CR_IIR]));
-    tcg_temp_free(tmp);
+    tcg_gen_st_reg(tcg_constant_reg(ctx->insn),
+                   cpu_env, offsetof(CPUHPPAState, cr[CR_IIR]));
     gen_excp(ctx, exc);
     return nullify_end(ctx);
 }
@@ -1154,13 +1151,12 @@ static void do_add(DisasContext *ctx, unsigned rt, TCGv_reg in1,
     }
 
     if (!is_l || cond_need_cb(c)) {
-        TCGv_reg zero = tcg_const_reg(0);
+        TCGv_reg zero = tcg_constant_reg(0);
         cb_msb = get_temp(ctx);
         tcg_gen_add2_reg(dest, cb_msb, in1, zero, in2, zero);
         if (is_c) {
             tcg_gen_add2_reg(dest, cb_msb, dest, cb_msb, cpu_psw_cb_msb, zero);
         }
-        tcg_temp_free(zero);
         if (!is_l) {
             cb = get_temp(ctx);
             tcg_gen_xor_reg(cb, in1, in2);
@@ -1246,7 +1242,7 @@ static void do_sub(DisasContext *ctx, unsigned rt, TCGv_reg in1,
     cb = tcg_temp_new();
     cb_msb = tcg_temp_new();
 
-    zero = tcg_const_reg(0);
+    zero = tcg_constant_reg(0);
     if (is_b) {
         /* DEST,C = IN1 + ~IN2 + C.  */
         tcg_gen_not_reg(cb, in2);
@@ -1262,7 +1258,6 @@ static void do_sub(DisasContext *ctx, unsigned rt, TCGv_reg in1,
         tcg_gen_eqv_reg(cb, in1, in2);
         tcg_gen_xor_reg(cb, cb, dest);
     }
-    tcg_temp_free(zero);
 
     /* Compute signed overflow if required.  */
     sv = NULL;
@@ -2453,17 +2448,16 @@ static bool trans_probe(DisasContext *ctx, arg_probe *a)
     form_gva(ctx, &addr, &ofs, a->b, 0, 0, 0, a->sp, 0, false);
 
     if (a->imm) {
-        level = tcg_const_i32(a->ri);
+        level = tcg_constant_i32(a->ri);
     } else {
         level = tcg_temp_new_i32();
         tcg_gen_trunc_reg_i32(level, load_gpr(ctx, a->ri));
         tcg_gen_andi_i32(level, level, 3);
     }
-    want = tcg_const_i32(a->write ? PAGE_WRITE : PAGE_READ);
+    want = tcg_constant_i32(a->write ? PAGE_WRITE : PAGE_READ);
 
     gen_helper_probe(dest, cpu_env, addr, level, want);
 
-    tcg_temp_free_i32(want);
     tcg_temp_free_i32(level);
 
     save_gpr(ctx, a->t, dest);
@@ -2603,17 +2597,13 @@ static bool trans_lpa(DisasContext *ctx, arg_ldst *a)
 
 static bool trans_lci(DisasContext *ctx, arg_lci *a)
 {
-    TCGv_reg ci;
-
     CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
 
     /* The Coherence Index is an implementation-defined function of the
        physical address.  Two addresses with the same CI have a coherent
        view of the cache.  Our implementation is to return 0 for all,
        since the entire address space is coherent.  */
-    ci = tcg_const_reg(0);
-    save_gpr(ctx, a->t, ci);
-    tcg_temp_free(ci);
+    save_gpr(ctx, a->t, tcg_constant_reg(0));
 
     cond_free(&ctx->null_cond);
     return true;
@@ -2714,8 +2704,6 @@ static bool trans_or(DisasContext *ctx, arg_rrr_cf *a)
          *                      currently implemented as idle.
          */
         if ((rt == 10 || rt == 31) && r1 == rt && r2 == rt) { /* PAUSE */
-            TCGv_i32 tmp;
-
             /* No need to check for supervisor, as userland can only pause
                until the next timer interrupt.  */
             nullify_over(ctx);
@@ -2726,10 +2714,8 @@ static bool trans_or(DisasContext *ctx, arg_rrr_cf *a)
             nullify_set(ctx, 0);
 
             /* Tell the qemu main loop to halt until this cpu has work.  */
-            tmp = tcg_const_i32(1);
-            tcg_gen_st_i32(tmp, cpu_env, -offsetof(HPPACPU, env) +
-                                         offsetof(CPUState, halted));
-            tcg_temp_free_i32(tmp);
+            tcg_gen_st_i32(tcg_constant_i32(1), cpu_env,
+                           offsetof(CPUState, halted) - offsetof(HPPACPU, env));
             gen_excp_1(EXCP_HALTED);
             ctx->base.is_jmp = DISAS_NORETURN;
 
@@ -2837,7 +2823,7 @@ static bool trans_ds(DisasContext *ctx, arg_rrr_cf *a)
     add2 = tcg_temp_new();
     addc = tcg_temp_new();
     dest = tcg_temp_new();
-    zero = tcg_const_reg(0);
+    zero = tcg_constant_reg(0);
 
     /* Form R1 << 1 | PSW[CB]{8}.  */
     tcg_gen_add_reg(add1, in1, in1);
@@ -2855,7 +2841,6 @@ static bool trans_ds(DisasContext *ctx, arg_rrr_cf *a)
     tcg_gen_add2_i32(dest, cpu_psw_cb_msb, dest, cpu_psw_cb_msb, addc, zero);
 
     tcg_temp_free(addc);
-    tcg_temp_free(zero);
 
     /* Write back the result register.  */
     save_gpr(ctx, a->t, dest);
@@ -2971,9 +2956,8 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
      */
     gen_helper_ldc_check(addr);
 
-    zero = tcg_const_reg(0);
+    zero = tcg_constant_reg(0);
     tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
-    tcg_temp_free(zero);
 
     if (a->m) {
         save_gpr(ctx, a->b, ofs);
@@ -3886,15 +3870,13 @@ static bool trans_fcmp_f(DisasContext *ctx, arg_fclass2 *a)
 
     ta = load_frw0_i32(a->r1);
     tb = load_frw0_i32(a->r2);
-    ty = tcg_const_i32(a->y);
-    tc = tcg_const_i32(a->c);
+    ty = tcg_constant_i32(a->y);
+    tc = tcg_constant_i32(a->c);
 
     gen_helper_fcmp_s(cpu_env, ta, tb, ty, tc);
 
     tcg_temp_free_i32(ta);
     tcg_temp_free_i32(tb);
-    tcg_temp_free_i32(ty);
-    tcg_temp_free_i32(tc);
 
     return nullify_end(ctx);
 }
@@ -3908,15 +3890,13 @@ static bool trans_fcmp_d(DisasContext *ctx, arg_fclass2 *a)
 
     ta = load_frd0(a->r1);
     tb = load_frd0(a->r2);
-    ty = tcg_const_i32(a->y);
-    tc = tcg_const_i32(a->c);
+    ty = tcg_constant_i32(a->y);
+    tc = tcg_constant_i32(a->c);
 
     gen_helper_fcmp_d(cpu_env, ta, tb, ty, tc);
 
     tcg_temp_free_i64(ta);
     tcg_temp_free_i64(tb);
-    tcg_temp_free_i32(ty);
-    tcg_temp_free_i32(tc);
 
     return nullify_end(ctx);
 }
-- 
2.25.1

Re: [PATCH 1/2] target/hppa: Use tcg_constant_*

[Philippe Mathieu-Daudé]

On 7/8/21 10:58 PM, Richard Henderson wrote:
> Replace uses of tcg_const_* with the allocate and free
> close together.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/hppa/translate.c | 56 +++++++++++++----------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)
> 
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 64af1e0d5c..d17568880f 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c

> @@ -2971,9 +2956,8 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>       */
>      gen_helper_ldc_check(addr);
>  
> -    zero = tcg_const_reg(0);
> +    zero = tcg_constant_reg(0);
>      tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
> -    tcg_temp_free(zero);
>  
>      if (a->m) {
>          save_gpr(ctx, a->b, ofs);

What about this one?

@@ -3385,7 +3385,7 @@ static bool do_depw_sar(DisasContext *ct
     /* Convert big-endian bit numbering in SAR to left-shift.  */
     tcg_gen_xori_reg(shift, cpu_sar, TARGET_REGISTER_BITS - 1);

-    mask = tcg_const_reg(msb + (msb - 1));
+    mask = tcg_constant_reg(msb + (msb - 1));
     tcg_gen_and_reg(tmp, val, mask);
     if (rs) {
         tcg_gen_shl_reg(mask, mask, shift);
@@ -3396,7 +3396,6 @@ static bool do_depw_sar(DisasContext *ct
         tcg_gen_shl_reg(dest, tmp, shift);
     }
     tcg_temp_free(shift);
-    tcg_temp_free(mask);
     tcg_temp_free(tmp);
     save_gpr(ctx, rt, dest);

> @@ -3886,15 +3870,13 @@ static bool trans_fcmp_f(DisasContext *ctx, arg_fclass2 *a)
>  
>      ta = load_frw0_i32(a->r1);
>      tb = load_frw0_i32(a->r2);
> -    ty = tcg_const_i32(a->y);
> -    tc = tcg_const_i32(a->c);
> +    ty = tcg_constant_i32(a->y);
> +    tc = tcg_constant_i32(a->c);
>  
>      gen_helper_fcmp_s(cpu_env, ta, tb, ty, tc);
>  
>      tcg_temp_free_i32(ta);
>      tcg_temp_free_i32(tb);
> -    tcg_temp_free_i32(ty);
> -    tcg_temp_free_i32(tc);
>  
>      return nullify_end(ctx);
>  }

Re: [PATCH 1/2] target/hppa: Use tcg_constant_*

[Richard Henderson]

On 7/9/21 12:08 AM, Philippe Mathieu-Daudé wrote:
> What about this one?
> 
> @@ -3385,7 +3385,7 @@ static bool do_depw_sar(DisasContext *ct
>       /* Convert big-endian bit numbering in SAR to left-shift.  */
>       tcg_gen_xori_reg(shift, cpu_sar, TARGET_REGISTER_BITS - 1);
> 
> -    mask = tcg_const_reg(msb + (msb - 1));
> +    mask = tcg_constant_reg(msb + (msb - 1));
>       tcg_gen_and_reg(tmp, val, mask);
>       if (rs) {
>           tcg_gen_shl_reg(mask, mask, shift);

Also used as destination, so it's an initialized temp not a constant.


r~

Re: [PATCH 1/2] target/hppa: Use tcg_constant_*

[Philippe Mathieu-Daudé]

On 7/9/21 4:24 PM, Richard Henderson wrote:
> On 7/9/21 12:08 AM, Philippe Mathieu-Daudé wrote:
>> What about this one?
>>
>> @@ -3385,7 +3385,7 @@ static bool do_depw_sar(DisasContext *ct
>>       /* Convert big-endian bit numbering in SAR to left-shift.  */
>>       tcg_gen_xori_reg(shift, cpu_sar, TARGET_REGISTER_BITS - 1);
>>
>> -    mask = tcg_const_reg(msb + (msb - 1));
>> +    mask = tcg_constant_reg(msb + (msb - 1));
>>       tcg_gen_and_reg(tmp, val, mask);
>>       if (rs) {
>>           tcg_gen_shl_reg(mask, mask, shift);
> 
> Also used as destination, so it's an initialized temp not a constant.

Obviously... Sorry :)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 2/2] target/hppa: Clean up DisasCond

[Richard Henderson]

The a0_is_n flag is redundant with comparing a0 to cpu_psw_n.
The a1_is_0 flag can be removed by initializing a1 to $0,
which also means that cond_prep can be removed entirely.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/hppa/translate.c | 43 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 34 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index d17568880f..fcb814735f 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -253,8 +253,6 @@
 typedef struct DisasCond {
     TCGCond c;
     TCGv_reg a0, a1;
-    bool a0_is_n;
-    bool a1_is_0;
 } DisasCond;
 
 typedef struct DisasContext {
@@ -449,9 +447,7 @@ static DisasCond cond_make_n(void)
     return (DisasCond){
         .c = TCG_COND_NE,
         .a0 = cpu_psw_n,
-        .a0_is_n = true,
-        .a1 = NULL,
-        .a1_is_0 = true
+        .a1 = tcg_constant_reg(0)
     };
 }
 
@@ -459,7 +455,7 @@ static DisasCond cond_make_0_tmp(TCGCond c, TCGv_reg a0)
 {
     assert (c != TCG_COND_NEVER && c != TCG_COND_ALWAYS);
     return (DisasCond){
-        .c = c, .a0 = a0, .a1_is_0 = true
+        .c = c, .a0 = a0, .a1 = tcg_constant_reg(0)
     };
 }
 
@@ -483,26 +479,14 @@ static DisasCond cond_make(TCGCond c, TCGv_reg a0, TCGv_reg a1)
     return r;
 }
 
-static void cond_prep(DisasCond *cond)
-{
-    if (cond->a1_is_0) {
-        cond->a1_is_0 = false;
-        cond->a1 = tcg_const_reg(0);
-    }
-}
-
 static void cond_free(DisasCond *cond)
 {
     switch (cond->c) {
     default:
-        if (!cond->a0_is_n) {
+        if (cond->a0 != cpu_psw_n) {
             tcg_temp_free(cond->a0);
         }
-        if (!cond->a1_is_0) {
-            tcg_temp_free(cond->a1);
-        }
-        cond->a0_is_n = false;
-        cond->a1_is_0 = false;
+        tcg_temp_free(cond->a1);
         cond->a0 = NULL;
         cond->a1 = NULL;
         /* fallthru */
@@ -560,9 +544,8 @@ static TCGv_reg dest_gpr(DisasContext *ctx, unsigned reg)
 static void save_or_nullify(DisasContext *ctx, TCGv_reg dest, TCGv_reg t)
 {
     if (ctx->null_cond.c != TCG_COND_NEVER) {
-        cond_prep(&ctx->null_cond);
         tcg_gen_movcond_reg(ctx->null_cond.c, dest, ctx->null_cond.a0,
-                           ctx->null_cond.a1, dest, t);
+                            ctx->null_cond.a1, dest, t);
     } else {
         tcg_gen_mov_reg(dest, t);
     }
@@ -669,11 +652,9 @@ static void nullify_over(DisasContext *ctx)
         assert(ctx->null_cond.c != TCG_COND_ALWAYS);
 
         ctx->null_lab = gen_new_label();
-        cond_prep(&ctx->null_cond);
 
         /* If we're using PSW[N], copy it to a temp because... */
-        if (ctx->null_cond.a0_is_n) {
-            ctx->null_cond.a0_is_n = false;
+        if (ctx->null_cond.a0 == cpu_psw_n) {
             ctx->null_cond.a0 = tcg_temp_new();
             tcg_gen_mov_reg(ctx->null_cond.a0, cpu_psw_n);
         }
@@ -686,7 +667,7 @@ static void nullify_over(DisasContext *ctx)
         }
 
         tcg_gen_brcond_reg(ctx->null_cond.c, ctx->null_cond.a0,
-                          ctx->null_cond.a1, ctx->null_lab);
+                           ctx->null_cond.a1, ctx->null_lab);
         cond_free(&ctx->null_cond);
     }
 }
@@ -700,10 +681,9 @@ static void nullify_save(DisasContext *ctx)
         }
         return;
     }
-    if (!ctx->null_cond.a0_is_n) {
-        cond_prep(&ctx->null_cond);
+    if (ctx->null_cond.a0 != cpu_psw_n) {
         tcg_gen_setcond_reg(ctx->null_cond.c, cpu_psw_n,
-                           ctx->null_cond.a0, ctx->null_cond.a1);
+                            ctx->null_cond.a0, ctx->null_cond.a1);
         ctx->psw_n_nonzero = true;
     }
     cond_free(&ctx->null_cond);
@@ -1182,7 +1162,6 @@ static void do_add(DisasContext *ctx, unsigned rt, TCGv_reg in1,
     /* Emit any conditional trap before any writeback.  */
     cond = do_cond(cf, dest, cb_msb, sv);
     if (is_tc) {
-        cond_prep(&cond);
         tmp = tcg_temp_new();
         tcg_gen_setcond_reg(cond.c, tmp, cond.a0, cond.a1);
         gen_helper_tcond(cpu_env, tmp);
@@ -1277,7 +1256,6 @@ static void do_sub(DisasContext *ctx, unsigned rt, TCGv_reg in1,
 
     /* Emit any conditional trap before any writeback.  */
     if (is_tc) {
-        cond_prep(&cond);
         tmp = tcg_temp_new();
         tcg_gen_setcond_reg(cond.c, tmp, cond.a0, cond.a1);
         gen_helper_tcond(cpu_env, tmp);
@@ -1403,7 +1381,6 @@ static void do_unit(DisasContext *ctx, unsigned rt, TCGv_reg in1,
 
         if (is_tc) {
             TCGv_reg tmp = tcg_temp_new();
-            cond_prep(&cond);
             tcg_gen_setcond_reg(cond.c, tmp, cond.a0, cond.a1);
             gen_helper_tcond(cpu_env, tmp);
             tcg_temp_free(tmp);
@@ -1859,7 +1836,6 @@ static bool do_cbranch(DisasContext *ctx, target_sreg disp, bool is_n,
     }
 
     taken = gen_new_label();
-    cond_prep(cond);
     tcg_gen_brcond_reg(c, cond->a0, cond->a1, taken);
     cond_free(cond);
 
@@ -1956,7 +1932,6 @@ static bool do_ibranch(DisasContext *ctx, TCGv_reg dest,
         tcg_gen_lookup_and_goto_ptr();
         return nullify_end(ctx);
     } else {
-        cond_prep(&ctx->null_cond);
         c = ctx->null_cond.c;
         a0 = ctx->null_cond.a0;
         a1 = ctx->null_cond.a1;
-- 
2.25.1

Re: [PATCH 2/2] target/hppa: Clean up DisasCond

[Philippe Mathieu-Daudé]

On 7/8/21 10:58 PM, Richard Henderson wrote:
> The a0_is_n flag is redundant with comparing a0 to cpu_psw_n.

Preferably split as 2 patches:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> The a1_is_0 flag can be removed by initializing a1 to $0,
> which also means that cond_prep can be removed entirely.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/hppa/translate.c | 43 +++++++++--------------------------------
>  1 file changed, 9 insertions(+), 34 deletions(-)

Re: [PATCH 2/2] target/hppa: Clean up DisasCond

[Richard Henderson]

On 8/4/21 11:01 AM, Philippe Mathieu-Daudé wrote:
> On 7/8/21 10:58 PM, Richard Henderson wrote:
>> The a0_is_n flag is redundant with comparing a0 to cpu_psw_n.
> 
> Preferably split as 2 patches:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Already in mainline for 3 weeks.


r~