Re: [PATCH v2 06/18] target/riscv: Add packed SIMD shift instructions

on 2026/8/20 0:10, Max Chou wrote:

> On 2026-07-17 10:06, Molly Chen wrote:
>> Signed-off-by: Molly Chen<[email protected]>
>> ---
>>   target/riscv/helper.h                       |  43 ++++++
>>   target/riscv/insn32.decode                  |  57 ++++++++
>>   target/riscv/tcg/insn_trans/trans_rvp.c.inc |  43 ++++++
>>   target/riscv/tcg/psimd_helper.c             | 149 ++++++++++++++++++++
>>   4 files changed, 292 insertions(+)
> ...
>> diff --git a/target/riscv/tcg/psimd_helper.c b/target/riscv/tcg/psimd_helper.c
>> index 034afe5a054..8b106a336f5 100644
>> --- a/target/riscv/tcg/psimd_helper.c
>> +++ b/target/riscv/tcg/psimd_helper.c
>> @@ -1903,3 +1903,152 @@ GEN_PSIMD_BINOP(mslt, uint32_t, int32_t, uint32_t,
>>   GEN_PSIMD_BINOP(msltu, uint32_t, uint32_t, uint32_t,
>>                   EXTRACT32, INSERT32, ELEMS_W, PSIMD_DO_LT_MASK)
>>   
>> +/* Shift operations (immediate and register) */
>> +
>> +GEN_PSIMD_SHIFTOP(pslli_b, target_ulong, uint8_t, uint8_t,
>> +                  EXTRACT8, INSERT8, ELEMS_B, 0x07, PSIMD_DO_SLL)
>> +GEN_PSIMD_SHIFTOP(psll_bs, target_ulong, uint8_t, uint8_t,
>> +                  EXTRACT8, INSERT8, ELEMS_B, 0x07, PSIMD_DO_SLL)
> The spec instead specifies a uniform 5-bit mask (shamt = X[rs2][4:0]),
> regardless of element width.
> So the SHMASK should be 0x1f for PSLL/PSRL/PSRA.[BS|HS] instructions.
>
> ...

|You are right. The byte and halfword shift forms should both use the 
low 5 bits of rs2. I will change their SHMASK arguments to 0x1f in the 
next revision.|

>> +
>> +/**
>> + * SSHAR - 32-bit scalar variable shift with rounding and saturation
>> + */
>> +uint32_t HELPER(sshar)(CPURISCVState *env, uint32_t rs1, uint32_t rs2)
>> +{
>> +    int32_t a = (int32_t)rs1;
>> +    int8_t shamt = (int8_t)(rs2 & 0xFF);
>> +    int sat = 0;
>> +    int32_t res;
>> +
>> +    if (shamt >= 0) {
>> +        int64_t shifted = (int64_t)a << shamt;
>> +        res = signed_saturate_w(shifted, &sat);
>> +    } else {
>> +        int right = -shamt;
>> +        if (right >= 32) {
>> +            res = (a < 0) ? -1 : 0;
> According to the p ext isa spec, SSHAR here extracts a sign-filled
> 33-bit value and then applies RNU rounding. The result here should be
> 0 for both positive and negative inputs.

Yes, I will fix it.

>
>> +        } else {
>> +            int64_t rounded = ((a >> (right - 1)) + 1) >> 1;
> Here may has undefined behavior for negative sources at in-range
> amounts.

|Right-shifting a negative signed integer is implementation-defined, 
rather than undefined. QEMU already relies on the supported compilers 
performing an arithmetic right shift in this case. However, the 
expression does contain a real undefined-behavior case for a positive 
source: when e1 is INT32_MAX and right is 1, the addition of one 
overflows before the result is assigned to WTYPE. I will cast e1 to 
WTYPE before the first shift so that the shift and rounding addition are 
both evaluated in the wider type.|

>
> I think that we could extract the SSHAR implementation from
> GEN_PSIMD_VAR_SSHAR and share it between here and
> GEN_PSIMD_VAR_SSHAR.

|Good suggestion. I will extract the common SSHAR operation into a 
shared helper and use it for both implementations in the next revision.|
||
|Thanks for the review.|
Molly