Re: [PATCH for-6.2 07/53] target/arm: Fix MVE 48-bit SQRSHRL for small right shifts

[Richard Henderson]

On 7/29/21 1:14 AM, Peter Maydell wrote:
> We got an edge case wrong in the 48-bit SQRSHRL implementation: if
> the shift is to the right, although it always makes the result
> smaller than the input value it might not be within the 48-bit range
> the result is supposed to be if the input had some bits in [63..48]
> set and the shift didn't bring all of those within the [47..0] range.
>
> Handle this similarly to the way we already do for this case in
> do_uqrshl48_d(): extend the calculated result from 48 bits,
> and return that if not saturating or if it doesn't change the
> result; otherwise fall through to return a saturated value.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Not squashed into the previous patch because that one has already
> been reviewed, so as this fixes a different edge case I thought
> it clearer kept separate.
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

>   target/arm/mve_helper.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/mve_helper.c b/target/arm/mve_helper.c
> index 5730b48f35e..1a4b2ef8075 100644
> --- a/target/arm/mve_helper.c
> +++ b/target/arm/mve_helper.c
> @@ -1563,6 +1563,8 @@ uint64_t HELPER(mve_uqrshll)(CPUARMState *env, uint64_t 
> n, uint32_t shift)
>   static inline int64_t do_sqrshl48_d(int64_t src, int64_t shift,
>                                       bool round, uint32_t *sat)
>   {
> +    int64_t val, extval;
> +
>       if (shift <= -48) {
>           /* Rounding the sign bit always produces 0. */
>           if (round) {
> @@ -1572,9 +1574,14 @@ static inline int64_t do_sqrshl48_d(int64_t src, int64_t 
> shift,
>       } else if (shift < 0) {
>           if (round) {
>               src >>= -shift - 1;
> -            return (src >> 1) + (src & 1);
> +            val = (src >> 1) + (src & 1);
> +        } else {
> +            val = src >> -shift;
> +        }
> +        extval = sextract64(val, 0, 48);
> +        if (!sat || val == extval) {
> +            return extval;
>           }
> -        return src >> -shift;

I'll note two things:

(1) The val == extval check could be sunk to the end of the function and shared with the 
left shift,

(2) sat will never be unset, as #48 is encoded as sat=1 in the insn.


r~