[PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve

[PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve

[Nicholas Piggin]

lqarx does not set cpu_reserve, which causes stqcx. to never succeed.

Cc: qemu-stable@nongnu.org
Fixes: 94bf2658676 ("target/ppc: Use atomic load for LQ and LQARX")
Fixes: 57b38ffd0c6 ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Fix bugs vs memory access fault [Richard]

 target/ppc/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 3650d2985d..7a5bf1d820 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3881,6 +3881,7 @@ static void gen_lqarx(DisasContext *ctx)
     tcg_gen_qemu_ld_i128(t16, EA, ctx->mem_idx, DEF_MEMOP(MO_128 | MO_ALIGN));
     tcg_gen_extr_i128_i64(lo, hi, t16);
 
+    tcg_gen_mov_tl(cpu_reserve, EA);
     tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
     tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
 }
-- 
2.40.1

[PATCH v2 2/4] target/ppc: Ensure stcx size matches larx

[Nicholas Piggin]

Differently-sized larx/stcx. pairs can succeed if the starting address
matches. Add a check to require the size of stcx. exactly match the larx
that established the reservation. Use the term "reserve_length" for this
state, which matches the terminology used in the ISA.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Changed lqarx/stqcx. reservation size to 16 [Richard]
- Changed name to reserve_length [Richard]

 target/ppc/cpu.h       | 5 +++--
 target/ppc/cpu_init.c  | 4 ++--
 target/ppc/translate.c | 9 +++++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 7959bfed0a..45d84ce06a 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1123,8 +1123,9 @@ struct CPUArchState {
     target_ulong ov32;
     target_ulong ca32;
 
-    target_ulong reserve_addr; /* Reservation address */
-    target_ulong reserve_val;  /* Reservation value */
+    target_ulong reserve_addr;   /* Reservation address */
+    target_ulong reserve_length; /* Reservation larx op size (bytes) */
+    target_ulong reserve_val;    /* Reservation value */
     target_ulong reserve_val2;
 
     /* These are used in supervisor mode only */
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 944a74befe..c3dd7052a3 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7421,8 +7421,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
         }
         qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
     }
-    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
-                 env->reserve_addr);
+    qemu_fprintf(f, " ]     RES %03x@" TARGET_FMT_lx "\n",
+                 (int)env->reserve_length, env->reserve_addr);
 
     if (flags & CPU_DUMP_FPU) {
         for (i = 0; i < 32; i++) {
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 7a5bf1d820..538f757dec 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -71,6 +71,7 @@ static TCGv cpu_cfar;
 #endif
 static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
 static TCGv cpu_reserve;
+static TCGv cpu_reserve_length;
 static TCGv cpu_reserve_val;
 static TCGv cpu_reserve_val2;
 static TCGv cpu_fpscr;
@@ -141,6 +142,10 @@ void ppc_translate_init(void)
     cpu_reserve = tcg_global_mem_new(cpu_env,
                                      offsetof(CPUPPCState, reserve_addr),
                                      "reserve_addr");
+    cpu_reserve_length = tcg_global_mem_new(cpu_env,
+                                            offsetof(CPUPPCState,
+                                                     reserve_length),
+                                            "reserve_length");
     cpu_reserve_val = tcg_global_mem_new(cpu_env,
                                          offsetof(CPUPPCState, reserve_val),
                                          "reserve_val");
@@ -3585,6 +3590,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
     gen_addr_reg_index(ctx, t0);
     tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
     tcg_gen_mov_tl(cpu_reserve, t0);
+    tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
     tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
 }
@@ -3816,6 +3822,7 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
     gen_set_access_type(ctx, ACCESS_RES);
     gen_addr_reg_index(ctx, t0);
     tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
 
     t0 = tcg_temp_new();
     tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
@@ -3882,6 +3889,7 @@ static void gen_lqarx(DisasContext *ctx)
     tcg_gen_extr_i128_i64(lo, hi, t16);
 
     tcg_gen_mov_tl(cpu_reserve, EA);
+    tcg_gen_movi_tl(cpu_reserve_length, 16);
     tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
     tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
 }
@@ -3907,6 +3915,7 @@ static void gen_stqcx_(DisasContext *ctx)
     gen_addr_reg_index(ctx, EA);
 
     tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
 
     cmp = tcg_temp_new_i128();
     val = tcg_temp_new_i128();
-- 
2.40.1

[PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics

[Nicholas Piggin]

larx and stcx. are not defined to order any memory operations.
Remove the barriers.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/translate.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 538f757dec..acb99d8691 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3592,7 +3592,6 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
     tcg_gen_mov_tl(cpu_reserve, t0);
     tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
 }
 
 #define LARX(name, memop)                  \
@@ -3836,11 +3835,6 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
 
     gen_set_label(l1);
 
-    /*
-     * Address mismatch implies failure.  But we still need to provide
-     * the memory barrier semantics of the instruction.
-     */
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
 
     gen_set_label(l2);
@@ -3944,11 +3938,6 @@ static void gen_stqcx_(DisasContext *ctx)
     tcg_gen_br(lab_over);
     gen_set_label(lab_fail);
 
-    /*
-     * Address mismatch implies failure.  But we still need to provide
-     * the memory barrier semantics of the instruction.
-     */
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
 
     gen_set_label(lab_over);
-- 
2.40.1

[PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch

[Nicholas Piggin]

Rework store conditional to avoid a branch in the success case.
Change some of the variable names and layout while here so
gen_conditional_store more closely matches gen_stqcx_.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Reinstate lost DEF_MEMOP [Richard]

I think the DEF_MEMOP is redundant here, but admit that's not something
that should be changed with this patch. I will look at cleaning those up
later.

Thanks,
Nick

 target/ppc/translate.c | 63 ++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index acb99d8691..434caad258 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3813,31 +3813,32 @@ static void gen_stdat(DisasContext *ctx)
 
 static void gen_conditional_store(DisasContext *ctx, MemOp memop)
 {
-    TCGLabel *l1 = gen_new_label();
-    TCGLabel *l2 = gen_new_label();
-    TCGv t0 = tcg_temp_new();
-    int reg = rS(ctx->opcode);
+    TCGLabel *lfail;
+    TCGv EA;
+    TCGv cr0;
+    TCGv t0;
+    int rs = rS(ctx->opcode);
 
+    lfail = gen_new_label();
+    EA = tcg_temp_new();
+    cr0 = tcg_temp_new();
+    t0 = tcg_temp_new();
+
+    tcg_gen_mov_tl(cr0, cpu_so);
     gen_set_access_type(ctx, ACCESS_RES);
-    gen_addr_reg_index(ctx, t0);
-    tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
-    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
+    gen_addr_reg_index(ctx, EA);
+    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), lfail);
 
-    t0 = tcg_temp_new();
     tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
-                              cpu_gpr[reg], ctx->mem_idx,
+                              cpu_gpr[rs], ctx->mem_idx,
                               DEF_MEMOP(memop) | MO_ALIGN);
     tcg_gen_setcond_tl(TCG_COND_EQ, t0, t0, cpu_reserve_val);
     tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
-    tcg_gen_or_tl(t0, t0, cpu_so);
-    tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
-    tcg_gen_br(l2);
+    tcg_gen_or_tl(cr0, cr0, t0);
 
-    gen_set_label(l1);
-
-    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
-
-    gen_set_label(l2);
+    gen_set_label(lfail);
+    tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
     tcg_gen_movi_tl(cpu_reserve, -1);
 }
 
@@ -3891,25 +3892,26 @@ static void gen_lqarx(DisasContext *ctx)
 /* stqcx. */
 static void gen_stqcx_(DisasContext *ctx)
 {
-    TCGLabel *lab_fail, *lab_over;
-    int rs = rS(ctx->opcode);
+    TCGLabel *lfail;
     TCGv EA, t0, t1;
+    TCGv cr0;
     TCGv_i128 cmp, val;
+    int rs = rS(ctx->opcode);
 
     if (unlikely(rs & 1)) {
         gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         return;
     }
 
-    lab_fail = gen_new_label();
-    lab_over = gen_new_label();
+    lfail = gen_new_label();
+    EA = tcg_temp_new();
+    cr0 = tcg_temp_new();
 
+    tcg_gen_mov_tl(cr0, cpu_so);
     gen_set_access_type(ctx, ACCESS_RES);
-    EA = tcg_temp_new();
     gen_addr_reg_index(ctx, EA);
-
-    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
-    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
+    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lfail);
 
     cmp = tcg_temp_new_i128();
     val = tcg_temp_new_i128();
@@ -3932,15 +3934,10 @@ static void gen_stqcx_(DisasContext *ctx)
 
     tcg_gen_setcondi_tl(TCG_COND_EQ, t0, t0, 0);
     tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
-    tcg_gen_or_tl(t0, t0, cpu_so);
-    tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
-
-    tcg_gen_br(lab_over);
-    gen_set_label(lab_fail);
-
-    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
+    tcg_gen_or_tl(cr0, cr0, t0);
 
-    gen_set_label(lab_over);
+    gen_set_label(lfail);
+    tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
     tcg_gen_movi_tl(cpu_reserve, -1);
 }
 #endif /* defined(TARGET_PPC64) */
-- 
2.40.1

Re: [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve

[Richard Henderson]

On 6/4/23 19:54, Nicholas Piggin wrote:
> lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: 94bf2658676 ("target/ppc: Use atomic load for LQ and LQARX")
> Fixes: 57b38ffd0c6 ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ")
> Signed-off-by: Nicholas Piggin<npiggin@gmail.com>
> ---
> v2:
> - Fix bugs vs memory access fault [Richard]
> 
>   target/ppc/translate.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve

[Daniel Henrique Barboza]

On 6/4/23 23:54, Nicholas Piggin wrote:
> lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 94bf2658676 ("target/ppc: Use atomic load for LQ and LQARX")
> Fixes: 57b38ffd0c6 ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Queued. Thanks,


Daniel

> v2:
> - Fix bugs vs memory access fault [Richard]
> 
>   target/ppc/translate.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 3650d2985d..7a5bf1d820 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3881,6 +3881,7 @@ static void gen_lqarx(DisasContext *ctx)
>       tcg_gen_qemu_ld_i128(t16, EA, ctx->mem_idx, DEF_MEMOP(MO_128 | MO_ALIGN));
>       tcg_gen_extr_i128_i64(lo, hi, t16);
>   
> +    tcg_gen_mov_tl(cpu_reserve, EA);
>       tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
>       tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
>   }

Re: [PATCH v2 2/4] target/ppc: Ensure stcx size matches larx

[Daniel Henrique Barboza]

On 6/4/23 23:54, Nicholas Piggin wrote:
> Differently-sized larx/stcx. pairs can succeed if the starting address
> matches. Add a check to require the size of stcx. exactly match the larx
> that established the reservation. Use the term "reserve_length" for this
> state, which matches the terminology used in the ISA.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Queued. Thanks,


Daniel

> v2:
> - Changed lqarx/stqcx. reservation size to 16 [Richard]
> - Changed name to reserve_length [Richard]
> 
>   target/ppc/cpu.h       | 5 +++--
>   target/ppc/cpu_init.c  | 4 ++--
>   target/ppc/translate.c | 9 +++++++++
>   3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 7959bfed0a..45d84ce06a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1123,8 +1123,9 @@ struct CPUArchState {
>       target_ulong ov32;
>       target_ulong ca32;
>   
> -    target_ulong reserve_addr; /* Reservation address */
> -    target_ulong reserve_val;  /* Reservation value */
> +    target_ulong reserve_addr;   /* Reservation address */
> +    target_ulong reserve_length; /* Reservation larx op size (bytes) */
> +    target_ulong reserve_val;    /* Reservation value */
>       target_ulong reserve_val2;
>   
>       /* These are used in supervisor mode only */
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 944a74befe..c3dd7052a3 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7421,8 +7421,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>           }
>           qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
>       }
> -    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
> -                 env->reserve_addr);
> +    qemu_fprintf(f, " ]     RES %03x@" TARGET_FMT_lx "\n",
> +                 (int)env->reserve_length, env->reserve_addr);
>   
>       if (flags & CPU_DUMP_FPU) {
>           for (i = 0; i < 32; i++) {
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7a5bf1d820..538f757dec 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -71,6 +71,7 @@ static TCGv cpu_cfar;
>   #endif
>   static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
>   static TCGv cpu_reserve;
> +static TCGv cpu_reserve_length;
>   static TCGv cpu_reserve_val;
>   static TCGv cpu_reserve_val2;
>   static TCGv cpu_fpscr;
> @@ -141,6 +142,10 @@ void ppc_translate_init(void)
>       cpu_reserve = tcg_global_mem_new(cpu_env,
>                                        offsetof(CPUPPCState, reserve_addr),
>                                        "reserve_addr");
> +    cpu_reserve_length = tcg_global_mem_new(cpu_env,
> +                                            offsetof(CPUPPCState,
> +                                                     reserve_length),
> +                                            "reserve_length");
>       cpu_reserve_val = tcg_global_mem_new(cpu_env,
>                                            offsetof(CPUPPCState, reserve_val),
>                                            "reserve_val");
> @@ -3585,6 +3590,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
>       gen_addr_reg_index(ctx, t0);
>       tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
>       tcg_gen_mov_tl(cpu_reserve, t0);
> +    tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
>       tcg_gen_mov_tl(cpu_reserve_val, gpr);
>       tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
>   }
> @@ -3816,6 +3822,7 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
>       gen_set_access_type(ctx, ACCESS_RES);
>       gen_addr_reg_index(ctx, t0);
>       tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
>   
>       t0 = tcg_temp_new();
>       tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
> @@ -3882,6 +3889,7 @@ static void gen_lqarx(DisasContext *ctx)
>       tcg_gen_extr_i128_i64(lo, hi, t16);
>   
>       tcg_gen_mov_tl(cpu_reserve, EA);
> +    tcg_gen_movi_tl(cpu_reserve_length, 16);
>       tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
>       tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
>   }
> @@ -3907,6 +3915,7 @@ static void gen_stqcx_(DisasContext *ctx)
>       gen_addr_reg_index(ctx, EA);
>   
>       tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
>   
>       cmp = tcg_temp_new_i128();
>       val = tcg_temp_new_i128();

Re: [PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics

[Daniel Henrique Barboza]

On 6/4/23 23:54, Nicholas Piggin wrote:
> larx and stcx. are not defined to order any memory operations.
> Remove the barriers.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Queued. Thanks,


Daniel

>   target/ppc/translate.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 538f757dec..acb99d8691 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3592,7 +3592,6 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
>       tcg_gen_mov_tl(cpu_reserve, t0);
>       tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
>       tcg_gen_mov_tl(cpu_reserve_val, gpr);
> -    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
>   }
>   
>   #define LARX(name, memop)                  \
> @@ -3836,11 +3835,6 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
>   
>       gen_set_label(l1);
>   
> -    /*
> -     * Address mismatch implies failure.  But we still need to provide
> -     * the memory barrier semantics of the instruction.
> -     */
> -    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
>       tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
>   
>       gen_set_label(l2);
> @@ -3944,11 +3938,6 @@ static void gen_stqcx_(DisasContext *ctx)
>       tcg_gen_br(lab_over);
>       gen_set_label(lab_fail);
>   
> -    /*
> -     * Address mismatch implies failure.  But we still need to provide
> -     * the memory barrier semantics of the instruction.
> -     */
> -    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
>       tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
>   
>       gen_set_label(lab_over);

Re: [PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch

[Daniel Henrique Barboza]

On 6/4/23 23:54, Nicholas Piggin wrote:
> Rework store conditional to avoid a branch in the success case.
> Change some of the variable names and layout while here so
> gen_conditional_store more closely matches gen_stqcx_.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Queued. Thanks,


Daniel

> v2:
> - Reinstate lost DEF_MEMOP [Richard]
> 
> I think the DEF_MEMOP is redundant here, but admit that's not something
> that should be changed with this patch. I will look at cleaning those up
> later.
> 
> Thanks,
> Nick
> 
>   target/ppc/translate.c | 63 ++++++++++++++++++++----------------------
>   1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index acb99d8691..434caad258 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3813,31 +3813,32 @@ static void gen_stdat(DisasContext *ctx)
>   
>   static void gen_conditional_store(DisasContext *ctx, MemOp memop)
>   {
> -    TCGLabel *l1 = gen_new_label();
> -    TCGLabel *l2 = gen_new_label();
> -    TCGv t0 = tcg_temp_new();
> -    int reg = rS(ctx->opcode);
> +    TCGLabel *lfail;
> +    TCGv EA;
> +    TCGv cr0;
> +    TCGv t0;
> +    int rs = rS(ctx->opcode);
>   
> +    lfail = gen_new_label();
> +    EA = tcg_temp_new();
> +    cr0 = tcg_temp_new();
> +    t0 = tcg_temp_new();
> +
> +    tcg_gen_mov_tl(cr0, cpu_so);
>       gen_set_access_type(ctx, ACCESS_RES);
> -    gen_addr_reg_index(ctx, t0);
> -    tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
> -    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
> +    gen_addr_reg_index(ctx, EA);
> +    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), lfail);
>   
> -    t0 = tcg_temp_new();
>       tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
> -                              cpu_gpr[reg], ctx->mem_idx,
> +                              cpu_gpr[rs], ctx->mem_idx,
>                                 DEF_MEMOP(memop) | MO_ALIGN);
>       tcg_gen_setcond_tl(TCG_COND_EQ, t0, t0, cpu_reserve_val);
>       tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
> -    tcg_gen_or_tl(t0, t0, cpu_so);
> -    tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
> -    tcg_gen_br(l2);
> +    tcg_gen_or_tl(cr0, cr0, t0);
>   
> -    gen_set_label(l1);
> -
> -    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> -
> -    gen_set_label(l2);
> +    gen_set_label(lfail);
> +    tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
>       tcg_gen_movi_tl(cpu_reserve, -1);
>   }
>   
> @@ -3891,25 +3892,26 @@ static void gen_lqarx(DisasContext *ctx)
>   /* stqcx. */
>   static void gen_stqcx_(DisasContext *ctx)
>   {
> -    TCGLabel *lab_fail, *lab_over;
> -    int rs = rS(ctx->opcode);
> +    TCGLabel *lfail;
>       TCGv EA, t0, t1;
> +    TCGv cr0;
>       TCGv_i128 cmp, val;
> +    int rs = rS(ctx->opcode);
>   
>       if (unlikely(rs & 1)) {
>           gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
>           return;
>       }
>   
> -    lab_fail = gen_new_label();
> -    lab_over = gen_new_label();
> +    lfail = gen_new_label();
> +    EA = tcg_temp_new();
> +    cr0 = tcg_temp_new();
>   
> +    tcg_gen_mov_tl(cr0, cpu_so);
>       gen_set_access_type(ctx, ACCESS_RES);
> -    EA = tcg_temp_new();
>       gen_addr_reg_index(ctx, EA);
> -
> -    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> -    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
> +    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lfail);
>   
>       cmp = tcg_temp_new_i128();
>       val = tcg_temp_new_i128();
> @@ -3932,15 +3934,10 @@ static void gen_stqcx_(DisasContext *ctx)
>   
>       tcg_gen_setcondi_tl(TCG_COND_EQ, t0, t0, 0);
>       tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
> -    tcg_gen_or_tl(t0, t0, cpu_so);
> -    tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
> -
> -    tcg_gen_br(lab_over);
> -    gen_set_label(lab_fail);
> -
> -    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> +    tcg_gen_or_tl(cr0, cr0, t0);
>   
> -    gen_set_label(lab_over);
> +    gen_set_label(lfail);
> +    tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
>       tcg_gen_movi_tl(cpu_reserve, -1);
>   }
>   #endif /* defined(TARGET_PPC64) */