| Message ID | 20210419060145.11152-1-frank.chang@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: fix vssub.vv saturation bug | expand |
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 > --- > 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 > >
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. 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 > > > > >
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 >> > >> >
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;