Re: [PATCH] Add RISCV Zilsd extension

[Roan Richmond <roan.richmond@codethink.co.uk>]

I understand your point about doing the check before the 2 ld_tl() and 
the 2 dst_gpr() calls.

My reasoning for doing the check after was the wording of the specification:
"LD instructions with destination x0 are processed as any other load, 
but the result is discarded entirely and x1 is not written"
This suggests that a load instruction is still dispatched but the result 
is then discarded.

I am happy to move the check if you think it is definitely necessary, 
otherwise I would prefer to leave it as happening after the calls to 
ld_tl() and dst_gpr, so we are closer the the expected behavior.

Thanks,
Roan

On 06/11/2025 12:17, Daniel Henrique Barboza wrote:
>
>
> On 11/4/25 8:59 AM, Roan Richmond wrote:
>> This is based on v1.0 of the Zilsd extension [1].
>> The specification is listed as in the Ratified state [2],
>>   since Jan 30, 2025.
>>
>> [1]: https://github.com/riscv/riscv-zilsd
>> [2]: 
>> https://riscv.atlassian.net/wiki/spaces/HOME/pages/16154861/RISC-V+Specs+Under+Development
>>
>> Signed-off-by: Roan Richmond <roan.richmond@codethink.co.uk>
>> ---
>>   target/riscv/cpu.c                      |  2 +
>>   target/riscv/cpu_cfg_fields.h.inc       |  1 +
>>   target/riscv/insn_trans/trans_rvi.c.inc | 56 ++++++++++++++++++++++++-
>>   3 files changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 73d4280d7c..6e0d37fb96 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -121,6 +121,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>       ISA_EXT_DATA_ENTRY(zihintntl, PRIV_VERSION_1_10_0, ext_zihintntl),
>>       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(zilsd, PRIV_VERSION_1_13_0, ext_zilsd),
>>       ISA_EXT_DATA_ENTRY(zimop, PRIV_VERSION_1_13_0, ext_zimop),
>>       ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
>>       ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, has_priv_1_12),
>> @@ -1247,6 +1248,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("zilsd", ext_zilsd, false),
>>       MULTI_EXT_CFG_BOOL("zimop", ext_zimop, false),
>>       MULTI_EXT_CFG_BOOL("zcmop", ext_zcmop, false),
>>       MULTI_EXT_CFG_BOOL("zacas", ext_zacas, false),
>> diff --git a/target/riscv/cpu_cfg_fields.h.inc 
>> b/target/riscv/cpu_cfg_fields.h.inc
>> index a154ecdc79..8d8e35e4cf 100644
>> --- a/target/riscv/cpu_cfg_fields.h.inc
>> +++ b/target/riscv/cpu_cfg_fields.h.inc
>> @@ -41,6 +41,7 @@ BOOL_FIELD(ext_zicond)
>>   BOOL_FIELD(ext_zihintntl)
>>   BOOL_FIELD(ext_zihintpause)
>>   BOOL_FIELD(ext_zihpm)
>> +BOOL_FIELD(ext_zilsd)
>>   BOOL_FIELD(ext_zimop)
>>   BOOL_FIELD(ext_zcmop)
>>   BOOL_FIELD(ext_ztso)
>> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc 
>> b/target/riscv/insn_trans/trans_rvi.c.inc
>> index 54b9b4f241..dcbb3811d9 100644
>> --- a/target/riscv/insn_trans/trans_rvi.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
>> @@ -370,6 +370,27 @@ static bool gen_load_tl(DisasContext *ctx, 
>> arg_lb *a, MemOp memop)
>>       return true;
>>   }
>>   +/* Zilsd extension adds load/store double for 32bit arch */
>> +static bool gen_load_zilsd(DisasContext *ctx, arg_lb *a)
>> +{
>> +    TCGv dest_1 = dest_gpr(ctx, a->rd);
>> +    TCGv dest_2 = dest_gpr(ctx, (a->rd)+1);
>> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
>> +    TCGv addr_2 = get_address(ctx, a->rs1, (a->imm)+4);
>> +
>> +    tcg_gen_qemu_ld_tl(dest_1, addr_1, ctx->mem_idx, MO_SL);
>> +    tcg_gen_qemu_ld_tl(dest_2, addr_2, ctx->mem_idx, MO_SL);
>> +
>> +    /* If destination is x0 then result of the load is discarded */
>> +    if (a->rd == 0) {
>> +        return true;
>> +    }
>
> Can't we exit earlier in this case? We're doing 2 dst_gpr() calls, 2 
> get_address() and
> 2 ld_tl() loads before checking a->rd == 0. Maybe something like this:
>
>
>> +    TCGv addr_1, addr_2, dest_1, dest_2;
>> +
>> +    /* If destination is x0 then result of the load is discarded */
>> +    if (a->rd == 0) {
>> +        return true;
>> +    }
>> +
>> +    dest_1 = dest_gpr(ctx, a->rd);
>> +    dest_2 = dest_gpr(ctx, (a->rd)+1);
>> +    addr_1 = get_address(ctx, a->rs1, a->imm);
>> +    addr_2 = get_address(ctx, a->rs1, (a->imm)+4);
>> +
>> +    tcg_gen_qemu_ld_tl(dest_1, addr_1, ctx->mem_idx, MO_SL);
>> +    tcg_gen_qemu_ld_tl(dest_2, addr_2, ctx->mem_idx, MO_SL);
>
>
> Thanks,
>
> Daniel
>
>
>
>> +
>> +    gen_set_gpr(ctx, a->rd, dest_1);
>> +    gen_set_gpr(ctx, a->rd+1, dest_2);
>> +    return true;
>> +}
>> +
>>   /* Compute only 64-bit addresses to use the address translation 
>> mechanism */
>>   static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
>>   {
>> @@ -409,6 +430,8 @@ static bool gen_load(DisasContext *ctx, arg_lb 
>> *a, MemOp memop)
>>       decode_save_opc(ctx, 0);
>>       if (get_xl(ctx) == MXL_RV128) {
>>           out = gen_load_i128(ctx, a, memop);
>> +    } else if (memop == MO_SQ && get_xl(ctx) == MXL_RV32) {
>> +        out = gen_load_zilsd(ctx, a);
>>       } else {
>>           out = gen_load_tl(ctx, a, memop);
>>       }
>> @@ -437,7 +460,10 @@ static bool trans_lw(DisasContext *ctx, arg_lw *a)
>>     static bool trans_ld(DisasContext *ctx, arg_ld *a)
>>   {
>> -    REQUIRE_64_OR_128BIT(ctx);
>> +    /* Check for Zilsd extension if 32bit */
>> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
>> +        return false;
>> +    }
>>       return gen_load(ctx, a, MO_SQ);
>>   }
>>   @@ -482,6 +508,27 @@ static bool gen_store_tl(DisasContext *ctx, 
>> arg_sb *a, MemOp memop)
>>       return true;
>>   }
>>   +/* Zilsd extension adds load/store double for 32bit arch */
>> +static bool gen_store_zilsd(DisasContext *ctx, arg_sb *a)
>> +{
>> +    TCGv data_1 = tcg_temp_new();
>> +    TCGv data_2 = tcg_temp_new();
>> +    if (a->rs2 != 0) {
>> +        data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
>> +        data_2 = get_gpr(ctx, (a->rs2)+1, EXT_NONE);
>> +    }
>> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
>> +    TCGv addr_2 = get_address(ctx, a->rs1, (a->imm)+4);
>> +
>> +    if (ctx->ztso) {
>> +        tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
>> +    }
>> +
>> +    tcg_gen_qemu_st_tl(data_1, addr_1, ctx->mem_idx, MO_SL);
>> +    tcg_gen_qemu_st_tl(data_2, addr_2, ctx->mem_idx, MO_SL);
>> +    return true;
>> +}
>> +
>>   static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
>>   {
>>       TCGv src1l = get_gpr(ctx, a->rs1, EXT_NONE);
>> @@ -511,6 +558,8 @@ static bool gen_store(DisasContext *ctx, arg_sb 
>> *a, MemOp memop)
>>       decode_save_opc(ctx, 0);
>>       if (get_xl(ctx) == MXL_RV128) {
>>           return gen_store_i128(ctx, a, memop);
>> +    } else if (memop == MO_UQ && get_xl(ctx) == MXL_RV32) {
>> +        return gen_store_zilsd(ctx, a);
>>       } else {
>>           return gen_store_tl(ctx, a, memop);
>>       }
>> @@ -533,7 +582,10 @@ static bool trans_sw(DisasContext *ctx, arg_sw *a)
>>     static bool trans_sd(DisasContext *ctx, arg_sd *a)
>>   {
>> -    REQUIRE_64_OR_128BIT(ctx);
>> +    /* Check for Zilsd extension if 32bit */
>> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
>> +        return false;
>> +    }
>>       return gen_store(ctx, a, MO_UQ);
>>   }
>
>
-- 
Roan Richmond (he/him)
Software Engineer
Codethink Ltd