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

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

[Philipp Tomsich]

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

Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---

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/cpu.c                           |  3 --
 target/riscv/insn_trans/trans_rvzicond.c.inc | 36 ++++++++++++--------
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5bc0005cc7..88c2484eee 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1191,9 +1191,6 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
     DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
 
-    DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
-    DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
-
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/target/riscv/insn_trans/trans_rvzicond.c.inc b/target/riscv/insn_trans/trans_rvzicond.c.inc
index 645260164e..b35cb4cbc7 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 inline 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 inline 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 inline 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

[PATCH v4 2/2] target/riscv: redirect XVentanaCondOps to use the Zicond functions

[Philipp Tomsich]

The Zicond standard extension implements the same instruction
semantics as XVentanaCondOps, although using different mnemonics and
opcodes.

Point XVentanaCondOps to the (newly implemented) Zicond implementation
to reduce the future maintenance burden.

Also updating MAINTAINERS as trans_xventanacondops.c.inc.

Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---

(no changes since v3)

Changes in v3:
- Don't downgrade to "Odd Fixes", but rather to "Maintained" (we are
  not being paid to look after this, but will look after it
  nonetheless).

Changes in v2:
- Calls into the gen_czero_{eqz,nez} helpers instead of calling
  trans_czero_{eqz,nez} to bypass the require-check and ensure that
  XVentanaCondOps can be enabled/disabled independently of Zicond.

 MAINTAINERS                                    |  2 +-
 .../insn_trans/trans_xventanacondops.c.inc     | 18 +++---------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 011fd85a09..1ad3c6fc9a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -311,7 +311,7 @@ F: target/riscv/xthead*.decode
 RISC-V XVentanaCondOps extension
 M: Philipp Tomsich <philipp.tomsich@vrull.eu>
 L: qemu-riscv@nongnu.org
-S: Supported
+S: Maintained
 F: target/riscv/XVentanaCondOps.decode
 F: target/riscv/insn_trans/trans_xventanacondops.c.inc
 
diff --git a/target/riscv/insn_trans/trans_xventanacondops.c.inc b/target/riscv/insn_trans/trans_xventanacondops.c.inc
index 16849e6d4e..38c15f2825 100644
--- a/target/riscv/insn_trans/trans_xventanacondops.c.inc
+++ b/target/riscv/insn_trans/trans_xventanacondops.c.inc
@@ -1,7 +1,7 @@
 /*
  * RISC-V translation routines for the XVentanaCondOps extension.
  *
- * Copyright (c) 2021-2022 VRULL GmbH.
+ * Copyright (c) 2021-2023 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,
@@ -16,24 +16,12 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-static bool gen_vt_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
-{
-    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);
-
-    tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
-
-    gen_set_gpr(ctx, a->rd, dest);
-    return true;
-}
-
 static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
 {
-    return gen_vt_condmask(ctx, a, TCG_COND_NE);
+    return gen_logic(ctx, a, gen_czero_eqz);
 }
 
 static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
 {
-    return gen_vt_condmask(ctx, a, TCG_COND_EQ);
+    return gen_logic(ctx, a, gen_czero_nez);
 }
-- 
2.34.1

Re: [PATCH v4 2/2] target/riscv: redirect XVentanaCondOps to use the Zicond functions

[Richard Henderson]

On 3/6/23 07:23, Philipp Tomsich wrote:
> The Zicond standard extension implements the same instruction
> semantics as XVentanaCondOps, although using different mnemonics and
> opcodes.
> 
> Point XVentanaCondOps to the (newly implemented) Zicond implementation
> to reduce the future maintenance burden.
> 
> Also updating MAINTAINERS as trans_xventanacondops.c.inc.
> 
> Signed-off-by: Philipp Tomsich<philipp.tomsich@vrull.eu>
> ---

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

r~

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

[Richard Henderson]

On 3/6/23 07:23, Philipp Tomsich 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).
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>

For any multi-patch series, you should use a cover letter.
Lacking this causes the set to be missed by tooling.

> -static bool trans_czero_eqz(DisasContext *ctx, arg_czero_eqz *a)
> +/* Emits "$rd = ($rs2 <cond> $zero) ? $zero : $rs1" */
> +static inline void gen_czero(TCGv dest, TCGv src1, TCGv src2, TCGCond cond)

Drop the inline markers and let the compiler choose.

> +static inline void gen_czero_eqz(TCGv dest, TCGv src1, TCGv src2)
> +static inline void gen_czero_nez(TCGv dest, TCGv src1, TCGv src2)

These especially, where we use their function pointer...

> +    return gen_logic(ctx, a, gen_czero_eqz);
> +    return gen_logic(ctx, a, gen_czero_nez);

here, so they will most definitely exist out-of-line.

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


r~