Re: [Qemu-devel] [PATCH v2] bitops: unify bitops_ffsl with the one in host-utils.h, using __builtin_ffsl

[Paolo Bonzini]

Il 30/01/2013 18:57, Paolo Bonzini ha scritto:
> @@ -30,34 +31,21 @@
>   */
>  static unsigned long bitops_ffsl(unsigned long word)
>  {
> -     int num = 0;
> +#if QEMU_GNUC_PREREQ(3, 4)
> +    return __builtin_ffsl(word) + 1;

Obviously wrong, sent before "git commit --amend".

But it also reminds me that I noticed most uses of ffsl tend to subtract
one, and GCC optimizes it a little better if it is written as ctz + 1
and inlined.

Nacked-by: Paolo Bonzini <address@hidden>

but v1 stands.

Paolo

> +#else
> +    if (!word) {
> +        return 0;
> +    }
>  
> -#if LONG_MAX > 0x7FFFFFFF
> -     if ((word & 0xffffffff) == 0) {
> -             num += 32;
> -             word >>= 32;
> -     }
> +    if (sizeof(long) == 4) {
> +        return ctz32(word) + 1;
> +    } else if (sizeof(long) == 8) {
> +        return ctz64(word) + 1;
> +    } else {
> +        abort();
> +    }
>  #endif
> -     if ((word & 0xffff) == 0) {
> -             num += 16;
> -             word >>= 16;
> -     }
> -     if ((word & 0xff) == 0) {
> -             num += 8;
> -             word >>= 8;
> -     }
> -     if ((word & 0xf) == 0) {
> -             num += 4;
> -             word >>= 4;
> -     }
> -     if ((word & 0x3) == 0) {
> -             num += 2;
> -             word >>= 2;
> -     }
> -     if ((word & 0x1) == 0) {
> -             num += 1;
> -        }
> -     return num;
>  }
>  
>  /**
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 73f5d1d..abad209 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -170,7 +170,7 @@ static inline int64_t hbitmap_iter_next(HBitmapIter *hbi)
>  
>      /* The next call will resume work from the next bit.  */
>      hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
> -    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ffsl(cur) - 1;
> +    item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + bitops_ffsl(cur) - 1;
>  
>      return item << hbi->granularity;
>  }
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index 2a32be4..40457bd 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -238,29 +238,4 @@ static inline int ctpop64(uint64_t val)
>  #endif
>  }
>  
> -/* glibc does not provide an inline version of ffsl, so always define
> - * ours.  We need to give it a different name, however.
> - */
> -#ifdef __GLIBC__
> -#define ffsl qemu_ffsl
> -#endif
> -static inline int ffsl(long val)
> -{
> -    if (!val) {
> -        return 0;
> -    }
> -
> -#if QEMU_GNUC_PREREQ(3, 4)
> -    return __builtin_ctzl(val) + 1;
> -#else
> -    if (sizeof(long) == 4) {
> -        return ctz32(val) + 1;
> -    } else if (sizeof(long) == 8) {
> -        return ctz64(val) + 1;
> -    } else {
> -        abort();
> -    }
> -#endif
> -}
> -
>  #endif
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 2aa487d..32c3d59 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -126,7 +126,7 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
>           * The index of this word's least significant set bit provides
>           * the low-order bits.
>           */
> -        pos = (pos << BITS_PER_LEVEL) + ffsl(cur) - 1;
> +        pos = (pos << BITS_PER_LEVEL) + bitops_ffsl(cur) - 1;
>          hbi->cur[i] = cur & (cur - 1);
>  
>          /* Set up next level for iteration.  */
>