On 9/10/21 4:26 AM, Luis Pires wrote:
> In preparation for changing the divu128/divs128 implementations
> to allow for quotients larger than 64 bits, move the div-by-zero
> and overflow checks to the callers.
>
> Signed-off-by: Luis Pires <luis.pires@eldorado.org.br>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Frederic, I had forgotten about this work that Luis has done here and in the next 3
patches. This overlaps with what you were doing in the RV128 patch set wrt getting a
proper 128-bit divide.
r~
> ---
> include/hw/clock.h | 5 +++--
> include/qemu/host-utils.h | 36 +++++++++++++----------------------
> target/ppc/int_helper.c | 14 +++++++++-----
> util/host-utils.c | 40 ++++++++++++++++++---------------------
> 4 files changed, 43 insertions(+), 52 deletions(-)
>
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index a7187eab95..5a40a076aa 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -320,8 +320,9 @@ static inline uint64_t clock_ns_to_ticks(const Clock *clk, uint64_t ns)
> return 0;
> }
> /*
> - * Ignore divu128() return value as we've caught div-by-zero and don't
> - * need different behaviour for overflow.
> + * BUG: when CONFIG_INT128 is not defined, the current implementation of
> + * divu128 does not return a valid truncated quotient, so the result will
> + * be wrong.
> */
> divu128(&lo, &hi, clk->period);
> return lo;
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index ca9f3f021b..e82e6239af 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -52,36 +52,26 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> return (__int128_t)a * b / c;
> }
>
> -static inline int divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor)
> -{
> - if (divisor == 0) {
> - return 1;
> - } else {
> - __uint128_t dividend = ((__uint128_t)*phigh << 64) | *plow;
> - __uint128_t result = dividend / divisor;
> - *plow = result;
> - *phigh = dividend % divisor;
> - return result > UINT64_MAX;
> - }
> +static inline void divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor)
> +{
> + __uint128_t dividend = ((__uint128_t)*phigh << 64) | *plow;
> + __uint128_t result = dividend / divisor;
> + *plow = result;
> + *phigh = dividend % divisor;
> }
>
> -static inline int divs128(int64_t *plow, int64_t *phigh, int64_t divisor)
> +static inline void divs128(int64_t *plow, int64_t *phigh, int64_t divisor)
> {
> - if (divisor == 0) {
> - return 1;
> - } else {
> - __int128_t dividend = ((__int128_t)*phigh << 64) | (uint64_t)*plow;
> - __int128_t result = dividend / divisor;
> - *plow = result;
> - *phigh = dividend % divisor;
> - return result != *plow;
> - }
> + __int128_t dividend = ((__int128_t)*phigh << 64) | (uint64_t)*plow;
> + __int128_t result = dividend / divisor;
> + *plow = result;
> + *phigh = dividend % divisor;
> }
> #else
> void muls64(uint64_t *plow, uint64_t *phigh, int64_t a, int64_t b);
> void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
> -int divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
> -int divs128(int64_t *plow, int64_t *phigh, int64_t divisor);
> +void divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
> +void divs128(int64_t *plow, int64_t *phigh, int64_t divisor);
>
> static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> {
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index f5dac3aa87..510faf24cf 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -104,10 +104,11 @@ uint64_t helper_divdeu(CPUPPCState *env, uint64_t ra, uint64_t rb, uint32_t oe)
> uint64_t rt = 0;
> int overflow = 0;
>
> - overflow = divu128(&rt, &ra, rb);
> -
> - if (unlikely(overflow)) {
> + if (unlikely(rb == 0 || ra >= rb)) {
> + overflow = 1;
> rt = 0; /* Undefined */
> + } else {
> + divu128(&rt, &ra, rb);
> }
>
> if (oe) {
> @@ -122,10 +123,13 @@ uint64_t helper_divde(CPUPPCState *env, uint64_t rau, uint64_t rbu, uint32_t oe)
> int64_t rt = 0;
> int64_t ra = (int64_t)rau;
> int64_t rb = (int64_t)rbu;
> - int overflow = divs128(&rt, &ra, rb);
> + int overflow = 0;
>
> - if (unlikely(overflow)) {
> + if (unlikely(rb == 0 || uabs64(ra) >= uabs64(rb))) {
> + overflow = 1;
> rt = 0; /* Undefined */
> + } else {
> + divs128(&rt, &ra, rb);
> }
>
> if (oe) {
> diff --git a/util/host-utils.c b/util/host-utils.c
> index a789a11b46..701a371843 100644
> --- a/util/host-utils.c
> +++ b/util/host-utils.c
> @@ -86,24 +86,23 @@ void muls64 (uint64_t *plow, uint64_t *phigh, int64_t a, int64_t b)
> *phigh = rh;
> }
>
> -/* Unsigned 128x64 division. Returns 1 if overflow (divide by zero or */
> -/* quotient exceeds 64 bits). Otherwise returns quotient via plow and */
> -/* remainder via phigh. */
> -int divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor)
> +/*
> + * Unsigned 128-by-64 division. Returns quotient via plow and
> + * remainder via phigh.
> + * The result must fit in 64 bits (plow) - otherwise, the result
> + * is undefined.
> + * This function will cause a division by zero if passed a zero divisor.
> + */
> +void divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor)
> {
> uint64_t dhi = *phigh;
> uint64_t dlo = *plow;
> unsigned i;
> uint64_t carry = 0;
>
> - if (divisor == 0) {
> - return 1;
> - } else if (dhi == 0) {
> + if (divisor == 0 || dhi == 0) {
> *plow = dlo / divisor;
> *phigh = dlo % divisor;
> - return 0;
> - } else if (dhi >= divisor) {
> - return 1;
> } else {
>
> for (i = 0; i < 64; i++) {
> @@ -120,15 +119,20 @@ int divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor)
>
> *plow = dlo;
> *phigh = dhi;
> - return 0;
> }
> }
>
> -int divs128(int64_t *plow, int64_t *phigh, int64_t divisor)
> +/*
> + * Signed 128-by-64 division. Returns quotient via plow and
> + * remainder via phigh.
> + * The result must fit in 64 bits (plow) - otherwise, the result
> + * is undefined.
> + * This function will cause a division by zero if passed a zero divisor.
> + */
> +void divs128(int64_t *plow, int64_t *phigh, int64_t divisor)
> {
> int sgn_dvdnd = *phigh < 0;
> int sgn_divsr = divisor < 0;
> - int overflow = 0;
>
> if (sgn_dvdnd) {
> *plow = ~(*plow);
> @@ -145,19 +149,11 @@ int divs128(int64_t *plow, int64_t *phigh, int64_t divisor)
> divisor = 0 - divisor;
> }
>
> - overflow = divu128((uint64_t *)plow, (uint64_t *)phigh, (uint64_t)divisor);
> + divu128((uint64_t *)plow, (uint64_t *)phigh, (uint64_t)divisor);
>
> if (sgn_dvdnd ^ sgn_divsr) {
> *plow = 0 - *plow;
> }
> -
> - if (!overflow) {
> - if ((*plow < 0) ^ (sgn_dvdnd ^ sgn_divsr)) {
> - overflow = 1;
> - }
> - }
> -
> - return overflow;
> }
> #endif
>
>