From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Christoph Muellner <christoph.muellner@vrull.eu>,
Palmer Dabbelt <palmer@dabbelt.com>,
Alistair Francis <alistair.francis@wdc.com>,
Liu Zhiwei <zhiwei_liu@linux.alibaba.com>,
Anton Johansson <anjo@rev.ng>,
Richard Henderson <richard.henderson@linaro.org>,
Valentin Haudiquet <valentin.haudiquet@canonical.com>,
Weiwei Li <liwei1518@gmail.com>,
qemu-riscv@nongnu.org,
Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
qemu-devel@nongnu.org
Subject: Re: [PATCH 11/13] target/riscv: Factor MemOp variable out when MO_TE is set
Date: Fri, 10 Oct 2025 18:18:36 +0200 [thread overview]
Message-ID: <b455449b-8ac4-4547-90db-e5b93b820772@canonical.com> (raw)
In-Reply-To: <20251010155045.78220-12-philmd@linaro.org>
On 10/10/25 17:50, Philippe Mathieu-Daudé wrote:
> In preparation of automatically replacing the MO_TE flag
> in the next commit, use an local @memop variable.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> target/riscv/insn_trans/trans_rvd.c.inc | 6 ++++--
> target/riscv/insn_trans/trans_rvf.c.inc | 6 ++++--
> target/riscv/insn_trans/trans_rvzacas.c.inc | 5 +++--
> target/riscv/insn_trans/trans_rvzce.c.inc | 6 ++++--
> target/riscv/insn_trans/trans_rvzfh.c.inc | 8 ++++++--
> target/riscv/insn_trans/trans_rvzicfiss.c.inc | 10 ++++++----
> 6 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
> index 33858206788..62b75358158 100644
> --- a/target/riscv/insn_trans/trans_rvd.c.inc
> +++ b/target/riscv/insn_trans/trans_rvd.c.inc
> @@ -42,7 +42,7 @@
> static bool trans_fld(DisasContext *ctx, arg_fld *a)
> {
> TCGv addr;
> - MemOp memop = MO_TE | MO_UQ;
> + MemOp memop = MO_UQ;
Thank you for the series which makes the introduction of big-endian
support easier. I really like how you have split it up into logical steps.
The change in trans_fld() looks correct but is not fully covered by the
commit description message: This function already uses a local memop
variable.
Here you are factoring out setting the endian bit which makes
'mechanically' replacing the setting of the bit easier.
Consider updating the commit message.
Otherwise the patch looks good to me.
Best regards
Heinrich
>
> REQUIRE_FPU;
> REQUIRE_EXT(ctx, RVD);
> @@ -60,6 +60,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
> } else {
> memop |= MO_ATOM_IFALIGN;
> }
> + memop |= MO_TE;
>
> decode_save_opc(ctx, 0);
> addr = get_address(ctx, a->rs1, a->imm);
> @@ -72,7 +73,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
> static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
> {
> TCGv addr;
> - MemOp memop = MO_TE | MO_UQ;
> + MemOp memop = MO_UQ;
>
> REQUIRE_FPU;
> REQUIRE_EXT(ctx, RVD);
> @@ -84,6 +85,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
> } else {
> memop |= MO_ATOM_IFALIGN;
> }
> + memop |= MO_TE;
>
> decode_save_opc(ctx, 0);
> addr = get_address(ctx, a->rs1, a->imm);
> diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
> index 150e2b9a7d4..878417eae92 100644
> --- a/target/riscv/insn_trans/trans_rvf.c.inc
> +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> @@ -43,11 +43,12 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
> {
> TCGv_i64 dest;
> TCGv addr;
> - MemOp memop = MO_TE | MO_UL;
> + MemOp memop = MO_UL;
>
> REQUIRE_FPU;
> REQUIRE_EXT(ctx, RVF);
>
> + memop |= MO_TE;
> if (ctx->cfg_ptr->ext_zama16b) {
> memop |= MO_ATOM_WITHIN16;
> }
> @@ -65,11 +66,12 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
> static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
> {
> TCGv addr;
> - MemOp memop = MO_TE | MO_UL;
> + MemOp memop = MO_UL;
>
> REQUIRE_FPU;
> REQUIRE_EXT(ctx, RVF);
>
> + memop |= MO_TE;
> if (ctx->cfg_ptr->ext_zama16b) {
> memop |= MO_ATOM_WITHIN16;
> }
> diff --git a/target/riscv/insn_trans/trans_rvzacas.c.inc b/target/riscv/insn_trans/trans_rvzacas.c.inc
> index d850b142642..6458ac4f241 100644
> --- a/target/riscv/insn_trans/trans_rvzacas.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzacas.c.inc
> @@ -119,12 +119,13 @@ static bool trans_amocas_q(DisasContext *ctx, arg_amocas_q *a)
> 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);
> + MemOp memop = MO_ALIGN | MO_UO;
>
> + memop |= MO_TE;
> tcg_gen_concat_i64_i128(src2, src2l, src2h);
> tcg_gen_concat_i64_i128(dest, destl, desth);
> decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> - tcg_gen_atomic_cmpxchg_i128(dest, src1, dest, src2, ctx->mem_idx,
> - (MO_ALIGN | MO_TE | MO_UO));
> + tcg_gen_atomic_cmpxchg_i128(dest, src1, dest, src2, ctx->mem_idx, memop);
>
> tcg_gen_extr_i128_i64(destl, desth, dest);
>
> diff --git a/target/riscv/insn_trans/trans_rvzce.c.inc b/target/riscv/insn_trans/trans_rvzce.c.inc
> index c8dc102c8e3..172c2c19c17 100644
> --- a/target/riscv/insn_trans/trans_rvzce.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzce.c.inc
> @@ -175,7 +175,7 @@ static bool gen_pop(DisasContext *ctx, arg_cmpp *a, bool ret, bool ret_val)
> return false;
> }
>
> - MemOp memop = get_ol(ctx) == MXL_RV32 ? MO_TE | MO_UL : MO_TE | MO_UQ;
> + MemOp memop = get_ol(ctx) == MXL_RV32 ? MO_UL : MO_UQ;
> int reg_size = memop_size(memop);
> target_ulong stack_adj = ROUND_UP(ctpop32(reg_bitmap) * reg_size, 16) +
> a->spimm;
> @@ -185,6 +185,7 @@ static bool gen_pop(DisasContext *ctx, arg_cmpp *a, bool ret, bool ret_val)
>
> tcg_gen_addi_tl(addr, sp, stack_adj - reg_size);
>
> + memop |= MO_TE;
> for (i = X_Sn + 11; i >= 0; i--) {
> if (reg_bitmap & (1 << i)) {
> TCGv dest = dest_gpr(ctx, i);
> @@ -228,7 +229,7 @@ static bool trans_cm_push(DisasContext *ctx, arg_cm_push *a)
> return false;
> }
>
> - MemOp memop = get_ol(ctx) == MXL_RV32 ? MO_TE | MO_UL : MO_TE | MO_UQ;
> + MemOp memop = get_ol(ctx) == MXL_RV32 ? MO_UL : MO_UQ;
> int reg_size = memop_size(memop);
> target_ulong stack_adj = ROUND_UP(ctpop32(reg_bitmap) * reg_size, 16) +
> a->spimm;
> @@ -238,6 +239,7 @@ static bool trans_cm_push(DisasContext *ctx, arg_cm_push *a)
>
> tcg_gen_subi_tl(addr, sp, reg_size);
>
> + memop |= MO_TE;
> for (i = X_Sn + 11; i >= 0; i--) {
> if (reg_bitmap & (1 << i)) {
> TCGv val = get_gpr(ctx, i, EXT_NONE);
> diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
> index eec478afcb0..5355cd46c3d 100644
> --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> @@ -42,12 +42,14 @@
>
> static bool trans_flh(DisasContext *ctx, arg_flh *a)
> {
> + MemOp memop = MO_UW;
> TCGv_i64 dest;
> TCGv t0;
>
> REQUIRE_FPU;
> REQUIRE_ZFHMIN_OR_ZFBFMIN(ctx);
>
> + memop |= MO_TE;
> decode_save_opc(ctx, 0);
> t0 = get_gpr(ctx, a->rs1, EXT_NONE);
> if (a->imm) {
> @@ -57,7 +59,7 @@ static bool trans_flh(DisasContext *ctx, arg_flh *a)
> }
>
> dest = cpu_fpr[a->rd];
> - tcg_gen_qemu_ld_i64(dest, t0, ctx->mem_idx, MO_TE | MO_UW);
> + tcg_gen_qemu_ld_i64(dest, t0, ctx->mem_idx, memop);
> gen_nanbox_h(dest, dest);
>
> mark_fs_dirty(ctx);
> @@ -66,11 +68,13 @@ static bool trans_flh(DisasContext *ctx, arg_flh *a)
>
> static bool trans_fsh(DisasContext *ctx, arg_fsh *a)
> {
> + MemOp memop = MO_UW;
> TCGv t0;
>
> REQUIRE_FPU;
> REQUIRE_ZFHMIN_OR_ZFBFMIN(ctx);
>
> + memop |= MO_TE;
> decode_save_opc(ctx, 0);
> t0 = get_gpr(ctx, a->rs1, EXT_NONE);
> if (a->imm) {
> @@ -79,7 +83,7 @@ static bool trans_fsh(DisasContext *ctx, arg_fsh *a)
> t0 = temp;
> }
>
> - tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], t0, ctx->mem_idx, MO_TE | MO_UW);
> + tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], t0, ctx->mem_idx, memop);
>
> return true;
> }
> diff --git a/target/riscv/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
> index c5555966175..89eed007587 100644
> --- a/target/riscv/insn_trans/trans_rvzicfiss.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzicfiss.c.inc
> @@ -100,12 +100,13 @@ static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a)
>
> TCGv dest = dest_gpr(ctx, a->rd);
> TCGv src1, src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> + MemOp memop = MO_ALIGN | MO_SL;
>
> decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> src1 = get_address(ctx, a->rs1, 0);
>
> - tcg_gen_atomic_xchg_tl(dest, src1, src2, SS_MMU_INDEX(ctx),
> - (MO_ALIGN | MO_TE | MO_SL));
> + memop |= MO_TE;
> + tcg_gen_atomic_xchg_tl(dest, src1, src2, SS_MMU_INDEX(ctx), memop);
> gen_set_gpr(ctx, a->rd, dest);
> return true;
> }
> @@ -129,12 +130,13 @@ static bool trans_ssamoswap_d(DisasContext *ctx, arg_amoswap_w *a)
>
> TCGv dest = dest_gpr(ctx, a->rd);
> TCGv src1, src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> + MemOp memop = MO_ALIGN | MO_SQ;
>
> decode_save_opc(ctx, RISCV_UW2_ALWAYS_STORE_AMO);
> src1 = get_address(ctx, a->rs1, 0);
>
> - tcg_gen_atomic_xchg_tl(dest, src1, src2, SS_MMU_INDEX(ctx),
> - (MO_ALIGN | MO_TE | MO_SQ));
> + memop |= MO_TE;
> + tcg_gen_atomic_xchg_tl(dest, src1, src2, SS_MMU_INDEX(ctx), memop);
> gen_set_gpr(ctx, a->rd, dest);
> return true;
> }
next prev parent reply other threads:[~2025-10-10 16:20 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-10 15:50 [PATCH 00/13] target/riscv: Centralize MO_TE uses in a pair of helpers Philippe Mathieu-Daudé
2025-10-10 15:50 ` [PATCH 01/13] target/riscv: Really use little endianness for 128-bit loads/stores Philippe Mathieu-Daudé
2025-10-10 18:44 ` Richard Henderson
2025-10-10 15:50 ` [PATCH 02/13] target/riscv: Explode MO_TExx -> MO_TE | MO_xx Philippe Mathieu-Daudé
2025-10-10 18:45 ` Richard Henderson
2025-10-14 4:59 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 03/13] target/riscv: Conceal MO_TE within gen_amo() Philippe Mathieu-Daudé
2025-10-10 18:46 ` Richard Henderson
2025-10-14 5:00 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 04/13] target/riscv: Conceal MO_TE within gen_inc() Philippe Mathieu-Daudé
2025-10-10 18:47 ` Richard Henderson
2025-10-14 5:01 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 05/13] target/riscv: Conceal MO_TE within gen_load() / gen_store() Philippe Mathieu-Daudé
2025-10-10 18:47 ` Richard Henderson
2025-10-14 5:02 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 06/13] target/riscv: Conceal MO_TE within gen_load_idx() / gen_store_idx() Philippe Mathieu-Daudé
2025-10-10 18:48 ` Richard Henderson
2025-10-14 5:03 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 07/13] target/riscv: Conceal MO_TE within gen_fload_idx() / gen_fstore_idx() Philippe Mathieu-Daudé
2025-10-10 18:49 ` Richard Henderson
2025-10-14 5:05 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 08/13] target/riscv: Conceal MO_TE within gen_storepair_tl() Philippe Mathieu-Daudé
2025-10-10 18:49 ` Richard Henderson
2025-10-14 5:06 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 09/13] target/riscv: Conceal MO_TE within gen_cmpxchg*() Philippe Mathieu-Daudé
2025-10-10 18:50 ` Richard Henderson
2025-10-14 5:07 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 10/13] target/riscv: Conceal MO_TE|MO_ALIGN within gen_lr() / gen_sc() Philippe Mathieu-Daudé
2025-10-10 18:51 ` Richard Henderson
2025-10-14 5:08 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 11/13] target/riscv: Factor MemOp variable out when MO_TE is set Philippe Mathieu-Daudé
2025-10-10 16:18 ` Heinrich Schuchardt [this message]
2025-10-14 5:11 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 12/13] target/riscv: Introduce mo_endian() helper Philippe Mathieu-Daudé
2025-10-10 16:35 ` Heinrich Schuchardt
2025-10-10 18:52 ` Richard Henderson
2025-10-14 5:13 ` Alistair Francis
2025-10-10 15:50 ` [PATCH 13/13] target/riscv: Introduce mo_endian_env() helper Philippe Mathieu-Daudé
2025-10-10 16:38 ` Heinrich Schuchardt
2025-10-14 5:15 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b455449b-8ac4-4547-90db-e5b93b820772@canonical.com \
--to=heinrich.schuchardt@canonical.com \
--cc=alistair.francis@wdc.com \
--cc=anjo@rev.ng \
--cc=christoph.muellner@vrull.eu \
--cc=dbarboza@ventanamicro.com \
--cc=liwei1518@gmail.com \
--cc=palmer@dabbelt.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=valentin.haudiquet@canonical.com \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).