On 27.05.2024 15:27, Jan Beulich wrote:
> On 24.05.2024 22:03, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/bitops.h
>> +++ b/xen/arch/x86/include/asm/bitops.h
>> @@ -432,12 +432,28 @@ static inline int ffsl(unsigned long x)
>>
>> static always_inline unsigned int arch_ffs(unsigned int x)
>> {
>> - int r;
>> + unsigned int r;
>> +
>> + if ( __builtin_constant_p(x > 0) && x > 0 )
>> + {
>> + /* Safe, when the compiler knows that x is nonzero. */
>> + asm ( "bsf %[val], %[res]"
>> + : [res] "=r" (r)
>> + : [val] "rm" (x) );
>> + }
>
> In patch 11 relevant things are all in a single patch, making it easier
> to spot that this is dead code: The sole caller already has a
> __builtin_constant_p(), hence I don't see how the one here could ever
> return true. With that the respective part of the description is then
> questionable, too, I'm afraid: Where did you observe any actual effect
> from this? Or if you did - what am I missing?
Hmm, thinking about it: I suppose that's why you have
__builtin_constant_p(x > 0), not __builtin_constant_p(x). I have to admit
I'm (positively) surprised that the former may return true when the latter
doesn't. Nevertheless I'm inclined to think this deserves a brief comment.
As an aside, to better match the comment inside the if()'s body, how about
if ( __builtin_constant_p(!!x) && x )
? That also may make a little more clear that this isn't just a style
choice, but actually needed for the intended purpose.
Jan
