On 27.08.2024 12:39, Andrew Cooper wrote:
> On 26/08/2024 12:40 pm, Jan Beulich wrote:
>> 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__?
>
> Hmm. This is following fls(), but on further consideration, they should
> be const too.
>
> I'll do a prep patch fixing that, although I'm going to rename it to
> __attr_const for brevity.
>
> Much as I'd prefer __const, I expect that is going too far, making it
> too close to regular const.
I was actually going to suggest using that name, if we want to shorten
__attribute_const__. Yes, gcc treats __const (and __const__) as
keywords, but do we care (much)? All it requires is that we don't start
using __const as a (real) keyword.
Of course __const is a good example of why really we shouldn't use
double-underscore prefixed names anywhere. Any of them can be assigned
a meaning by the compiler, and here that's clearly the case. Therefore,
taking your planned rename, maybe better make it attr_const then? And
eventually switch stuff like __packed, __pure, and __weak to attr_* as
well? Or even introduce something like
#define attr(attr...) __attribute__((attr))
and use attr(const) here?
>>> +{
>>> + 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;
>> }
>
> How do you express that in terms of a call to hweightl()?
hweightl((unsigned long)"");
Yet as said - it's absurd. It merely serves to make the point that what
__builtin_constant_p() returns true for doesn't necessarily constant-
fold in expressions.
> Again, this is following the layout started with fls() in order to avoid
> each arch opencoding different versions of constant folding.
>
> https://godbolt.org/z/r544c49oY
>
> When it's forced through the hweightl() interface, even GCC 4.1 decides
> that it's non-constant and falls back to generic_hweightl().
>
>
> I did spend a *lot* of time with the fls() series checking that all
> compilers we supported did what we wanted in this case, so I don't
> expect it to be a problem.
Right, and I guess I was pointlessly more concerned about popcount than
I was for ffs() / fls(). The criteria upon which gcc decides whether to
constant-fold the uses is exactly the same.
> But, if a library call is emitted, it will
> be very obvious (link failure), and we can re-evaluate.
Indeed, we certainly would notice, albeit the diagnostic may be cryptic
to people.
Bottom line - keep it as is.
Jan
