Re: [PATCH v11 08/26] target/loongarch: Add fixed point atomic instruction translation

[Richard Henderson]

On 11/19/21 7:13 AM, Song Gao wrote:
> This includes:
> - LL.{W/D}, SC.{W/D}
> - AM{SWAP/ADD/AND/OR/XOR/MAX/MIN}[_DB].{W/D}
> - AM{MAX/MIN}[_DB].{WU/DU}
>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/loongarch/insn_trans/trans_atomic.c.inc | 130 +++++++++++++++++++++++++
>   target/loongarch/insns.decode                  |  44 +++++++++
>   target/loongarch/translate.c                   |   1 +
>   3 files changed, 175 insertions(+)
>   create mode 100644 target/loongarch/insn_trans/trans_atomic.c.inc
>
> diff --git a/target/loongarch/insn_trans/trans_atomic.c.inc 
> b/target/loongarch/insn_trans/trans_atomic.c.inc
> new file mode 100644
> index 0000000..96957bb
> --- /dev/null
> +++ b/target/loongarch/insn_trans/trans_atomic.c.inc
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2021 Loongson Technology Corporation Limited
> + */
> +
> +static bool gen_ll(DisasContext *ctx, arg_rr_i *a,
> +                   void (*func)(TCGv, TCGv, int))
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
> +    TCGv t0 = tcg_temp_new();
> +
> +    tcg_gen_addi_tl(t0, src1, a->imm << 2);

The ll/sc instructions would of course use the same pre-shifted immediate as for 
ldptr/stptr, as discussed wrt the previous patch.

> +static bool gen_am(DisasContext *ctx, arg_rrr *a,
> +                   void (*func)(TCGv, TCGv, TCGv, TCGArg, MemOp),
> +                   MemOp mop)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv addr = gpr_src(ctx, a->rj, EXT_NONE);
> +    TCGv val = gpr_src(ctx, a->rk, EXT_NONE);
> +
> +    if ((a->rd != 0) && ((a->rj == a->rd) || (a->rk == a->rd))) {

BTW, you don't need to overdo it with the parenthesis.

   if (a != b && (c == d || e == f)) {

is fine.

> +static bool gen_am_db(DisasContext *ctx, arg_rrr *a,
> +                      void (*func)(TCGv, TCGv, TCGv, TCGArg, MemOp),
> +                      MemOp mop)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv addr = gpr_src(ctx, a->rj, EXT_NONE);
> +    TCGv val = gpr_src(ctx, a->rk, EXT_NONE);
> +
> +    if ((a->rd != 0) && ((a->rj == a->rd) || (a->rk == a->rd))) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Warning: source register overlaps destination register"
> +                      "in atomic insn at pc=0x" TARGET_FMT_lx "\n",
> +                      ctx->base.pc_next - 4);
> +        return false;
> +    }
> +
> +    gen_loongarch_sync(0x10);
> +    func(dest, addr, val, ctx->mem_idx, mop);

All tcg atomic ops are sequentially consistent, so you don't need the extra sync, and thus 
this entire function.



r~