[PATCH 0/2] Re: [PATCH 1/1] trans_rvi.c.inc: Make lq and sq use 128-bit ld/st

[PATCH 0/2] Re: [PATCH 1/1] trans_rvi.c.inc: Make lq and sq use 128-bit ld/st

[frederic.petrot--- via]

Hi Philippe,

On 12/26/25 17:06, Philippe Mathieu-Daudé wrote:
> Hi Frédéric,
> 
> On 16/12/25 22:24, Frédéric Pétrot wrote:
>> The lq and sq helpers for the experimental rv128 architecture currently
>> use direct (and erroneous) memory accesses.
>> Replace these direct accesses with the standard tcg_gen_qemu_{ld,st}_i128
>> TCG helpers that handle endianness issues.
>>
>> Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> 
> Errors when building qemu-system-riscv32:
> 
> [3/5] Compiling C object libqemu-riscv32-softmmu.a.p/target_riscv_translate.c.o
> In file included from ../../target/riscv/translate.c:1192:
> ../../target/riscv/insn_trans/trans_rvi.c.inc:395:35: error: incompatible 
> pointer types passing 'TCGv' (aka 'struct TCGv_i32_d *') to parameter of type 
> 'TCGv_i64' (aka 'struct TCGv_i64_d *') [-Werror,-Wincompatible-pointer-types]
>    395 |             tcg_gen_extr_i128_i64(destl, desth, t16);
>        |                                   ^~~~~
> include/tcg/tcg-op-common.h:309:37: note: passing argument to parameter 'lo' here
>    309 | void tcg_gen_extr_i128_i64(TCGv_i64 lo, TCGv_i64 hi, TCGv_i128 arg);
>        |                                     ^~
> ... 

Outch, sorry for that!

Since lq/sq are meaningful only for x-rv128 that is based on rv64, I've
handled this compilation issue in such a way that it compiles for rv32
and compiles and does what is expected for rv64 with '-cpu x-rv128'.
Using these 128-bit ld/st has the additional benefit of allowing the use
of the mttcg, as, if I understood correctly, they are atomic.

Frédéric Pétrot (2):
  trans_rvi.c.inc: Make lq and sq use 128-bit ld/st
  riscv/tcg/tcg-cpu.c: Remove smp check for x-rv128

 target/riscv/insn_trans/trans_rvi.c.inc | 32 ++++++++++++++++++-------
 target/riscv/tcg/tcg-cpu.c              | 10 --------
 2 files changed, 24 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH 1/2] trans_rvi.c.inc: Make lq and sq use 128-bit ld/st

[frederic.petrot--- via]

The lq and sq helpers for the experimental rv128 architecture currently
use direct memory accesses.
Replace these direct accesses with the standard tcg_gen_qemu_{ld,st}_i128
TCG helpers that handle endianness issues.

Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
---
 target/riscv/insn_trans/trans_rvi.c.inc | 32 ++++++++++++++++++-------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index 54b9b4f241..2c82ae41a7 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -377,6 +377,9 @@ static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
     TCGv destl = dest_gpr(ctx, a->rd);
     TCGv desth = dest_gprh(ctx, a->rd);
     TCGv addrl = tcg_temp_new();
+    TCGv_i128 t16 = tcg_temp_new_i128();
+    TCGv_i64 tl = tcg_temp_new_i64();
+    TCGv_i64 th = tcg_temp_new_i64();
 
     tcg_gen_addi_tl(addrl, src1l, a->imm);
 
@@ -388,10 +391,14 @@ static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
             tcg_gen_movi_tl(desth, 0);
         }
     } else {
-        /* assume little-endian memory access for now */
-        tcg_gen_qemu_ld_tl(destl, addrl, ctx->mem_idx, MO_TEUQ);
-        tcg_gen_addi_tl(addrl, addrl, 8);
-        tcg_gen_qemu_ld_tl(desth, addrl, ctx->mem_idx, MO_TEUQ);
+        tcg_gen_qemu_ld_i128(t16, addrl, ctx->mem_idx, memop);
+        if (mo_endian(ctx) == MO_LE) {
+            tcg_gen_extr_i128_i64(tl, th, t16);
+        } else {
+            tcg_gen_extr_i128_i64(th, tl, t16);
+        }
+        tcg_gen_trunc_i64_tl(destl, tl);
+        tcg_gen_trunc_i64_tl(desth, th);
     }
 
     gen_set_gpr128(ctx, a->rd, destl, desth);
@@ -488,16 +495,25 @@ static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
     TCGv src2l = get_gpr(ctx, a->rs2, EXT_NONE);
     TCGv src2h = get_gprh(ctx, a->rs2);
     TCGv addrl = tcg_temp_new();
+    TCGv_i128 t16 = tcg_temp_new_i128();
+    TCGv_i64 tl = tcg_temp_new_i64();
+    TCGv_i64 th = tcg_temp_new_i64();
 
     tcg_gen_addi_tl(addrl, src1l, a->imm);
 
     if ((memop & MO_SIZE) <= MO_64) {
         tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, memop);
     } else {
-        /* little-endian memory access assumed for now */
-        tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, MO_TEUQ);
-        tcg_gen_addi_tl(addrl, addrl, 8);
-        tcg_gen_qemu_st_tl(src2h, addrl, ctx->mem_idx, MO_TEUQ);
+
+        tcg_gen_ext_tl_i64(tl, src2l);
+        tcg_gen_ext_tl_i64(th, src2h);
+
+        if (mo_endian(ctx) == MO_LE) {
+            tcg_gen_concat_i64_i128(t16, tl, th);
+        } else {
+            tcg_gen_concat_i64_i128(t16, th, tl);
+        }
+        tcg_gen_qemu_st_i128(t16, addrl, ctx->mem_idx, memop);
     }
     return true;
 }
-- 
2.43.0

[PATCH 2/2] riscv/tcg/tcg-cpu.c: Remove mttcg check for x-rv128

[frederic.petrot--- via]

We had to check that mttcg was not used when executing QEMU with
-cpu x-rv128 as a single 128-bit access was done as two distinct 64-bit
accesses.
Now that we use the 128-bit ld/st that access the data atomically,
this check is no longer necessary.

Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
---
 target/riscv/tcg/tcg-cpu.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 440626ddfa..15d39f9912 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1305,16 +1305,6 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
     }
 
 #ifndef CONFIG_USER_ONLY
-    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
-
-    if (mcc->def->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
-        /* Missing 128-bit aligned atomics */
-        error_setg(errp,
-                   "128-bit RISC-V currently does not work with Multi "
-                   "Threaded TCG. Please use: -accel tcg,thread=single");
-        return false;
-    }
-
     CPURISCVState *env = &cpu->env;
 
     tcg_cflags_set(CPU(cs), CF_PCREL);
-- 
2.43.0

Re: [PATCH 2/2] riscv/tcg/tcg-cpu.c: Remove mttcg check for x-rv128

[Philippe Mathieu-Daudé]

On 1/1/26 19:14, frederic.petrot--- via wrote:
> We had to check that mttcg was not used when executing QEMU with
> -cpu x-rv128 as a single 128-bit access was done as two distinct 64-bit
> accesses.
> Now that we use the 128-bit ld/st that access the data atomically,
> this check is no longer necessary.
> 
> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> ---
>   target/riscv/tcg/tcg-cpu.c | 10 ----------
>   1 file changed, 10 deletions(-)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 440626ddfa..15d39f9912 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1305,16 +1305,6 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
>       }
>   
>   #ifndef CONFIG_USER_ONLY
> -    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> -
> -    if (mcc->def->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
> -        /* Missing 128-bit aligned atomics */
> -        error_setg(errp,
> -                   "128-bit RISC-V currently does not work with Multi "
> -                   "Threaded TCG. Please use: -accel tcg,thread=single");
> -        return false;
> -    }
> -
>       CPURISCVState *env = &cpu->env;
>   
>       tcg_cflags_set(CPU(cs), CF_PCREL);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] trans_rvi.c.inc: Make lq and sq use 128-bit ld/st

[Philippe Mathieu-Daudé]

On 1/1/26 19:14, frederic.petrot--- via wrote:
> The lq and sq helpers for the experimental rv128 architecture currently
> use direct memory accesses.
> Replace these direct accesses with the standard tcg_gen_qemu_{ld,st}_i128
> TCG helpers that handle endianness issues.
> 
> Reported-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>
> ---
>   target/riscv/insn_trans/trans_rvi.c.inc | 32 ++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index 54b9b4f241..2c82ae41a7 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -377,6 +377,9 @@ static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
>       TCGv destl = dest_gpr(ctx, a->rd);
>       TCGv desth = dest_gprh(ctx, a->rd);
>       TCGv addrl = tcg_temp_new();
> +    TCGv_i128 t16 = tcg_temp_new_i128();
> +    TCGv_i64 tl = tcg_temp_new_i64();
> +    TCGv_i64 th = tcg_temp_new_i64();
>   
>       tcg_gen_addi_tl(addrl, src1l, a->imm);
>   
> @@ -388,10 +391,14 @@ static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
>               tcg_gen_movi_tl(desth, 0);
>           }
>       } else {
> -        /* assume little-endian memory access for now */
> -        tcg_gen_qemu_ld_tl(destl, addrl, ctx->mem_idx, MO_TEUQ);
> -        tcg_gen_addi_tl(addrl, addrl, 8);
> -        tcg_gen_qemu_ld_tl(desth, addrl, ctx->mem_idx, MO_TEUQ);
> +        tcg_gen_qemu_ld_i128(t16, addrl, ctx->mem_idx, memop);
> +        if (mo_endian(ctx) == MO_LE) {
> +            tcg_gen_extr_i128_i64(tl, th, t16);
> +        } else {
> +            tcg_gen_extr_i128_i64(th, tl, t16);
> +        }
> +        tcg_gen_trunc_i64_tl(destl, tl);
> +        tcg_gen_trunc_i64_tl(desth, th);

I'd have rathered avoid the temps / truncate, but I suppose
we are restricted by the GPR being declared as target_ulong,
so this code wouldn't build for rv32 althought it isn't
reachable, and your code is almost a simple copy on rv64.

   bool le = mo_endian(ctx) == MO_LE;
   TCGv destl = (le ? dest_gpr : dest_gprh)(ctx, a->rd);
   TCGv desth = (le ? dest_gprh : dest_gpr)(ctx, a->rd);
   ...
   tcg_gen_extr_i128_i64(destl, desth, t16);

>       }
>   
>       gen_set_gpr128(ctx, a->rd, destl, desth);
> @@ -488,16 +495,25 @@ static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
>       TCGv src2l = get_gpr(ctx, a->rs2, EXT_NONE);
>       TCGv src2h = get_gprh(ctx, a->rs2);
>       TCGv addrl = tcg_temp_new();
> +    TCGv_i128 t16 = tcg_temp_new_i128();
> +    TCGv_i64 tl = tcg_temp_new_i64();
> +    TCGv_i64 th = tcg_temp_new_i64();
>   
>       tcg_gen_addi_tl(addrl, src1l, a->imm);
>   
>       if ((memop & MO_SIZE) <= MO_64) {
>           tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, memop);
>       } else {
> -        /* little-endian memory access assumed for now */
> -        tcg_gen_qemu_st_tl(src2l, addrl, ctx->mem_idx, MO_TEUQ);
> -        tcg_gen_addi_tl(addrl, addrl, 8);
> -        tcg_gen_qemu_st_tl(src2h, addrl, ctx->mem_idx, MO_TEUQ);
> +
> +        tcg_gen_ext_tl_i64(tl, src2l);
> +        tcg_gen_ext_tl_i64(th, src2h);
> +
> +        if (mo_endian(ctx) == MO_LE) {
> +            tcg_gen_concat_i64_i128(t16, tl, th);

(Ditto)

> +        } else {
> +            tcg_gen_concat_i64_i128(t16, th, tl);
> +        }
> +        tcg_gen_qemu_st_i128(t16, addrl, ctx->mem_idx, memop);
>       }
>       return true;
>   }

This approach is good enough for me for now:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!