| Message ID | 20210729111512.16541-8-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: MVE slices 3 and 4 | expand |
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~
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
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)) {
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