Re: [PATCH 38/55] target/arm: Implement MVE VQADD and VQSUB

[Richard Henderson]

On 6/7/21 9:58 AM, Peter Maydell wrote:
> +#define DO_2OP_SAT_SCALAR(OP, ESIZE, TYPE, H, FN)                       \
> +    void HELPER(glue(mve_, OP))(CPUARMState *env, void *vd, void *vn,   \
> +                                uint32_t rm)                            \
> +    {                                                                   \
> +        TYPE *d = vd, *n = vn;                                          \
> +        TYPE m = rm;                                                    \
> +        uint16_t mask = mve_element_mask(env);                          \
> +        unsigned e;                                                     \
> +        for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) {              \
> +            bool sat = false;                                           \
> +            TYPE r = FN(n[H(e)], m, &sat);                              \
> +            uint64_t bytemask = mask_to_bytemask##ESIZE(mask);          \
> +            d[H(e)] &= ~bytemask;                                       \
> +            d[H(e)] |= (r & bytemask);                                  \
> +            if (sat && (mask & 1)) {                                    \
> +                env->vfp.qc[0] = 1;                                     \
> +            }                                                           \
> +        }                                                               \
> +        mve_advance_vpt(env);                                           \
> +    }

Perhaps slightly better as

  bool qc = false;

    qc |= sat & mask & 1;

  if (qc) {
    env->vfp.qc[0] = qc;
  }

Maybe reverse the store into &sat (set false if no saturation), and init as

    bool sat = mask & 1;

Though if you choose not to exploit this kind of conditional store, perhaps it 
would be better to fully set *s within do_sat_bhw.  That is, do not rely on 
initialization to false outside the subroutine.

Which you choose,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~