[Qemu-devel] [PATCH 5/9] target/riscv: Use new atomic min/max expanders

Richard Henderson posted 9 patches 7 years, 6 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 5/9] target/riscv: Use new atomic min/max expanders
Posted by Richard Henderson 7 years, 6 months ago
Cc: Michael Clark <mjc@sifive.com>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/translate.c | 72 ++++++++++++++----------------------------------
 1 file changed, 20 insertions(+), 52 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 808eab7f50..9cab717088 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -725,7 +725,6 @@ static void gen_atomic(DisasContext *ctx, uint32_t opc,
     TCGv src1, src2, dat;
     TCGLabel *l1, *l2;
     TCGMemOp mop;
-    TCGCond cond;
     bool aq, rl;
 
     /* Extract the size of the atomic operation.  */
@@ -823,60 +822,29 @@ static void gen_atomic(DisasContext *ctx, uint32_t opc,
         tcg_gen_atomic_fetch_or_tl(src2, src1, src2, ctx->mem_idx, mop);
         gen_set_gpr(rd, src2);
         break;
-
     case OPC_RISC_AMOMIN:
-        cond = TCG_COND_LT;
-        goto do_minmax;
-    case OPC_RISC_AMOMAX:
-        cond = TCG_COND_GT;
-        goto do_minmax;
-    case OPC_RISC_AMOMINU:
-        cond = TCG_COND_LTU;
-        goto do_minmax;
-    case OPC_RISC_AMOMAXU:
-        cond = TCG_COND_GTU;
-        goto do_minmax;
-    do_minmax:
-        /* Handle the RL barrier.  The AQ barrier is handled along the
-           parallel path by the SC atomic cmpxchg.  On the serial path,
-           of course, barriers do not matter.  */
-        if (rl) {
-            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
-        }
-        if (tb_cflags(ctx->tb) & CF_PARALLEL) {
-            l1 = gen_new_label();
-            gen_set_label(l1);
-        } else {
-            l1 = NULL;
-        }
-
         gen_get_gpr(src1, rs1);
         gen_get_gpr(src2, rs2);
-        if ((mop & MO_SSIZE) == MO_SL) {
-            /* Sign-extend the register comparison input.  */
-            tcg_gen_ext32s_tl(src2, src2);
-        }
-        dat = tcg_temp_local_new();
-        tcg_gen_qemu_ld_tl(dat, src1, ctx->mem_idx, mop);
-        tcg_gen_movcond_tl(cond, src2, dat, src2, dat, src2);
-
-        if (tb_cflags(ctx->tb) & CF_PARALLEL) {
-            /* Parallel context.  Make this operation atomic by verifying
-               that the memory didn't change while we computed the result.  */
-            tcg_gen_atomic_cmpxchg_tl(src2, src1, dat, src2, ctx->mem_idx, mop);
-
-            /* If the cmpxchg failed, retry. */
-            /* ??? There is an assumption here that this will eventually
-               succeed, such that we don't live-lock.  This is not unlike
-               a similar loop that the compiler would generate for e.g.
-               __atomic_fetch_and_xor, so don't worry about it.  */
-            tcg_gen_brcond_tl(TCG_COND_NE, dat, src2, l1);
-        } else {
-            /* Serial context.  Directly store the result.  */
-            tcg_gen_qemu_st_tl(src2, src1, ctx->mem_idx, mop);
-        }
-        gen_set_gpr(rd, dat);
-        tcg_temp_free(dat);
+        tcg_gen_atomic_fetch_smin_tl(src2, src1, src2, ctx->mem_idx, mop);
+        gen_set_gpr(rd, src2);
+        break;
+    case OPC_RISC_AMOMAX:
+        gen_get_gpr(src1, rs1);
+        gen_get_gpr(src2, rs2);
+        tcg_gen_atomic_fetch_smax_tl(src2, src1, src2, ctx->mem_idx, mop);
+        gen_set_gpr(rd, src2);
+        break;
+    case OPC_RISC_AMOMINU:
+        gen_get_gpr(src1, rs1);
+        gen_get_gpr(src2, rs2);
+        tcg_gen_atomic_fetch_umin_tl(src2, src1, src2, ctx->mem_idx, mop);
+        gen_set_gpr(rd, src2);
+        break;
+    case OPC_RISC_AMOMAXU:
+        gen_get_gpr(src1, rs1);
+        gen_get_gpr(src2, rs2);
+        tcg_gen_atomic_fetch_umax_tl(src2, src1, src2, ctx->mem_idx, mop);
+        gen_set_gpr(rd, src2);
         break;
 
     default:
-- 
2.14.3


Re: [Qemu-devel] [PATCH 5/9] target/riscv: Use new atomic min/max expanders
Posted by Michael Clark 7 years, 6 months ago
On Fri, Apr 27, 2018 at 12:26 PM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> Cc: Michael Clark <mjc@sifive.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>

Reviewed-by: Michael Clark <mjc@sifive.com>

If you give me a remote I can pull into my local workspace and test. The
change looks pretty straight-forward. There are tests for amos in
riscv-tests but no parallel tests (i.e. contended case).

Regarding this target/riscv/translate.c. I'd like to make a patch to
remove CPURISCVState *env arguments from several gen functions.

---
>  target/riscv/translate.c | 72 ++++++++++++++----------------
> ------------------
>  1 file changed, 20 insertions(+), 52 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 808eab7f50..9cab717088 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -725,7 +725,6 @@ static void gen_atomic(DisasContext *ctx, uint32_t opc,
>      TCGv src1, src2, dat;
>      TCGLabel *l1, *l2;
>      TCGMemOp mop;
> -    TCGCond cond;
>      bool aq, rl;
>
>      /* Extract the size of the atomic operation.  */
> @@ -823,60 +822,29 @@ static void gen_atomic(DisasContext *ctx, uint32_t
> opc,
>          tcg_gen_atomic_fetch_or_tl(src2, src1, src2, ctx->mem_idx, mop);
>          gen_set_gpr(rd, src2);
>          break;
> -
>      case OPC_RISC_AMOMIN:
> -        cond = TCG_COND_LT;
> -        goto do_minmax;
> -    case OPC_RISC_AMOMAX:
> -        cond = TCG_COND_GT;
> -        goto do_minmax;
> -    case OPC_RISC_AMOMINU:
> -        cond = TCG_COND_LTU;
> -        goto do_minmax;
> -    case OPC_RISC_AMOMAXU:
> -        cond = TCG_COND_GTU;
> -        goto do_minmax;
> -    do_minmax:
> -        /* Handle the RL barrier.  The AQ barrier is handled along the
> -           parallel path by the SC atomic cmpxchg.  On the serial path,
> -           of course, barriers do not matter.  */
> -        if (rl) {
> -            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
> -        }
> -        if (tb_cflags(ctx->tb) & CF_PARALLEL) {
> -            l1 = gen_new_label();
> -            gen_set_label(l1);
> -        } else {
> -            l1 = NULL;
> -        }
> -
>          gen_get_gpr(src1, rs1);
>          gen_get_gpr(src2, rs2);
> -        if ((mop & MO_SSIZE) == MO_SL) {
> -            /* Sign-extend the register comparison input.  */
> -            tcg_gen_ext32s_tl(src2, src2);
> -        }
> -        dat = tcg_temp_local_new();
> -        tcg_gen_qemu_ld_tl(dat, src1, ctx->mem_idx, mop);
> -        tcg_gen_movcond_tl(cond, src2, dat, src2, dat, src2);
> -
> -        if (tb_cflags(ctx->tb) & CF_PARALLEL) {
> -            /* Parallel context.  Make this operation atomic by verifying
> -               that the memory didn't change while we computed the
> result.  */
> -            tcg_gen_atomic_cmpxchg_tl(src2, src1, dat, src2,
> ctx->mem_idx, mop);
> -
> -            /* If the cmpxchg failed, retry. */
> -            /* ??? There is an assumption here that this will eventually
> -               succeed, such that we don't live-lock.  This is not unlike
> -               a similar loop that the compiler would generate for e.g.
> -               __atomic_fetch_and_xor, so don't worry about it.  */
> -            tcg_gen_brcond_tl(TCG_COND_NE, dat, src2, l1);
> -        } else {
> -            /* Serial context.  Directly store the result.  */
> -            tcg_gen_qemu_st_tl(src2, src1, ctx->mem_idx, mop);
> -        }
> -        gen_set_gpr(rd, dat);
> -        tcg_temp_free(dat);
> +        tcg_gen_atomic_fetch_smin_tl(src2, src1, src2, ctx->mem_idx,
> mop);
> +        gen_set_gpr(rd, src2);
> +        break;
> +    case OPC_RISC_AMOMAX:
> +        gen_get_gpr(src1, rs1);
> +        gen_get_gpr(src2, rs2);
> +        tcg_gen_atomic_fetch_smax_tl(src2, src1, src2, ctx->mem_idx,
> mop);
> +        gen_set_gpr(rd, src2);
> +        break;
> +    case OPC_RISC_AMOMINU:
> +        gen_get_gpr(src1, rs1);
> +        gen_get_gpr(src2, rs2);
> +        tcg_gen_atomic_fetch_umin_tl(src2, src1, src2, ctx->mem_idx,
> mop);
> +        gen_set_gpr(rd, src2);
> +        break;
> +    case OPC_RISC_AMOMAXU:
> +        gen_get_gpr(src1, rs1);
> +        gen_get_gpr(src2, rs2);
> +        tcg_gen_atomic_fetch_umax_tl(src2, src1, src2, ctx->mem_idx,
> mop);
> +        gen_set_gpr(rd, src2);
>          break;
>
>      default:
> --
> 2.14.3
>
>
Re: [Qemu-devel] [PATCH 5/9] target/riscv: Use new atomic min/max expanders
Posted by Richard Henderson 7 years, 6 months ago
On 04/26/2018 09:24 PM, Michael Clark wrote:
> 
> 
> On Fri, Apr 27, 2018 at 12:26 PM, Richard Henderson
> <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> wrote:
> 
>     Cc: Michael Clark <mjc@sifive.com <mailto:mjc@sifive.com>>
>     Cc: Palmer Dabbelt <palmer@sifive.com <mailto:palmer@sifive.com>>
>     Cc: Sagar Karandikar <sagark@eecs.berkeley.edu
>     <mailto:sagark@eecs.berkeley.edu>>
>     Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de
>     <mailto:kbastian@mail.uni-paderborn.de>>
>     Signed-off-by: Richard Henderson <richard.henderson@linaro.org
>     <mailto:richard.henderson@linaro.org>>
> 
> 
> Reviewed-by: Michael Clark <mjc@sifive.com <mailto:mjc@sifive.com>>
> 
> If you give me a remote I can pull into my local workspace and test. The change
> looks pretty straight-forward. There are tests for amos in riscv-tests but no
> parallel tests (i.e. contended case).

git://github.com/rth7680/qemu.git tgt-arm-atomic-2


r~

Re: [Qemu-devel] [PATCH 5/9] target/riscv: Use new atomic min/max expanders
Posted by Michael Clark 7 years, 6 months ago
On Sat, Apr 28, 2018 at 5:53 AM, Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 04/26/2018 09:24 PM, Michael Clark wrote:
> >
> >
> > On Fri, Apr 27, 2018 at 12:26 PM, Richard Henderson
> > <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>>
> wrote:
> >
> >     Cc: Michael Clark <mjc@sifive.com <mailto:mjc@sifive.com>>
> >     Cc: Palmer Dabbelt <palmer@sifive.com <mailto:palmer@sifive.com>>
> >     Cc: Sagar Karandikar <sagark@eecs.berkeley.edu
> >     <mailto:sagark@eecs.berkeley.edu>>
> >     Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de
> >     <mailto:kbastian@mail.uni-paderborn.de>>
> >     Signed-off-by: Richard Henderson <richard.henderson@linaro.org
> >     <mailto:richard.henderson@linaro.org>>
> >
> >
> > Reviewed-by: Michael Clark <mjc@sifive.com <mailto:mjc@sifive.com>>
> >
> > If you give me a remote I can pull into my local workspace and test. The
> change
> > looks pretty straight-forward. There are tests for amos in riscv-tests
> but no
> > parallel tests (i.e. contended case).
>
> git://github.com/rth7680/qemu.git tgt-arm-atomic-2


I just did a fetch and only saw tgt-arm-atomic. Maybe you haven't pushed
tgt-arm-atomic-2.

In any case the tgt-arm-atomic branch passes riscv-tests.

Michael.