| Message ID | 1360002362-16355-1-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
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
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.
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~
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); } }
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(-)