Patchwork bitops: unify bitops_flsl with host-utils.h, call it bitops_clzl

login
register
mail settings
Submitter Richard Henderson
Date Feb. 4, 2013, 6:26 p.m.
Message ID <1360002362-16355-1-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/218030/
State New
Headers show

Comments

Richard Henderson - Feb. 4, 2013, 6:26 p.m.
Similar to the bitops_ctz change just made.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 include/qemu/bitops.h  | 44 +++++++++++++++-----------------------------
 target-i386/topology.h |  2 +-
 util/bitops.c          |  2 +-
 3 files changed, 17 insertions(+), 31 deletions(-)
Peter Maydell - Feb. 4, 2013, 6:34 p.m.
On 4 February 2013 18:26, Richard Henderson <rth@twiddle.net> wrote:
>  /**
> - * bitops_fls - find last (most-significant) set bit in a long word
> + * bitops_clzl - find last (most-significant) set bit in a long word

Should probably change the descriptive text to say "count leading
zeros"...

>   * @word: the word to search
>   *
>   * Undefined if no set bit exists, so code should check against 0 first.

Should we change this to match clz32()/clz64()/bitops_ctzl() in not
having an undefined-behaviour case?

>   */
> -static inline unsigned long bitops_flsl(unsigned long word)
> +static inline unsigned long bitops_clzl(unsigned long word)
>  {
> -       int num = BITS_PER_LONG - 1;
> -
> -#if LONG_MAX > 0x7FFFFFFF
> -       if (!(word & (~0ul << 32))) {
> -               num -= 32;
> -               word <<= 32;
> -       }
> +    /* Both this function and the gcc builtin are undefined at 0, whereas
> +       the host-utils functions return word-size at 0.  Thus we prefer the
> +       gcc builtin as the closer match.  */

I think we should just fix the inconsistency rather than
retaining several similarly-named functions which have differing
corner-case behaviour.

-- PMM
Eric Blake - Feb. 4, 2013, 6:36 p.m.
On 02/04/2013 11:26 AM, Richard Henderson wrote:
> Similar to the bitops_ctz change just made.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  include/qemu/bitops.h  | 44 +++++++++++++++-----------------------------
>  target-i386/topology.h |  2 +-
>  util/bitops.c          |  2 +-
>  3 files changed, 17 insertions(+), 31 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index 8b88791..07ada63 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -50,41 +50,27 @@ static unsigned long bitops_ctzl(unsigned long word)
>  }
>  
>  /**
> - * bitops_fls - find last (most-significant) set bit in a long word
> + * bitops_clzl - find last (most-significant) set bit in a long word
>   * @word: the word to search
>   *
>   * Undefined if no set bit exists, so code should check against 0 first.

Interesting that you chose to leave 0 undefined here, even though
bitops_ctzl specifically defines that case, but not a show-stopper.  Per
Paolo's arguments in the other thread, if we did decide to define
bitops_clzl(0), setting it to the word size would be a reasonable choice
(bitops_clzl(2)=>62, bitops_clzl(1)=>63, bitops_clzl(0)=>64), but that
can be done later if it turns out to be useful.
Richard Henderson - Feb. 4, 2013, 6:38 p.m.
On 2013-02-04 10:34, Peter Maydell wrote:
>> -       }
>> +    /* Both this function and the gcc builtin are undefined at 0, whereas
>> +       the host-utils functions return word-size at 0.  Thus we prefer the
>> +       gcc builtin as the closer match.  */
>
> I think we should just fix the inconsistency rather than
> retaining several similarly-named functions which have differing
> corner-case behaviour.

As opposed to bitops_ctzl (-1) and host-utils ctzN (N) ?


r~

Patch

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 8b88791..07ada63 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -50,41 +50,27 @@  static unsigned long bitops_ctzl(unsigned long word)
 }
 
 /**
- * bitops_fls - find last (most-significant) set bit in a long word
+ * bitops_clzl - find last (most-significant) set bit in a long word
  * @word: the word to search
  *
  * Undefined if no set bit exists, so code should check against 0 first.
  */
-static inline unsigned long bitops_flsl(unsigned long word)
+static inline unsigned long bitops_clzl(unsigned long word)
 {
-	int num = BITS_PER_LONG - 1;
-
-#if LONG_MAX > 0x7FFFFFFF
-	if (!(word & (~0ul << 32))) {
-		num -= 32;
-		word <<= 32;
-	}
+    /* Both this function and the gcc builtin are undefined at 0, whereas
+       the host-utils functions return word-size at 0.  Thus we prefer the
+       gcc builtin as the closer match.  */
+#if QEMU_GNUC_PREREQ(3, 4)
+    return __builtin_clzl(word);
+#else
+    if (sizeof(long) == 4) {
+        return clz32(word);
+    } else if (sizeof(long) == 8) {
+        return clz64(word);
+    } else {
+        abort();
+    }
 #endif
-	if (!(word & (~0ul << (BITS_PER_LONG-16)))) {
-		num -= 16;
-		word <<= 16;
-	}
-	if (!(word & (~0ul << (BITS_PER_LONG-8)))) {
-		num -= 8;
-		word <<= 8;
-	}
-	if (!(word & (~0ul << (BITS_PER_LONG-4)))) {
-		num -= 4;
-		word <<= 4;
-	}
-	if (!(word & (~0ul << (BITS_PER_LONG-2)))) {
-		num -= 2;
-
-		word <<= 2;
-	}
-	if (!(word & (~0ul << (BITS_PER_LONG-1))))
-		num -= 1;
-	return num;
 }
 
 /**
diff --git a/target-i386/topology.h b/target-i386/topology.h
index 24ed525..96a559e 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -55,7 +55,7 @@  static unsigned apicid_bitwidth_for_count(unsigned count)
     if (count == 1) {
         return 0;
     }
-    return bitops_flsl(count - 1) + 1;
+    return bitops_clzl(count - 1) + 1;
 }
 
 /* Bit width of the SMT_ID (thread ID) field on the APIC ID
diff --git a/util/bitops.c b/util/bitops.c
index 7b853cf..4f4fa95 100644
--- a/util/bitops.c
+++ b/util/bitops.c
@@ -133,7 +133,7 @@  unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
         tmp = addr[--words];
         if (tmp) {
         found:
-            return words * BITS_PER_LONG + bitops_flsl(tmp);
+            return words * BITS_PER_LONG + bitops_clzl(tmp);
         }
     }