On 23.08.2024 01:06, Andrew Cooper wrote:
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -35,6 +35,12 @@ extern void __bitop_bad_size(void);
unsigned int __pure generic_ffsl(unsigned long x);
unsigned int __pure generic_flsl(unsigned long x);
> +/*
> + * Hamming Weight, also called Population Count. Returns the number of set
> + * bits in @x.
> + */
> +unsigned int __pure generic_hweightl(unsigned long x);
Aren't this and ...
> @@ -284,6 +290,18 @@ static always_inline __pure unsigned int fls64(uint64_t
> x)
> (_v & (_v - 1)) != 0; \
> })
>
> +static always_inline __pure unsigned int hweightl(unsigned long x)
... this even __attribute_const__?
> +{
> + if ( __builtin_constant_p(x) )
> + return __builtin_popcountl(x);
How certain are you that compilers (even old ones) will reliably fold
constant expressions here, and never emit a libgcc call instead? The
conditions look to be more tight than just __builtin_constant_p(); a
pretty absurd example:
unsigned ltest(void) {
return __builtin_constant_p("") ? __builtin_popcountl((unsigned long)"") :
~0;
}
> --- /dev/null
> +++ b/xen/lib/generic-hweightl.c
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/bitops.h>
> +#include <xen/init.h>
> +#include <xen/self-tests.h>
> +
> +/* Mask value @b broadcast to every byte in a long */
> +#if BITS_PER_LONG == 32
> +# define MASK(b) ((b) * 0x01010101UL)
> +#elif BITS_PER_LONG == 64
> +# define MASK(b) ((b) * 0x0101010101010101UL)
> +#else
> +# error Extend me please
> +#endif
> +
> +unsigned int generic_hweightl(unsigned long x)
> +{
> + x -= (x >> 1) & MASK(0x55);
> + x = (x & MASK(0x33)) + ((x >> 2) & MASK(0x33));
> + x = (x + (x >> 4)) & MASK(0x0f);
> +
> + if ( IS_ENABLED(CONFIG_HAS_FAST_MULTIPLY) )
> + return (x * MASK(0x01)) >> (BITS_PER_LONG - 8);
I realize it's nitpicking, yet especially this use isn't really "mask"-
like. Could I talk you into naming the macro e.g. BCST()?
> + x += x >> 8;
> + x += x >> 16;
> +#if BITS_PER_LONG > 32
> + x += x >> 32;
> +#endif
> +
> + return x & 0xff;
> +}
Perhaps #undef MASK here, or else ...
> +#ifdef CONFIG_SELF_TESTS
> +static void __init __constructor test_generic_hweightl(void)
> +{
> + RUNTIME_CHECK(generic_hweightl, 0, 0);
> + RUNTIME_CHECK(generic_hweightl, 1, 1);
> + RUNTIME_CHECK(generic_hweightl, 3, 2);
> + RUNTIME_CHECK(generic_hweightl, 7, 3);
> + RUNTIME_CHECK(generic_hweightl, 0xff, 8);
> +
> + RUNTIME_CHECK(generic_hweightl, 1 | (1UL << (BITS_PER_LONG - 1)), 2);
> + RUNTIME_CHECK(generic_hweightl, -1UL, BITS_PER_LONG);
> +}
... actually use it some here, to have a few more cases?
Jan
