[PATCH] x86_64: Optimize ffsll function code size.

[Sunil Pandey]

On Wed, Jul 26, 2023 at 10:00 AM Noah Goldstein <goldstein.w.n@gmail.com>
wrote:

> On Wed, Jul 26, 2023 at 11:52 AM Sunil Pandey via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > On Wed, Jul 26, 2023 at 9:38 AM Richard Henderson <
> > richard.henderson@linaro.org> wrote:
> >
> > > On 7/26/23 09:05, Sunil K Pandey via Libc-alpha wrote:
> > > > Ffsll function size is 17 byte, this patch optimizes size to 16 byte.
> > > > Currently ffsll function randomly regress by ~20%, depending on how
> > > > code get aligned.
> > > >
> > > > This patch fixes ffsll function random performance regression.
> > > > ---
> > > >   sysdeps/x86_64/ffsll.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/sysdeps/x86_64/ffsll.c b/sysdeps/x86_64/ffsll.c
> > > > index a1c13d4906..dbded6f0a1 100644
> > > > --- a/sysdeps/x86_64/ffsll.c
> > > > +++ b/sysdeps/x86_64/ffsll.c
> > > > @@ -29,7 +29,7 @@ ffsll (long long int x)
> > > >     long long int tmp;
> > > >
> > > >     asm ("bsfq %2,%0\n"               /* Count low bits in X and
> store
> > > in %1.  */
> > > > -       "cmoveq %1,%0\n"              /* If number was zero, use -1
> as
> > > result.  */
> > > > +       "cmove %k1,%k0\n"     /* If number was zero, use -1 as
> result.
> > > */
> > >
> > > This no longer produces -1, but 0xffffffff in cnt.  However, since the
> > > return type is
> > > 'int', cnt need not be 'long long int' either.  I'm not sure why tmp
> > > exists at all, since
> > > cnt is the only register modified.
> > >
> >
> > Here is the exact assembly produced with this change.
> > ./build-x86_64-linux/string/ffsll.o:     file format elf64-x86-64
> >
> >
> > Disassembly of section .text:
> >
> > 0000000000000000 <ffsll>:
> >    0: ba ff ff ff ff       mov    $0xffffffff,%edx
> >    5: 48 0f bc c7           bsf    %rdi,%rax
> >    9: 0f 44 c2             cmove  %edx,%eax
> >    c: 83 c0 01             add    $0x1,%eax
> >    f: c3                   ret
> >
>
> FWIW it should be:
> ```
> 0000000000000000 <.text>:
>    0: b8 ff ff ff ff        mov    $0xffffffff,%eax
>    5: 48 0f bc c7          bsf    %rdi,%rax
>    9: ff c0                inc    %eax
> ```
>
> And since its in inline asm no reason not to get that.
>

We shouldn't remove cmove because as per Intel BSF instruction manual, if
the content of source operand
 is 0, the content of the destination operand is undefined. Also removing
cmove doesn't provide any perf
advantage.



> >
> > >
> > >
> > > r~
> > >
>