Re: [PATCH v3 05/22] host-utils: move checks out of divu128/divs128

[Richard Henderson <richard.henderson@linaro.org>]

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
>   
>