Re: [PATCH v5 1/2] target/riscv: refactor Zicond support

[Alistair Francis]

On Wed, Mar 8, 2023 at 4:10 AM Philipp Tomsich <philipp.tomsich@vrull.eu> wrote:
>
> After the original Zicond support was stuck/fell through the cracks on
> the mailing list at v3 (and a different implementation was merged in
> the meanwhile), we need to refactor Zicond to prepare it to be reused
> by XVentanaCondOps.
>
> This commit lifts the common logic out into gen_czero and uses this
> via gen_logic and 2 helper functions (effectively partial closures).
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
> Changes in v5:
> - fix a rebase artifact
> - drop the 'inline' specifiers (as per review comments)
>
> Changes in v4:
> - rebase onto master
>
> Changes in v3:
> - don't add this to MAINTAINERS, as it is an official extension
>
> Changes in v2:
> - gates availability of the instructions through a REQUIRE_ZICOND
>   macro (these were previously always enabled)
>
>  target/riscv/insn_trans/trans_rvzicond.c.inc | 36 ++++++++++++--------
>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvzicond.c.inc 
> b/target/riscv/insn_trans/trans_rvzicond.c.inc
> index 645260164e..c8e43fa325 100644
> --- a/target/riscv/insn_trans/trans_rvzicond.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzicond.c.inc
> @@ -2,6 +2,7 @@
>   * RISC-V translation routines for the Zicond Standard Extension.
>   *
>   * Copyright (c) 2020-2023 PLCT Lab
> + * Copyright (c) 2022 VRULL GmbH.
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -22,28 +23,33 @@
>      }                                     \
>  } while (0)
>
> -static bool trans_czero_eqz(DisasContext *ctx, arg_czero_eqz *a)
> +/* Emits "$rd = ($rs2 <cond> $zero) ? $zero : $rs1" */
> +static void gen_czero(TCGv dest, TCGv src1, TCGv src2, TCGCond cond)
>  {
> -    REQUIRE_ZICOND(ctx);
> +    TCGv zero = tcg_constant_tl(0);
> +    tcg_gen_movcond_tl(cond, dest, src2, zero, zero, src1);
> +}
>
> -    TCGv dest = dest_gpr(ctx, a->rd);
> -    TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> -    TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +static void gen_czero_eqz(TCGv dest, TCGv src1, TCGv src2)
> +{
> +    gen_czero(dest, src1, src2, TCG_COND_EQ);
> +}
>
> -    tcg_gen_movcond_tl(TCG_COND_EQ, dest, src2, ctx->zero, ctx->zero, src1);
> -    gen_set_gpr(ctx, a->rd, dest);
> -    return true;
> +static void gen_czero_nez(TCGv dest, TCGv src1, TCGv src2)
> +{
> +    gen_czero(dest, src1, src2, TCG_COND_NE);
>  }
>
> -static bool trans_czero_nez(DisasContext *ctx, arg_czero_nez *a)
> +static bool trans_czero_eqz(DisasContext *ctx, arg_r *a)
>  {
>      REQUIRE_ZICOND(ctx);
>
> -    TCGv dest = dest_gpr(ctx, a->rd);
> -    TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> -    TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> +    return gen_logic(ctx, a, gen_czero_eqz);
> +}
> +
> +static bool trans_czero_nez(DisasContext *ctx, arg_r *a)
> +{
> +    REQUIRE_ZICOND(ctx);
>
> -    tcg_gen_movcond_tl(TCG_COND_NE, dest, src2, ctx->zero, ctx->zero, src1);
> -    gen_set_gpr(ctx, a->rd, dest);
> -    return true;
> +    return gen_logic(ctx, a, gen_czero_nez);
>  }
> --
> 2.34.1
>
>