Re: [PATCH v2] Add RISCV Zilsd extension

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Roan,

On 13/11/25 09:53, Roan Richmond wrote:
> On 12/11/2025 01:41, Alistair Francis wrote:
>> On Mon, Nov 10, 2025 at 7:05 PM Roan Richmond
>> <roan.richmond@codethink.co.uk> 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
>>>
>>> Reviewed-by: Daniel Henrique barboza <dbarboza@ventanamicro.com>
>>> Reviewed-by: Richard Henderson  <richard.henderson@linaro.org>
>>> Signed-off-by: Roan Richmond <roan.richmond@codethink.co.uk>
>>> ---
>>> v2:
>>>    - In `gen_load_zilsd` swapped `dest_gpr` with `tcg_temp_new`, this 
>>> prevents
>>>      writing to `r1/r0` in the case where `r0` is the set as `rd`.
>>>    - Removed unneeded brackets around `a->rd` + c expressions
>>>
>>>   target/riscv/cpu.c                      |  2 +
>>>   target/riscv/cpu_cfg_fields.h.inc       |  1 +
>>>   target/riscv/insn_trans/trans_rvi.c.inc | 57 ++++++++++++++++++++++++-
>>>   3 files changed, 58 insertions(+), 2 deletions(-)


>>> @@ -482,6 +509,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);
>>> +    }
>> Don't mix code and declarations, otherwise
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
> Struggling to see what you mean by this.
> 
> Could you provide a bit more clarification on the changes you would like 
> me to make and then I'll send out a v3.
> 
> Thanks,
> Roan
> 
>>
>>> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);

The declaration part is:

    TCGv addr_1;

and the code part is:

    addr_1 = get_address(ctx, a->rs1, a->imm);

Per our coding style (docs/devel/style.rst) chapter "Declarations":

    Mixed declarations (interleaving statements and declarations
    within blocks) are generally not allowed; declarations should
    be at the beginning of blocks.

Declarations should go in [*].

For v3 you should address Richard's comments.

Regards,

Phil.

>>> +    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 +559,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 +583,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);
>>>   }
>>>
>>> -- 
>>> 2.43.0
>>>