Re: [PATCH] target/riscv: fix vssub.vv saturation bug

[Alistair Francis]

On Mon, Apr 19, 2021 at 4:31 PM Frank Chang <0xc0de0125@gmail.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> 於 2021年4月19日 週一 下午2:28寫道：
>>
>> On Mon, Apr 19, 2021 at 4:02 PM <frank.chang@sifive.com> wrote:
>> >
>> > From: Frank Chang <frank.chang@sifive.com>
>> >
>> > Doing a negate (0x0 – 0x80000000) using vssub.vv produces
>> > an incorrect result of 0x80000000 (should saturate to 0x7fffffff)
>> >
>> > Fix this bug by treating zero as a positive number.
>> >
>> > Signed-off-by: Frank Chang <frank.chang@sifive.com>
>>
>> Thanks for the patch!
>>
>> A similar fix is already in the riscv-to-apply.next queue
>>
>>    target/riscv: Fixup saturate subtract function
>>
>>    The overflow predication ((a - b) ^ a) & (a ^ b) & INT64_MIN is right.
>>    However, when the predication is ture and a is 0, it should return 
>> maximum.
>>
>>    Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
>>    Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>    Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>    Message-id: 20210212150256.885-4-zhiwei_liu@c-sky.com
>>    Message-Id: <20210212150256.885-4-zhiwei_liu@c-sky.com>
>>
>>
>> Alistair
>
>
> Thanks, I might missed that patch.

No worries. Thanks anyway for the fix.

Alistair

> Frank Chang
>
>>
>>
>> > ---
>> >  target/riscv/vector_helper.c | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
>> > index a156573d281..356cef8a090 100644
>> > --- a/target/riscv/vector_helper.c
>> > +++ b/target/riscv/vector_helper.c
>> > @@ -2451,7 +2451,7 @@ static inline int8_t ssub8(CPURISCVState *env, int 
>> > vxrm, int8_t a, int8_t b)
>> >  {
>> >      int8_t res = a - b;
>> >      if ((res ^ a) & (a ^ b) & INT8_MIN) {
>> > -        res = a > 0 ? INT8_MAX : INT8_MIN;
>> > +        res = a >= 0 ? INT8_MAX : INT8_MIN;
>> >          env->vxsat = 0x1;
>> >      }
>> >      return res;
>> > @@ -2461,7 +2461,7 @@ static inline int16_t ssub16(CPURISCVState *env, int 
>> > vxrm, int16_t a, int16_t b)
>> >  {
>> >      int16_t res = a - b;
>> >      if ((res ^ a) & (a ^ b) & INT16_MIN) {
>> > -        res = a > 0 ? INT16_MAX : INT16_MIN;
>> > +        res = a >= 0 ? INT16_MAX : INT16_MIN;
>> >          env->vxsat = 0x1;
>> >      }
>> >      return res;
>> > @@ -2471,7 +2471,7 @@ static inline int32_t ssub32(CPURISCVState *env, int 
>> > vxrm, int32_t a, int32_t b)
>> >  {
>> >      int32_t res = a - b;
>> >      if ((res ^ a) & (a ^ b) & INT32_MIN) {
>> > -        res = a > 0 ? INT32_MAX : INT32_MIN;
>> > +        res = a >= 0 ? INT32_MAX : INT32_MIN;
>> >          env->vxsat = 0x1;
>> >      }
>> >      return res;
>> > @@ -2481,7 +2481,7 @@ static inline int64_t ssub64(CPURISCVState *env, int 
>> > vxrm, int64_t a, int64_t b)
>> >  {
>> >      int64_t res = a - b;
>> >      if ((res ^ a) & (a ^ b) & INT64_MIN) {
>> > -        res = a > 0 ? INT64_MAX : INT64_MIN;
>> > +        res = a >= 0 ? INT64_MAX : INT64_MIN;
>> >          env->vxsat = 0x1;
>> >      }
>> >      return res;
>> > --
>> > 2.17.1
>> >
>> >