Re: [PATCH v2 4/8] xen/ppc: Implement bitops.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.08.2023 22:06, Shawn Anastasio wrote:
> On 8/29/23 8:59 AM, Jan Beulich wrote:
>> On 23.08.2023 22:07, Shawn Anastasio wrote:
>>> +{                                                                          
>>>     \
>>> +    unsigned long old;                                                     
>>>     \
>>> +    unsigned int *p = (unsigned int *)_p;                                  
>>>     \
>>> +    asm volatile (                                                         
>>>     \
>>> +    prefix                                                                 
>>>     \
>>> +"1: lwarx %0,0,%3,0\n"                                                     
>>>     \
>>> +    #op "%I2 %0,%0,%2\n"                                                   
>>>     \
>>> +    "stwcx. %0,0,%3\n"                                                     
>>>     \
>>> +    "bne- 1b\n"                                                            
>>>     \
>>> +    : "=&r" (old), "+m" (*p)                                               
>>>     \
>>> +    : "rK" (mask), "r" (p)                                                 
>>>     \
>>> +    : "cc", "memory");                                                     
>>>     \
>>
>> The asm() body wants indenting by another four blanks (more instances below).
>>
> 
> If I were to match the style used in the previous patch's atomic.h, the
> body should be indented to line up with the opening ( of the asm
> statement, right?

Depends on whether the ( is to remain the last token on its line. If so, ...

> I'll go ahead and do that for consistency's sake
> unless you think it would be better to just leave it as-is with an extra
> 4 spaces of indentation.

... this style of indentation would want using. Either is fine, in case you
have a specific preference.

>>> +/* Based on linux/include/asm-generic/bitops/ffz.h */
>>> +/*
>>> + * ffz - find first zero in word.
>>> + * @word: The word to search
>>> + *
>>> + * Undefined if no zero exists, so code should check against ~0UL first.
>>> + */
>>> +#define ffz(x)  __ffs(~(x))
>>> +
>>> +/**
>>> + * hweightN - returns the hamming weight of a N-bit word
>>> + * @x: the word to weigh
>>> + *
>>> + * The Hamming Weight of a number is the total number of bits set in it.
>>> + */
>>> +#define hweight64(x) generic_hweight64(x)
>>> +#define hweight32(x) generic_hweight32(x)
>>> +#define hweight16(x) generic_hweight16(x)
>>> +#define hweight8(x) generic_hweight8(x)
>>
>> Not using popcnt{b,w,d}, e.g. via a compiler builtin?
>>
> 
> Excellent point. It looks like gcc's __builtin_popcount* family of
> builtins will do what we want here. I suppose the other architectures in
> Xen don't do this because they target toolchains old enough to not have
> these builtins?

Well, on x86 a patch introducing its use is actually pending ("x86: use
POPCNT for hweight<N>() when available").

>>> +    tmp = (*p) & (~0UL >> (BITS_PER_LONG - size));
>>> +    if (tmp == 0UL)        /* Are any bits set? */
>>> +        return result + size;    /* Nope. */
>>> +found:
>>
>> Labels indented by at least one blank please. (More elsewhere.)
> 
> I wasn't aware of this style convention -- so a single blank before the
> label would be correct?

Yes. (There are cases where deeper indentation is neater, but this isn't
one of them.)

Jan