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

On Fri, Jul 17, 2026 at 10:06:55AM +0800, 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
> 
> diff --git a/target/riscv/tcg/meson.build b/target/riscv/tcg/meson.build
> index a05ab642f41..cc438d7c0d2 100644
> --- a/target/riscv/tcg/meson.build
> +++ b/target/riscv/tcg/meson.build
> @@ -15,7 +15,8 @@ riscv_ss.add(files(
>    'vcrypto_helper.c',
>    'vector_helper.c',
>    'vector_internals.c',
> -  'zce_helper.c'))
> +  'zce_helper.c',
> +  '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
> @@ -0,0 +1,1656 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* RISC-V Packed SIMD Extension Helpers for QEMU. */
> +/* Copyright (C) 2026 ISRC ISCAS. */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "qemu/host-utils.h"
> +#include "exec/helper-proto.h"
> +#include "fpu/softfloat.h"
> +#include "internals.h"
> +
> +
> +/* Helper macros */
> +
> +/* Element count calculations */
> +#define ELEMS_B(target) (sizeof(target) * 8 / 8)    /* byte elements count */
> +#define ELEMS_H(target) (sizeof(target) * 8 / 16)
> +#define ELEMS_W(target) (sizeof(target) * 8 / 32)   /* word elements count */
> +#define ELEMS_D(target) (sizeof(target) * 8 / 64)
> +
> +/* Element extraction macros - unsigned to avoid sign extension */
> +#define EXTRACT8(val, idx)  (((val) >> ((idx) * 8)) & 0xFF)
> +#define EXTRACT16(val, idx) (((val) >> ((idx) * 16)) & 0xFFFF)
> +#define EXTRACT32(val, idx) (((val) >> ((idx) * 32)) & 0xFFFFFFFF)
> +#define EXTRACT64(val, idx) (((val) >> ((idx) * 64)) & 0xFFFFFFFFFFFFFFFFULL)
> +
QEMU has the generator bit-operation helper macros. I believe you don't need to
define them again, so you can just follow this header file path.

path: include/qemu/bitops.h

```
  extract8(value, start, length)
  extract16(value, start, length)
  extract32(value, start, length)
  extract64(value, start, length)

  sextract32(value, start, length)
  sextract64(value, start, length)
```

So we can redefine these helper macros like this:

```
  #define EXTRACT_B(val, idx) extract64((val), (idx) * 8, 8)
  #define EXTRACT_H(val, idx) extract64((val), (idx) * 16, 16)
  #define EXTRACT_W(val, idx) extract64((val), (idx) * 32, 32)
  #define EXTRACT_D(val, idx) extract64((val), (idx) * 64, 64)
```

> +/* Element insertion macros */
> +#define INSERT8(val, res, idx) \
> +    ((val) | ((target_ulong)(uint8_t)(res) << ((idx) * 8)))
> +#define INSERT16(val, res, idx) \
> +    ((val) | ((target_ulong)(uint16_t)(res) << ((idx) * 16)))
> +#define INSERT32(val, res, idx) \
> +    ((val) | ((target_ulong)(uint32_t)(res) << ((idx) * 32)))
> +#define INSERT32_64(val, res, idx) \
> +    ((val) | ((uint64_t)(uint32_t)(res) << ((idx) * 32)))
> +#define INSERT64(val, res, idx) \
> +    ((val) | ((uint64_t)(res) << ((idx) * 64)))
> +
> +/* Saturation constants */
> +static const int8_t   SAT_MAX_B = 127;
> +static const int8_t   SAT_MIN_B = -128;
> +static const int16_t  SAT_MAX_H = 32767;
> +static const int16_t  SAT_MIN_H = -32768;
> +static const int32_t  SAT_MAX_W = 2147483647;
> +static const int32_t  SAT_MIN_W = -2147483648LL;
> +static const uint8_t  USAT_MAX_B = 255;
> +static const uint16_t USAT_MAX_H = 65535;
> +static const uint32_t USAT_MAX_W = 4294967295U;
> +
> +
> +/* Saturation helper functions */
> +
> +/**
> + * Signed saturation for 8-bit elements
> + * Returns saturated value and sets *sat if saturation occurred
> + */
This helper function has some comments that explain what it does.
I believe we don't need these explanations if the function name is
already clear.

The other helper functions below are similar. I think we only need
to add comments when it's necessary to explain the reasoning behind
why we're doing something.

d> +static inline int8_t signed_saturate_b(int32_t val, int *sat)
> +{
> +    if (val > SAT_MAX_B) {
> +        *sat = 1;
> +        return SAT_MAX_B;
> +    }
> +    if (val < SAT_MIN_B) {
> +        *sat = 1;
> +        return SAT_MIN_B;
> +    }
> +    return (int8_t)val;
> +}
> +
> +/**
> + * Signed saturation for 16-bit elements
> + */
> +static inline int16_t signed_saturate_h(int32_t val, int *sat)
> +{
> +    if (val > SAT_MAX_H) {
> +        *sat = 1;
> +        return SAT_MAX_H;
> +    }
> +    if (val < SAT_MIN_H) {
> +        *sat = 1;
> +        return SAT_MIN_H;
> +    }
> +    return (int16_t)val;
> +}
> +
> +/**
> + * Signed saturation for 32-bit elements
> + */
> +static inline int32_t signed_saturate_w(int64_t val, int *sat)
> +{
> +    if (val > SAT_MAX_W) {
> +        *sat = 1;
> +        return SAT_MAX_W;
> +    }
> +    if (val < SAT_MIN_W) {
> +        *sat = 1;
> +        return SAT_MIN_W;
> +    }
> +    return (int32_t)val;
> +}
> +
> +/**
> + * Unsigned saturation for 8-bit elements
> + */
> +static inline uint8_t unsigned_saturate_b(uint32_t val, int *sat)
> +{
> +    if (val > USAT_MAX_B) {
> +        *sat = 1;
> +        return USAT_MAX_B;
> +    }
> +    return (uint8_t)val;
> +}
> +
> +/**
> + * Unsigned saturation for 16-bit elements
> + */
> +static inline uint16_t unsigned_saturate_h(uint32_t val, int *sat)
> +{
> +    if (val > USAT_MAX_H) {
> +        *sat = 1;
> +        return USAT_MAX_H;
> +    }
> +    return (uint16_t)val;
> +}
> +
> +/**
> + * Unsigned saturation for 32-bit elements
> + */
> +static inline uint32_t unsigned_saturate_w(uint64_t val, int *sat)
> +{
> +    if (val > USAT_MAX_W) {
> +        *sat = 1;
> +        return USAT_MAX_W;
> +    }
> +    return (uint32_t)val;
> +}
> +
> +static inline target_ulong psimd_abdsumu_b(target_ulong rs1,
> +                                           target_ulong rs2,
> +                                           target_ulong sum)
> +{
> +    int elems = ELEMS_B(rs1);
> +
> +    for (int i = 0; i < elems; i++) {
> +        uint8_t e1 = EXTRACT8(rs1, i);
> +        uint8_t e2 = EXTRACT8(rs2, i);
> +        uint8_t diff = (e1 > e2) ? (e1 - e2) : (e2 - e1);
> +        sum += diff;
> +    }
> +
> +    return sum;
> +}
> +
> +#define PSIMD_DO_ADD(N, M) ((N) + (M))
> +#define PSIMD_DO_SUB(N, M) ((N) - (M))
> +#define PSIMD_DO_ABD(N, M) ((N) >= (M) ? (N) - (M) : (M) - (N))
> +#define PSIMD_DO_EQ_MASK(N, M) ((N) == (M) ? -1 : 0)
> +#define PSIMD_DO_LT_MASK(N, M) ((N) < (M) ? -1 : 0)
> +#define PSIMD_DO_MIN(N, M) ((N) < (M) ? (N) : (M))
> +#define PSIMD_DO_MAX(N, M) ((N) > (M) ? (N) : (M))
> +#define PSIMD_DO_SLL(N, M) ((N) << (M))
> +#define PSIMD_DO_SRL(N, M) ((N) >> (M))
> +#define PSIMD_DO_SRA(N, M) ((N) >> (M))
> +

[...]

> +
> +#define GEN_PSIMD_SCALAR_ABS(NAME, RTYPE, STYPE, UTYPE)                   \
> +RTYPE HELPER(NAME)(CPURISCVState *env, RTYPE rs1)                         \
> +{                                                                         \
> +    STYPE value = (STYPE)rs1;                                            \
> +    UTYPE result = (UTYPE)value;                                         \
> +                                                                          \
> +    if (value < 0) {                                                      \
> +        result = (UTYPE)0 - result;                                      \
> +    }                                                                     \
> +    return (RTYPE)(STYPE)result;                                         \
> +}
The '\' for the column widths aren't aligned here. It would
look much better if we could align them consistently throughout.

[...]

> +uint64_t HELPER(NAME)(CPURISCVState *env, uint64_t rs1, uint64_t rs2)     \
> +{                                                                         \
> +    int64_t a = (int64_t)rs1;                                            \
> +    int8_t shamt = (int8_t)(rs2 & 0xff);                                 \
> +                                                                          \
> +    if (shamt >= 0) {                                                     \
> +        return (uint64_t)(a << shamt);                                    \
> +    }                                                                     \
> +                                                                          \
> +    int right = -shamt;                                                   \
> +    if (right >= 64) {                                                    \
> +        return (a < 0) ? (uint64_t)-1 : 0;                               \
> +    }                                                                     \
> +    return (uint64_t)RIGHT_OP(a, right);                                  \
> +}
There are similar alignment issues here.

> +
> +#define PSIMD_DO_SRL64(A, B) (((B) >= 64) ? 0 : ((A) >> (B)))
> +#define PSIMD_DO_RNDSRL64(A, B)                                          \
> +    (((B) > 64) ? 0 : ((((A) >> ((B) - 1)) + 1) >> 1))
> +
> +#define GEN_PSIMD_VAR_SRL64(NAME, RIGHT_OP)                               \
> +uint64_t HELPER(NAME)(CPURISCVState *env, uint64_t rs1, uint64_t rs2)     \
> +{                                                                         \
> +    int8_t shamt = (int8_t)(rs2 & 0xff);                                 \
> +                                                                          \
> +    if (shamt < 0) {                                                      \
> +        return RIGHT_OP(rs1, -shamt);                                     \
> +    }                                                                     \
> +    return (shamt >= 64) ? 0 : (rs1 << shamt);                           \
> +}
> +
Same here.You can check again for the patches.

Thanks,
Chao