Hi Alistair, Thanks for providing some feedback - responses below:
On Mon, 2023-12-18 at 12:54 +1000, Alistair Francis wrote: > On Fri, Dec 8, 2023 at 1:40 AM Rob Bradford <rbradf...@rivosinc.com> > wrote: > > > > From: Weiwei Li <liwei...@iscas.ac.cn> > > > > Add support for amocas.w/d/q instructions which are part of the > > ratified > > Zacas extension: https://github.com/riscv/riscv-zacas > > > > Signed-off-by: Weiwei Li <liwei...@iscas.ac.cn> > > Signed-off-by: Junqiang Wang <wangjunqi...@iscas.ac.cn> > > Signed-off-by: Rob Bradford <rbradf...@rivosinc.com> > > --- > > target/riscv/cpu.c | 2 + > > target/riscv/cpu_cfg.h | 1 + > > target/riscv/insn32.decode | 6 + > > target/riscv/insn_trans/trans_rvzacas.c.inc | 150 > > ++++++++++++++++++++ > > target/riscv/tcg/tcg-cpu.c | 5 + > > target/riscv/translate.c | 1 + > > 6 files changed, 165 insertions(+) > > create mode 100644 target/riscv/insn_trans/trans_rvzacas.c.inc > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index 83c7c0cf07..a265833b17 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -87,6 +87,7 @@ const RISCVIsaExtData isa_edata_arr[] = { > > ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, > > ext_zihintpause), > > ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm), > > ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul), > > + ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas), > > ISA_EXT_DATA_ENTRY(zawrs, PRIV_VERSION_1_12_0, ext_zawrs), > > ISA_EXT_DATA_ENTRY(zfa, PRIV_VERSION_1_12_0, ext_zfa), > > ISA_EXT_DATA_ENTRY(zfbfmin, PRIV_VERSION_1_12_0, ext_zfbfmin), > > @@ -1300,6 +1301,7 @@ const RISCVCPUMultiExtConfig > > riscv_cpu_extensions[] = { > > MULTI_EXT_CFG_BOOL("zicsr", ext_zicsr, true), > > MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true), > > MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true), > > + MULTI_EXT_CFG_BOOL("zacas", ext_zacas, false), > > MULTI_EXT_CFG_BOOL("zawrs", ext_zawrs, true), > > MULTI_EXT_CFG_BOOL("zfa", ext_zfa, true), > > MULTI_EXT_CFG_BOOL("zfh", ext_zfh, false), > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h > > index f4605fb190..d516de4a44 100644 > > --- a/target/riscv/cpu_cfg.h > > +++ b/target/riscv/cpu_cfg.h > > @@ -77,6 +77,7 @@ struct RISCVCPUConfig { > > bool ext_svnapot; > > bool ext_svpbmt; > > bool ext_zdinx; > > + bool ext_zacas; > > bool ext_zawrs; > > bool ext_zfa; > > bool ext_zfbfmin; > > diff --git a/target/riscv/insn32.decode > > b/target/riscv/insn32.decode > > index 33597fe2bb..f22df04cfd 100644 > > --- a/target/riscv/insn32.decode > > +++ b/target/riscv/insn32.decode > > @@ -1004,3 +1004,9 @@ vgmul_vv 101000 1 ..... 10001 010 ..... > > 1110111 @r2_vm_1 > > vsm4k_vi 100001 1 ..... ..... 010 ..... 1110111 @r_vm_1 > > vsm4r_vv 101000 1 ..... 10000 010 ..... 1110111 @r2_vm_1 > > vsm4r_vs 101001 1 ..... 10000 010 ..... 1110111 @r2_vm_1 > > + > > +# *** RV32 Zacas Standard Extension *** > > +amocas_w 00101 . . ..... ..... 010 ..... 0101111 @atom_st > > +amocas_d 00101 . . ..... ..... 011 ..... 0101111 @atom_st > > +# *** RV64 Zacas Standard Extension *** > > +amocas_q 00101 . . ..... ..... 100 ..... 0101111 @atom_st > > diff --git a/target/riscv/insn_trans/trans_rvzacas.c.inc > > b/target/riscv/insn_trans/trans_rvzacas.c.inc > > new file mode 100644 > > index 0000000000..5d274d4c08 > > --- /dev/null > > +++ b/target/riscv/insn_trans/trans_rvzacas.c.inc > > @@ -0,0 +1,150 @@ > > +/* > > + * RISC-V translation routines for the RV64 Zacas Standard > > Extension. > > + * > > + * Copyright (c) 2020-2023 PLCT Lab > > + * > > + * This program is free software; you can redistribute it and/or > > modify it > > + * under the terms and conditions of the GNU General Public > > License, > > + * version 2 or later, as published by the Free Software > > Foundation. > > + * > > + * This program is distributed in the hope it will be useful, but > > WITHOUT > > + * ANY WARRANTY; without even the implied warranty of > > MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public > > License for > > + * more details. > > + * > > + * You should have received a copy of the GNU General Public > > License along with > > + * this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#define REQUIRE_ZACAS(ctx) do { \ > > + if (!ctx->cfg_ptr->ext_zacas) { \ > > + return false; \ > > + } \ > > +} while (0) > > + > > +static bool gen_cmpxchg(DisasContext *ctx, arg_atomic *a, MemOp > > mop) > > +{ > > + TCGv dest = get_gpr(ctx, a->rd, EXT_NONE); > > + TCGv src1 = get_address(ctx, a->rs1, 0); > > + TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE); > > + > > + decode_save_opc(ctx); > > + tcg_gen_atomic_cmpxchg_tl(dest, src1, dest, src2, ctx- > > >mem_idx, mop); > > + > > + gen_set_gpr(ctx, a->rd, dest); > > + return true; > > +} > > + > > +static bool trans_amocas_w(DisasContext *ctx, arg_amocas_w *a) > > +{ > > + REQUIRE_ZACAS(ctx); > > + return gen_cmpxchg(ctx, a, MO_ALIGN | MO_TESL); > > +} > > + > > +static TCGv_i64 get_gpr_pair(DisasContext *ctx, int reg_num) > > +{ > > + TCGv_i64 t; > > + > > + assert(get_ol(ctx) == MXL_RV32); > > + > > + if (reg_num == 0) { > > + return tcg_constant_i64(0); > > + } > > + > > + t = tcg_temp_new_i64(); > > + tcg_gen_concat_tl_i64(t, cpu_gpr[reg_num], cpu_gpr[reg_num + > > 1]); > > + return t; > > +} > > + > > +static void gen_set_gpr_pair(DisasContext *ctx, int reg_num, > > TCGv_i64 t) > > +{ > > + assert(get_ol(ctx) == MXL_RV32); > > + > > + if (reg_num != 0) { > > +#ifdef TARGET_RISCV32 > > + tcg_gen_extr_i64_i32(cpu_gpr[reg_num], cpu_gpr[reg_num + > > 1], t); > > +#else > > + tcg_gen_ext32s_i64(cpu_gpr[reg_num], t); > > + tcg_gen_sari_i64(cpu_gpr[reg_num + 1], t, 32); > > +#endif > > This should be dynamic based on the MXL. > This function is already conditional on MXL = 32 as it us only called from trans_amocas_d() when get_ol() == MXL_RV32 (there is an assertion at the top to enforce this.) The compile time conditional code is required as the cpu_gpr type (TCGv) changes between TCGv_i32 or TCGv_i64 at build time - requiring different functions to be called to split the value and assign to registers. > > + > > + if (get_xl_max(ctx) == MXL_RV128) { > > + tcg_gen_sari_tl(cpu_gprh[reg_num], cpu_gpr[reg_num], > > 63); > > + tcg_gen_sari_tl(cpu_gprh[reg_num + 1], cpu_gpr[reg_num > > + 1], 63); > > + } > > + } > > +} > > + > > +static bool gen_cmpxchg64(DisasContext *ctx, arg_atomic *a, MemOp > > mop) > > +{ > > + /* > > + * Encodings with odd numbered registers specified in rs2 and > > rd are > > + * reserved. > > + */ > > + if ((a->rs2 | a->rd) & 1) { > > + return false; > > + } > > + > > + TCGv_i64 dest = get_gpr_pair(ctx, a->rd); > > + TCGv src1 = get_address(ctx, a->rs1, 0); > > + TCGv_i64 src2 = get_gpr_pair(ctx, a->rs2); > > + > > + decode_save_opc(ctx); > > + tcg_gen_atomic_cmpxchg_i64(dest, src1, dest, src2, ctx- > > >mem_idx, mop); > > + > > + gen_set_gpr_pair(ctx, a->rd, dest); > > + return true; > > +} > > + > > +static bool trans_amocas_d(DisasContext *ctx, arg_amocas_d *a) > > +{ > > + REQUIRE_ZACAS(ctx); > > + switch (get_ol(ctx)) { > > + case MXL_RV32: > > + return gen_cmpxchg64(ctx, a, MO_ALIGN | MO_TEUQ); > > + case MXL_RV64: > > + case MXL_RV128: > > + return gen_cmpxchg(ctx, a, MO_ALIGN | MO_TEUQ); > > + default: > > + g_assert_not_reached(); > > + } > > +} > > + > > +static bool trans_amocas_q(DisasContext *ctx, arg_amocas_q *a) > > +{ > > + REQUIRE_ZACAS(ctx); > > + REQUIRE_64BIT(ctx); > > + > > + /* > > + * Encodings with odd numbered registers specified in rs2 and > > rd are > > + * reserved. > > + */ > > + if ((a->rs2 | a->rd) & 1) { > > + return false; > > + } > > + > > +#ifdef TARGET_RISCV64 > > We already have a REQUIRE_64BIT(ctx), do we need this as well? > Unfortunately yes, because when compiling under TARGET_RISCV32 there is a type mismatch error from the get_gpr() method - similar to the requirement to have two paths in the gen_cmpxchg64() function: e.g. to concatenate the two TGV_i64 to a TGV_i128 the datatype from get_gpr() must be TGV_i64. The REQUIRE_6BIT() provides the necessary check at runtime to check that we are running with MXL = 64. Cheers, Rob > Alistair > > > + TCGv_i128 dest = tcg_temp_new_i128(); > > + TCGv src1 = get_address(ctx, a->rs1, 0); > > + TCGv_i128 src2 = tcg_temp_new_i128(); > > + TCGv_i64 src2l = get_gpr(ctx, a->rs2, EXT_NONE); > > + TCGv_i64 src2h = get_gpr(ctx, a->rs2 == 0 ? 0 : a->rs2 + 1, > > EXT_NONE); > > + TCGv_i64 destl = get_gpr(ctx, a->rd, EXT_NONE); > > + TCGv_i64 desth = get_gpr(ctx, a->rd == 0 ? 0 : a->rd + 1, > > EXT_NONE); > > + > > + tcg_gen_concat_i64_i128(src2, src2l, src2h); > > + tcg_gen_concat_i64_i128(dest, destl, desth); > > + decode_save_opc(ctx); > > + tcg_gen_atomic_cmpxchg_i128(dest, src1, dest, src2, ctx- > > >mem_idx, > > + (MO_ALIGN | MO_TEUO)); > > + > > + tcg_gen_extr_i128_i64(destl, desth, dest); > > + > > + if (a->rd != 0) { > > + gen_set_gpr(ctx, a->rd, destl); > > + gen_set_gpr(ctx, a->rd + 1, desth); > > + } > > +#endif > > + > > + return true; > > +} > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg- > > cpu.c > > index 8a35683a34..29b5a88931 100644 > > --- a/target/riscv/tcg/tcg-cpu.c > > +++ b/target/riscv/tcg/tcg-cpu.c > > @@ -343,6 +343,11 @@ void > > riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp) > > return; > > } > > > > + if ((cpu->cfg.ext_zacas) && !riscv_has_ext(env, RVA)) { > > + error_setg(errp, "Zacas extension requires A extension"); > > + return; > > + } > > + > > if ((cpu->cfg.ext_zawrs) && !riscv_has_ext(env, RVA)) { > > error_setg(errp, "Zawrs extension requires A extension"); > > return; > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > > index f0be79bb16..071fbad7ef 100644 > > --- a/target/riscv/translate.c > > +++ b/target/riscv/translate.c > > @@ -1089,6 +1089,7 @@ static uint32_t opcode_at(DisasContextBase > > *dcbase, target_ulong pc) > > #include "insn_trans/trans_rvv.c.inc" > > #include "insn_trans/trans_rvb.c.inc" > > #include "insn_trans/trans_rvzicond.c.inc" > > +#include "insn_trans/trans_rvzacas.c.inc" > > #include "insn_trans/trans_rvzawrs.c.inc" > > #include "insn_trans/trans_rvzicbo.c.inc" > > #include "insn_trans/trans_rvzfa.c.inc" > > -- > > 2.43.0 > > > >