Re: [Qemu-devel] [PULL 3/7] qxl: refactor rounding up to a nearest power of 2

[Markus Armbruster]

Gerd Hoffmann <address@hidden> writes:

> From: Radim Krčmář <address@hidden>
>
> We already have pow2floor, mirror it and use instead of a function with
> similar results (same in used domain), to clarify our intent.
>
> Signed-off-by: Radim Krčmář <address@hidden>
> Signed-off-by: Gerd Hoffmann <address@hidden>
[...]
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 644b46d..1b5cffb 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -418,6 +418,9 @@ static inline bool is_power_of_2(uint64_t value)
>  /* round down to the nearest power of 2*/
>  int64_t pow2floor(int64_t value);
>  
> +/* round up to the nearest power of 2 (0 if overflow) */
> +uint64_t pow2ceil(uint64_t value);
> +
>  #include "qemu/module.h"
>  
>  /*
> diff --git a/util/cutils.c b/util/cutils.c
> index dbe7412..c2250d1 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -483,6 +483,20 @@ int64_t pow2floor(int64_t value)
>      return value;
>  }
>  
> +/* round up to the nearest power of 2 (0 if overflow) */

Callers need to check for overflow, but that's the callers' problem.

> +uint64_t pow2ceil(uint64_t value)
> +{
> +    uint8_t nlz = clz64(value);

You convert the value of clz64() from int to uint8_t only to promote it
right back to int in every single use.  Please don't muddy the waters
that way.

> +
> +    if (is_power_of_2(value)) {
> +        return value;
> +    }
> +    if (!nlz) {
> +        return 0;
> +    }
> +    return 1ULL << (64 - nlz);
> +}
> +
>  /*
>   * Implementation of  ULEB128 (http://en.wikipedia.org/wiki/LEB128)
>   * Input is limited to 14-bit numbers

Doesn't really mirror pow2floor() in master, because that one uses
int64_t.  Fine with me, because my "[PATCH 0/2] Proactive pow2floor()
fix, and dead code removal" changes pow2floor() to uint64_t.

Unfortunately, the two patches conflict.

This patch's implementation of pow2ceil() is needlessly complicated,
just like pow2floor() in master.  Simpler:

    uint64_t pow2ceil2(uint64_t value)
    {
        int n = clz64(value - 1);
        return n ? 1ull << (64 - n) : 0;
    }

I can rebase my patch on top of this one, and clean things up to my
taste.