[4/6] target-arm: fix saturated values for Neon right shifts.

From: Christophe Lyon <christophe.lyon@st.com>

Fix value returned by signed qrshl helpers (8, 16 and 32 bits).

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
---
 target-arm/neon_helper.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

On 11 February 2011 15:11,  <christophe.lyon@st.com> wrote:

> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -903,7 +903,7 @@ uint64_t HELPER(neon_qrshl_u64)(CPUState *env, uint64_t val, uint64_t shiftop)
>         dest = src1 << tmp; \
>         if ((dest >> tmp) != src1) { \
>             SET_QC(); \
> -            dest = src1 >> 31; \
> +            dest = (uint32_t)(1 << (sizeof(src1) * 8 - 1)) - (src1 > 0 ? 1 : 0); \
>         } \
>     }} while (0)

This gives the right values but is a pretty long expression (among
other things it is more than 80 chars and breaks the QEMU coding
style). I'd rather do the same as the existing qshl_s* helper:

            dest = (uint32_t)(1 << (sizeof(src1) * 8 - 1)); \
            if (src1 > 0) { \
                dest--; \
            } \

>  NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
> @@ -924,7 +924,11 @@ uint32_t HELPER(neon_qrshl_s32)(CPUState *env, uint32_t valop, uint32_t shiftop)
>         dest = val << shift;
>         if ((dest >> shift) != val) {
>             SET_QC();
> -            dest = (uint32_t)(1 << (sizeof(val) * 8 - 1)) - (val > 0 ? 1 : 0);
> +            if (val < 0) {
> +                dest = INT32_MIN;
> +            } else {
> +                dest = INT32_MAX;
> +            }

Again, right answers but the way most of the rest of the code
forces a 32 bit value to signed saturation is
   dest = (val >> 31) ^ ~SIGNBIT;

-- PMM

On 14 February 2011 17:46, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 February 2011 15:11,  <christophe.lyon@st.com> wrote:
>>  NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
>> @@ -924,7 +924,11 @@ uint32_t HELPER(neon_qrshl_s32)(CPUState *env, uint32_t valop, uint32_t shiftop)
>>         dest = val << shift;
>>         if ((dest >> shift) != val) {
>>             SET_QC();
>> -            dest = (uint32_t)(1 << (sizeof(val) * 8 - 1)) - (val > 0 ? 1 : 0);
>> +            if (val < 0) {
>> +                dest = INT32_MIN;
>> +            } else {
>> +                dest = INT32_MAX;
>> +            }
>
> Again, right answers but the way most of the rest of the code
> forces a 32 bit value to signed saturation is
>   dest = (val >> 31) ^ ~SIGNBIT;

...and also this is patching a function newly introduced in
patch 1/6 -- better to just have 1/6 have the correct code.

-- PMM

>> @@ -924,7 +924,11 @@ uint32_t HELPER(neon_qrshl_s32)(CPUState *env, uint32_t valop, uint32_t shiftop)
>>         dest = val << shift;
>>         if ((dest >> shift) != val) {
>>             SET_QC();
>> -            dest = (uint32_t)(1 << (sizeof(val) * 8 - 1)) - (val > 0 ? 1 : 0);
>> +            if (val < 0) {
>> +                dest = INT32_MIN;
>> +            } else {
>> +                dest = INT32_MAX;
>> +            }
> 
> Again, right answers but the way most of the rest of the code
> forces a 32 bit value to signed saturation is
>    dest = (val >> 31) ^ ~SIGNBIT;
> 

Indeed; I hadn't given a close look at how saturation is performed elsewhere.
Anyway I find my version more readable ;-)

Quite a few times, I have wondered whether there are rules in QEmu development helping decide between performance of the code vs readability/maintainability?

Christophe.