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

Message ID 20210729111512.16541-8-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • target/arm: MVE slices 3 and 4
Related show

Commit Message

Peter Maydell July 29, 2021, 11:14 a.m.
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.
---
 target/arm/mve_helper.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.20.1

Comments

Richard Henderson July 30, 2021, 7:07 p.m. | #1
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~
Peter Maydell Aug. 12, 2021, 9:43 a.m. | #2
On Fri, 30 Jul 2021 at 20:07, Richard Henderson
<richard.henderson@linaro.org> wrote:
> 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.


True; I'm kind of aiming for parity with the other sqrshl utility routines,
which is why the unused no-saturation code is in there.

-- PMM

Patch

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;
     } else if (shift < 48) {
         int64_t extval = sextract64(src << shift, 0, 48);
         if (!sat || src == (extval >> shift)) {