Re: [PATCH] x86/bitops: Replace __scanbit() with a simpler tzcnt()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/11/2026 8:33 am, Jan Beulich wrote:
> On 10.11.2026 16:31, Andrew Cooper wrote:
>> There are 4 remaining users of __scanbit(), and they're capable of dealing
>> with max being BITS_PER_LONG.  Switch to a simpler tzcnt() helper, which also
>> removed some alternatives from behind the scenes.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> (with a suggestion below)
>
>> Supercedes "x86/bitops: Improve code generation for __scanbit()"
> Quite a bit neater indeed.
>
>> --- a/xen/arch/x86/include/asm/bitops.h
>> +++ b/xen/arch/x86/include/asm/bitops.h
>> @@ -320,18 +320,23 @@ extern unsigned int __find_first_zero_bit(
>>  extern unsigned int __find_next_zero_bit(
>>      const unsigned long *addr, unsigned int size, unsigned int offset);
>>  
>> -static always_inline unsigned int __scanbit(unsigned long val, unsigned int 
>> max)
>> +/*
>> + * Search for the first set bit in @x, returning BITS_PER_LONG if @x was 0.
>> + * See arch_ffs() for safety discussions.
>> + */
>> +static always_inline unsigned int tzcnt(unsigned long x)
>>  {
>> -    if ( __builtin_constant_p(max) && max == BITS_PER_LONG )
>> -        alternative_io("bsf %[in],%[out]; cmovz %[max],%k[out]",
>> -                       "rep; bsf %[in],%[out]",
>> -                       X86_FEATURE_BMI1,
>> -                       [out] "=&r" (val),
>> -                       [in] "r" (val), [max] "r" (max));
>> -    else
>> -        asm ( "bsf %1,%0 ; cmovz %2,%k0"
>> -              : "=&r" (val) : "r" (val), "r" (max) );
>> -    return (unsigned int)val;
>> +    unsigned int r;
>> +
>> +    /*
>> +     * The destination register still needs pre-loading for older CPUs which
>> +     * will execute TZCNT as BSF.
>> +     */
>> +    asm ( "tzcnt %[x], %q[r]"
>> +          : [r] "=r" (r)
>> +          : [x] "rm" (x), "[r]" (BITS_PER_LONG) );
>> +
>> +    return r;
>>  }
> By having r and the function return type be unsigned int, when the actual
> insn really is a qword operation, you force call sites to do the zero-
> extension if they want an unsigned long quantity. When both are unsigned
> long, no zero-extension needs performing by the compiler. Truncation to
> unsigned int (or less) won't require any extra insn(s) anyway. While this
> doesn't look as if it would make a difference for present uses, future
> ones may benefit.

This matches the other helpers, so if we want to change something, it
wants doing consistently.

I picked it this way around because it improved code generation for
preloading the destination register.

https://godbolt.org/z/qjoMPh3Kb

The zero-extension is as you say, but as the output is strictly in the
range 0 to 64, it shouldn't ever need to be in a long as opposed to an int.


What I don't understand is why the zero-extension is being inserted. 
There's no way to produce a 32bit value without the upper bits having
been cleared, and if you write a 64bit value but lie to the compiler
about it being 32bit then that's your own fault for having lied about
your constraints.

~Andrew