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

[Michael Clark]

On Fri, Apr 27, 2018 at 12:26 PM, Richard Henderson <
address@hidden> wrote:

> Cc: Michael Clark <address@hidden>
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Sagar Karandikar <address@hidden>
> Cc: Bastian Koppelmann <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
>

Reviewed-by: Michael Clark <address@hidden>

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
>
>