Re: [PATCH] bitops/32: Convert variable_ffs() and fls() zero-case handling to C

["H. Peter Anvin" <hpa@zytor.com>]

On April 28, 2025 12:14:40 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Ingo Molnar <mingo@kernel.org> wrote:
>
>> And once we remove 486, I think we can do the optimization below to 
>> just assume the output doesn't get clobbered by BS*L in the 
>> zero-case, right?
>> 
>> In the text size space it's a substantial optimization on x86-32 
>> defconfig:
>> 
>>         text	   data	       bss	     dec	    hex	filename
>>   16,577,728    7598826    1744896      25921450        18b87aa vmlinux.vanilla      # CMOV+BS*L
>>   16,577,908	7598838	   1744896	25921642	18b886a	vmlinux.linus_patch  # if()+BS*L
>>   16,573,568	7602922	   1744896	25921386	18b876a	vmlinux.noclobber    # BS*L
>
>And BTW, *that* is a price that all of non-486 x86-32 was paying for 
>486 support...
>
>And, just out of intellectual curiosity, I also tried to measure the 
>code generation price of the +1 standards-quirk in the fls()/ffs() 
>interface as well:
>
>         text	   data	       bss	     dec	    hex	filename
>   16,577,728   7598826    1744896      25921450        18b87aa vmlinux.vanilla      # CMOV+BS*L
>   16,577,908	7598838	   1744896	25921642	18b886a	vmlinux.linus_patch  # if()+BS*L
>   16,573,568	7602922	   1744896	25921386	18b876a	vmlinux.noclobber    # BS*L
>   ..........
>   16,573,552	7602922	   1744896	25921370	18b875a	vmlinux.broken       # BROKEN: 0 baseline instead of 1
>
>... and unless I messed up the patch, it seems to have a surprisingly 
>low impact - maybe because the compiler can amortize its cost by 
>adjusting all dependent code mostly at build time, so the +1 doesn't 
>end up being generated most of the time?
>
>Thanks,
>
>	Ingo
>
>===============================>
>
>This broken patch is broken: it intentionally breaks the ffs()/fls() 
>interface in an attempt to measure the code generation effects of 
>interface details.
>
>NOT-Signed-off-by: <anyone@anywhere.anytime>
>---
> arch/x86/include/asm/bitops.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
>index e3e94a806656..21707696bafe 100644
>--- a/arch/x86/include/asm/bitops.h
>+++ b/arch/x86/include/asm/bitops.h
>@@ -318,7 +318,7 @@ static __always_inline int variable_ffs(int x)
> 	    : "=r" (r)
> 	    : ASM_INPUT_RM (x), "0" (-1));
> 
>-	return r + 1;
>+	return r;
> }
> 
> /**
>@@ -362,7 +362,7 @@ static __always_inline int fls(unsigned int x)
> 	    : "=r" (r)
> 	    : ASM_INPUT_RM (x), "0" (-1));
> 
>-	return r + 1;
>+	return r;
> }
> 
> /**

My recollection was that you can't assume that even for 586; that it is only safe for 686, but it has been a long time...