Re: [PATCH v2 02/18] target/riscv: Add packed SIMD helper framework

|Hi Max, ||You're right. For a rounding right shift at or beyond the element 
width, the shift amount should be clamped to the element width so that 
the result is determined by rounding the element's MSB. Returning zero 
unconditionally when `right > BITS` is incorrect when the MSB is set.||I will use the suggested `MIN()` approach in the next version. Thanks, 
Molly|

在 2026/8/16 23:49, Max Chou 写道:
> On 2026-07-17 10:06, Molly Chen wrote:
>> Signed-off-by: Molly Chen<[email protected]>
>> ---
>>   target/riscv/tcg/meson.build    |    3 +-
>>   target/riscv/tcg/psimd_helper.c | 1656 +++++++++++++++++++++++++++++++
>>   2 files changed, 1658 insertions(+), 1 deletion(-)
>>   create mode 100644 target/riscv/tcg/psimd_helper.c
>>
> ...
>>   riscv_system_ss.add(files(
>> diff --git a/target/riscv/tcg/psimd_helper.c b/target/riscv/tcg/psimd_helper.c
>> new file mode 100644
>> index 00000000000..2948bbb2a86
>> --- /dev/null
>> +++ b/target/riscv/tcg/psimd_helper.c
> ...
>> +#define GEN_PSIMD_VAR_USHLR(NAME, RTYPE, ETYPE, WTYPE, EXTRACT, INSERT,   \
>> +                            ELEMS, BITS, SAT_FN)                          \
>> +RTYPE HELPER(NAME)(CPURISCVState *env, RTYPE rs1, RTYPE rs2)              \
>> +{                                                                         \
>> +    RTYPE rd = 0;                                                         \
>> +    int elems = ELEMS(rd);                                                \
>> +    int sat = 0;                                                          \
>> +    int8_t shamt = (int8_t)(rs2 & 0xff);                                  \
>> +                                                                          \
>> +    for (int i = 0; i < elems; i++) {                                     \
>> +        ETYPE e1 = (ETYPE)EXTRACT(rs1, i);                                \
>> +        ETYPE res;                                                        \
>> +                                                                          \
>> +        if (shamt >= 0) {                                                 \
>> +            WTYPE shifted = (shamt >= (BITS)) ?                           \
>> +                            ((WTYPE)e1 << (BITS)) :                       \
>> +                            ((WTYPE)e1 << shamt);                         \
>> +            res = SAT_FN(shifted, &sat);                                  \
>> +        } else {                                                          \
>> +            int right = -shamt;                                           \
>> +            if (right > (BITS)) {                                         \
>> +                res = 0;                                                  \
> Here may has an issue that a rounding right shift past the element
> width does not degenerate to zero -- it saturates to the rounding of
> the element's MSB.
>
> According to the P ext isa spec:
>
>    "The SSHLR instruction performs an unsigned variable shift of `rs1`
>     using the signed shift amount in `rs2[7:0]`. Right shifts are
>     rounded, and left shifts saturate to the signed 32-bit range."
>
>      if sshamt < 0:
>          // arithmetic right shift with rounding
>          x  = zero_extend(64, s1) @ 0b0            // 65-bit
>          y  = (sshamt <= -32) ? x[64:32]
>                               : (x >> (0 - shamt)[4:0])[32:0]
>          X[rd] = (y + 1)[32:1]
>
> The operation Sail code shows that every right shift at or beyond
> the element width collapses to the same x[64:32], which is the
> element's MSB, and then still gets the (y + 1) >> 1 rounding.
> PSSHLR.HS and PSSHLR.WS have the identical construct at the 16- and
> 32-bit boundaries.
>
> Maybe we could fix it liked:
>
>             } else {                                                          \
>                 int right = MIN(-shamt, (BITS));                              \
>                 WTYPE rounded = ((WTYPE)e1 >> (right - 1)) + 1;               \
>                 res = (ETYPE)(rounded >> 1);                                  \
>             }                                                                 \
>
>
> rnax
>
>> +            } else {                                                      \
>> +                WTYPE rounded = ((WTYPE)e1 >> (right - 1)) + 1;           \
>> +                res = (ETYPE)(rounded >> 1);                              \
>> +            }                                                             \
>> +        }                                                                 \