[PATCH v2] string: Use builtins for ffs and ffsll

[PATCH v2] string: Use builtins for ffs and ffsll

[Adhemerval Zanella]

It allows to remove a lot of arch-specific implementations.

Checked on x86_64, aarch64, powerpc64.
---
 string/ffs.c                            |  6 +-
 string/ffsll.c                          | 10 ++-
 sysdeps/aarch64/math-use-builtins-ffs.h |  2 +
 sysdeps/alpha/alphaev67/ffs.S           | 51 --------------
 sysdeps/alpha/alphaev67/ffsll.S         | 44 ------------
 sysdeps/alpha/ffs.S                     | 90 -------------------------
 sysdeps/alpha/ffsll.S                   |  1 -
 sysdeps/alpha/math-use-builtins-ffs.h   |  7 ++
 sysdeps/arc/math-use-builtins-ffs.h     |  2 +
 sysdeps/arm/armv6t2/ffs.S               | 36 ----------
 sysdeps/arm/armv6t2/ffsll.S             | 50 --------------
 sysdeps/arm/math-use-builtins-ffs.h     |  2 +
 sysdeps/generic/math-use-builtins-ffs.h |  2 +
 sysdeps/generic/math-use-builtins.h     |  1 +
 sysdeps/i386/ffs.c                      | 49 --------------
 sysdeps/i386/i686/ffs.c                 | 47 -------------
 sysdeps/i386/math-use-builtins-ffs.h    |  2 +
 sysdeps/ia64/math-use-builtins-ffs.h    |  2 +
 sysdeps/m68k/ffs.c                      | 46 -------------
 sysdeps/m68k/math-use-builtins-ffs.h    |  7 ++
 sysdeps/powerpc/ffs.c                   | 46 -------------
 sysdeps/powerpc/math-use-builtins-ffs.h |  6 ++
 sysdeps/powerpc/powerpc64/ffsll.c       | 36 ----------
 sysdeps/s390/ffs.c                      | 69 -------------------
 sysdeps/x86_64/ffs.c                    | 38 -----------
 sysdeps/x86_64/ffsll.c                  | 41 -----------
 sysdeps/x86_64/math-use-builtins-ffs.h  |  2 +
 sysdeps/x86_64/x32/ffs.c                |  4 --
 28 files changed, 48 insertions(+), 651 deletions(-)
 create mode 100644 sysdeps/aarch64/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/alpha/alphaev67/ffs.S
 delete mode 100644 sysdeps/alpha/alphaev67/ffsll.S
 delete mode 100644 sysdeps/alpha/ffs.S
 delete mode 100644 sysdeps/alpha/ffsll.S
 create mode 100644 sysdeps/alpha/math-use-builtins-ffs.h
 create mode 100644 sysdeps/arc/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/arm/armv6t2/ffs.S
 delete mode 100644 sysdeps/arm/armv6t2/ffsll.S
 create mode 100644 sysdeps/arm/math-use-builtins-ffs.h
 create mode 100644 sysdeps/generic/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/i386/ffs.c
 delete mode 100644 sysdeps/i386/i686/ffs.c
 create mode 100644 sysdeps/i386/math-use-builtins-ffs.h
 create mode 100644 sysdeps/ia64/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/m68k/ffs.c
 create mode 100644 sysdeps/m68k/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/powerpc/ffs.c
 create mode 100644 sysdeps/powerpc/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/powerpc/powerpc64/ffsll.c
 delete mode 100644 sysdeps/s390/ffs.c
 delete mode 100644 sysdeps/x86_64/ffs.c
 delete mode 100644 sysdeps/x86_64/ffsll.c
 create mode 100644 sysdeps/x86_64/math-use-builtins-ffs.h
 delete mode 100644 sysdeps/x86_64/x32/ffs.c

diff --git a/string/ffs.c b/string/ffs.c
index bf58f0b7ca..8302ae8420 100644
--- a/string/ffs.c
+++ b/string/ffs.c
@@ -18,13 +18,16 @@
 #include <limits.h>
 #define ffsl __something_else
 #include <string.h>
-
 #undef	ffs
+#include <math-use-builtins.h>
 
 /* Find the first bit set in I.  */
 int
 __ffs (int i)
 {
+#if USE_FFS_BUILTIN
+  return __builtin_ffs (i);
+#else
   static const unsigned char table[] =
     {
       0,1,2,2,3,3,3,3,4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,5,
@@ -42,6 +45,7 @@ __ffs (int i)
   a = x <= 0xffff ? (x <= 0xff ? 0 : 8) : (x <= 0xffffff ?  16 : 24);
 
   return table[x >> a] + a;
+#endif
 }
 weak_alias (__ffs, ffs)
 libc_hidden_def (__ffs)
diff --git a/string/ffsll.c b/string/ffsll.c
index 0cc461a1cf..2315fe1bd7 100644
--- a/string/ffsll.c
+++ b/string/ffsll.c
@@ -18,20 +18,26 @@
 #include <limits.h>
 #define ffsl __something_else
 #include <string.h>
-
 #undef	ffsll
+#include <math-use-builtins.h>
+#include <libc-diag.h>
 
 /* Find the first bit set in I.  */
 int
-ffsll (long long int i)
+__ffsll (long long int i)
 {
+#if USE_FFSLL_BUILTIN
+  return __builtin_ffsll (i);
+#else
   unsigned long long int x = i & -i;
 
   if (x <= 0xffffffff)
     return ffs (i);
   else
     return 32 + ffs (i >> 32);
+#endif
 }
+weak_alias (__ffsll, ffsll)
 
 #if ULONG_MAX != UINT_MAX
 #undef ffsl
diff --git a/sysdeps/aarch64/math-use-builtins-ffs.h b/sysdeps/aarch64/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..a83bb15414
--- /dev/null
+++ b/sysdeps/aarch64/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   1
+#define USE_FFSLL_BUILTIN 1
diff --git a/sysdeps/alpha/alphaev67/ffs.S b/sysdeps/alpha/alphaev67/ffs.S
deleted file mode 100644
index 48361bd8c5..0000000000
--- a/sysdeps/alpha/alphaev67/ffs.S
+++ /dev/null
@@ -1,51 +0,0 @@
-/* Copyright (C) 2000-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-/* Finds the first bit set in an integer.  */
-
-#include <sysdep.h>
-
-	.arch ev6
-	.set noreorder
-	.set noat
-
-
-ENTRY(__ffs)
-#ifdef PROF
-	ldgp	gp, 0(pv)
-	lda	AT, _mcount
-	jsr	AT, (AT), _mcount
-	.prologue 1
-#else
-	.prologue 0
-#endif
-
-	zap	$16, 0xF0, $16
-	cttz	$16, $0
-	addq	$0, 1, $0
-	cmoveq	$16, 0, $0
-
-	nop
-	nop
-	nop
-	ret
-
-END(__ffs)
-
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
diff --git a/sysdeps/alpha/alphaev67/ffsll.S b/sysdeps/alpha/alphaev67/ffsll.S
deleted file mode 100644
index 52f406ec32..0000000000
--- a/sysdeps/alpha/alphaev67/ffsll.S
+++ /dev/null
@@ -1,44 +0,0 @@
-/* Copyright (C) 2000-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-/* Finds the first bit set in a long.  */
-
-#include <sysdep.h>
-
-	.arch ev6
-	.set noreorder
-	.set noat
-
-ENTRY(ffsl)
-#ifdef PROF
-	ldgp	gp, 0(pv)
-	lda	AT, _mcount
-	jsr	AT, (AT), _mcount
-	.prologue 1
-#else
-	.prologue 0
-#endif
-
-	cttz	$16, $0
-	addq	$0, 1, $0
-	cmoveq	$16, 0, $0
-	ret
-
-END(ffsl)
-
-weak_extern (ffsl)
-weak_alias (ffsl, ffsll)
diff --git a/sysdeps/alpha/ffs.S b/sysdeps/alpha/ffs.S
deleted file mode 100644
index 047fd60352..0000000000
--- a/sysdeps/alpha/ffs.S
+++ /dev/null
@@ -1,90 +0,0 @@
-/* Copyright (C) 1996-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-/* Finds the first bit set in an integer.  Optimized for the Alpha
-   architecture.  */
-
-#include <sysdep.h>
-
-	.set noreorder
-	.set noat
-
-
-ENTRY(__ffs)
-#ifdef PROF
-	ldgp	gp, 0(pv)
-	lda	AT, _mcount
-	jsr	AT, (AT), _mcount
-	.prologue 1
-	zap	$16, 0xF0, $16
-	br	$ffsl..ng
-#else
-	.prologue 0
-	zap	$16, 0xF0, $16
-	# FALLTHRU
-#endif
-END(__ffs)
-
-	.align 4
-ENTRY(ffsl)
-#ifdef PROF
-	ldgp	gp, 0(pv)
-	lda	AT, _mcount
-	jsr	AT, (AT), _mcount
-	.prologue 1
-$ffsl..ng:
-#else
-	.prologue 0
-#endif
-	not	$16, $1		# e0    :
-	ldi	$2, -1		# .. e1 :
-	cmpbge	$1, $2, $3	# e0    : bit N == 1 for byte N == 0
-	clr	$0		# .. e1 :
-	addq	$3, 1, $4	# e0    :
-	bic	$4, $3, $3	# e1    : bit N == 1 for first byte N != 0
-	and	$3, 0xF0, $4	# e0    :
-	and	$3, 0xCC, $5	# .. e1 :
-	and	$3, 0xAA, $6	# e0    :
-	cmovne	$4, 4, $0	# .. e1 :
-	cmovne	$5, 2, $5	# e0    :
-	cmovne  $6, 1, $6	# .. e1 :
-	addl	$0, $5, $0	# e0    :
-	addl	$0, $6, $0	# e1    : $0 == N
-	extbl	$16, $0, $1	# e0    : $1 == byte N
-	ldi	$2, 1		# .. e1 :
-	negq	$1, $3		# e0    :
-	and	$3, $1, $3	# e1    : bit N == least bit set of byte N
-	and	$3, 0xF0, $4	# e0    :
-	and	$3, 0xCC, $5	# .. e1 :
-	and	$3, 0xAA, $6	# e0    :
-	cmovne	$4, 5, $2	# .. e1 :
-	cmovne	$5, 2, $5	# e0    :
-	cmovne	$6, 1, $6	# .. e1 :
-	s8addl	$0, $2, $0	# e0    : mult byte ofs by 8 and sum
-	addl	$5, $6, $5	# .. e1 :
-	addl	$0, $5, $0	# e0    :
-	nop			# .. e1 :
-	cmoveq	$16, 0, $0	# e0    : trap input == 0 case.
-	ret			# .. e1 : 18
-
-END(ffsl)
-
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
-weak_extern (ffsl)
-weak_alias (ffsl, ffsll)
diff --git a/sysdeps/alpha/ffsll.S b/sysdeps/alpha/ffsll.S
deleted file mode 100644
index b2f46d899c..0000000000
--- a/sysdeps/alpha/ffsll.S
+++ /dev/null
@@ -1 +0,0 @@
-/* This function is defined in ffs.S.  */
diff --git a/sysdeps/alpha/math-use-builtins-ffs.h b/sysdeps/alpha/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..9925e374ac
--- /dev/null
+++ b/sysdeps/alpha/math-use-builtins-ffs.h
@@ -0,0 +1,7 @@
+#ifdef __alpha_cix__
+# define USE_FFS_BUILTIN   1
+# define USE_FFSLL_BUILTIN 1
+#else
+# define USE_FFS_BUILTIN   0
+# define USE_FFSLL_BUILTIN 0
+#endif
diff --git a/sysdeps/arc/math-use-builtins-ffs.h b/sysdeps/arc/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..c0f108c264
--- /dev/null
+++ b/sysdeps/arc/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   1
+#define USE_FFSLL_BUILTIN 0
diff --git a/sysdeps/arm/armv6t2/ffs.S b/sysdeps/arm/armv6t2/ffs.S
deleted file mode 100644
index 870957a4bb..0000000000
--- a/sysdeps/arm/armv6t2/ffs.S
+++ /dev/null
@@ -1,36 +0,0 @@
-/* ffs -- find first set bit in an int, from least significant end.
-   Copyright (C) 2013-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <sysdep.h>
-
-	.syntax unified
-	.text
-
-ENTRY (__ffs)
-	cmp	r0, #0
-	rbit	r0, r0
-	itt	ne
-	clzne	r0, r0
-	addne	r0, r0, #1
-	bx	lr
-END (__ffs)
-
-weak_alias (__ffs, ffs)
-weak_alias (__ffs, ffsl)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
diff --git a/sysdeps/arm/armv6t2/ffsll.S b/sysdeps/arm/armv6t2/ffsll.S
deleted file mode 100644
index bc3cbf81b0..0000000000
--- a/sysdeps/arm/armv6t2/ffsll.S
+++ /dev/null
@@ -1,50 +0,0 @@
-/* ffsll -- find first set bit in a long long, from least significant end.
-   Copyright (C) 2013-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <sysdep.h>
-
-	.syntax unified
-	.text
-
-ENTRY (ffsll)
-	@ If low part is 0, operate on the high part.  Ensure that the
-	@ word on which we operate is in r0.  Set r2 to the bit offset
-	@ of the word being considered.  Set the flags for the word
-	@ being operated on.
-#ifdef __ARMEL__
-	cmp	r0, #0
-	itee	ne
-	movne	r2, #0
-	moveq	r2, #32
-	movseq	r0, r1
-#else
-	cmp	r1, #0
-	ittee	ne
-	movne	r2, #0
-	movne	r0, r1
-	moveq	r2, #32
-	cmpeq	r0, #0
-#endif
-	@ Perform the ffs on r0.
-	rbit	r0, r0
-	ittt	ne
-	clzne	r0, r0
-	addne	r2, r2, #1
-	addne	r0, r0, r2
-	bx	lr
-END (ffsll)
diff --git a/sysdeps/arm/math-use-builtins-ffs.h b/sysdeps/arm/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..c0f108c264
--- /dev/null
+++ b/sysdeps/arm/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   1
+#define USE_FFSLL_BUILTIN 0
diff --git a/sysdeps/generic/math-use-builtins-ffs.h b/sysdeps/generic/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..add8537470
--- /dev/null
+++ b/sysdeps/generic/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   0
+#define USE_FFSLL_BUILTIN 0
diff --git a/sysdeps/generic/math-use-builtins.h b/sysdeps/generic/math-use-builtins.h
index 04b1d0d5ab..038eb2c523 100644
--- a/sysdeps/generic/math-use-builtins.h
+++ b/sysdeps/generic/math-use-builtins.h
@@ -40,5 +40,6 @@
 #include <math-use-builtins-lrint.h>
 #include <math-use-builtins-llrint.h>
 #include <math-use-builtins-logb.h>
+#include <math-use-builtins-ffs.h>
 
 #endif /* MATH_USE_BUILTINS_H  */
diff --git a/sysdeps/i386/ffs.c b/sysdeps/i386/ffs.c
deleted file mode 100644
index b8e798420b..0000000000
--- a/sysdeps/i386/ffs.c
+++ /dev/null
@@ -1,49 +0,0 @@
-/* ffs -- find first set bit in a word, counted from least significant end.
-   For Intel 80x86, x>=3.
-   This file is part of the GNU C Library.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#define ffsl __something_else
-#include <string.h>
-
-#undef	ffs
-
-#ifdef	__GNUC__
-
-int
-__ffs (int x)
-{
-  int cnt;
-  int tmp;
-
-  asm ("xorl %0,%0\n"		/* Set CNT to zero.  */
-       "bsfl %2,%1\n"		/* Count low bits in X and store in %1.  */
-       "jz 1f\n"		/* Jump if OK, i.e. X was non-zero.  */
-       "leal 1(%1),%0\n"	/* Return bsfl-result plus one on %0.  */
-       "1:" : "=&a" (cnt), "=r" (tmp) : "rm" (x));
-
-  return cnt;
-}
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
-#undef ffsl
-weak_alias (__ffs, ffsl)
-
-#else
-#include <string/ffs.c>
-#endif
diff --git a/sysdeps/i386/i686/ffs.c b/sysdeps/i386/i686/ffs.c
deleted file mode 100644
index a522077cfe..0000000000
--- a/sysdeps/i386/i686/ffs.c
+++ /dev/null
@@ -1,47 +0,0 @@
-/* ffs -- find first set bit in a word, counted from least significant end.
-   For Intel 80x86, x>=6.
-   This file is part of the GNU C Library.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#define ffsl __something_else
-#include <string.h>
-
-#undef	ffs
-
-#ifdef	__GNUC__
-
-int
-__ffs (int x)
-{
-  int cnt;
-  int tmp;
-
-  asm ("bsfl %2,%0\n"		/* Count low bits in X and store in %1.  */
-       "cmovel %1,%0\n"		/* If number was zero, use -1 as result.  */
-       : "=&r" (cnt), "=r" (tmp) : "rm" (x), "1" (-1));
-
-  return cnt + 1;
-}
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
-#undef ffsl
-weak_alias (__ffs, ffsl)
-
-#else
-#include <string/ffs.c>
-#endif
diff --git a/sysdeps/i386/math-use-builtins-ffs.h b/sysdeps/i386/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..c0f108c264
--- /dev/null
+++ b/sysdeps/i386/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   1
+#define USE_FFSLL_BUILTIN 0
diff --git a/sysdeps/ia64/math-use-builtins-ffs.h b/sysdeps/ia64/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..a83bb15414
--- /dev/null
+++ b/sysdeps/ia64/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   1
+#define USE_FFSLL_BUILTIN 1
diff --git a/sysdeps/m68k/ffs.c b/sysdeps/m68k/ffs.c
deleted file mode 100644
index 4415bb0cff..0000000000
--- a/sysdeps/m68k/ffs.c
+++ /dev/null
@@ -1,46 +0,0 @@
-/* ffs -- find first set bit in a word, counted from least significant end.
-   For mc68020, mc68030, mc68040.
-   This file is part of the GNU C Library.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#define ffsl __something_else
-#include <string.h>
-
-#undef	ffs
-
-#if	defined (__GNUC__) && defined (__mc68020__)
-
-int
-__ffs (int x)
-{
-  int cnt;
-
-  asm ("bfffo %1{#0:#0},%0" : "=d" (cnt) : "dm" (x & -x));
-
-  return 32 - cnt;
-}
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
-#undef ffsl
-weak_alias (__ffs, ffsl)
-
-#else
-
-#include <string/ffs.c>
-
-#endif
diff --git a/sysdeps/m68k/math-use-builtins-ffs.h b/sysdeps/m68k/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..4e1d994453
--- /dev/null
+++ b/sysdeps/m68k/math-use-builtins-ffs.h
@@ -0,0 +1,7 @@
+#if defined __mc68020__ || defined __mc68030__ || defined __mc68040__ \
+    || defined __mc68060__
+# define USE_FFS_BUILTIN  1
+#else
+# define USE_FFS_BUILTIN  0
+#endif
+#define USE_FFSLL_BUILTIN 0
diff --git a/sysdeps/powerpc/ffs.c b/sysdeps/powerpc/ffs.c
deleted file mode 100644
index 0a046dab99..0000000000
--- a/sysdeps/powerpc/ffs.c
+++ /dev/null
@@ -1,46 +0,0 @@
-/* Find first set bit in a word, counted from least significant end.
-   For PowerPC.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#define ffsl __something_else
-#include <limits.h>
-#include <string.h>
-
-#undef	ffs
-
-#ifdef 	__GNUC__
-
-int
-__ffs (int x)
-{
-  int cnt;
-
-  asm ("cntlzw %0,%1" : "=r" (cnt) : "r" (x & -x));
-  return 32 - cnt;
-}
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
-#if ULONG_MAX == UINT_MAX
-#undef ffsl
-weak_alias (__ffs, ffsl)
-#endif
-
-#else
-#include <string/ffs.c>
-#endif
diff --git a/sysdeps/powerpc/math-use-builtins-ffs.h b/sysdeps/powerpc/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..7b1d401823
--- /dev/null
+++ b/sysdeps/powerpc/math-use-builtins-ffs.h
@@ -0,0 +1,6 @@
+#define USE_FFS_BUILTIN    1
+#ifdef __powerpc64__
+# define USE_FFSLL_BUILTIN 1
+#else
+# define USE_FFSLL_BUILTIN 0
+#endif
diff --git a/sysdeps/powerpc/powerpc64/ffsll.c b/sysdeps/powerpc/powerpc64/ffsll.c
deleted file mode 100644
index d17cdd1c6b..0000000000
--- a/sysdeps/powerpc/powerpc64/ffsll.c
+++ /dev/null
@@ -1,36 +0,0 @@
-/* Find first set bit in a word, counted from least significant end.
-   For PowerPC.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#define ffsl __something_else
-#include <limits.h>
-#include <string.h>
-
-#undef	ffs
-
-int
-__ffsll (long long int x)
-{
-  int cnt;
-
-  asm ("cntlzd %0,%1" : "=r" (cnt) : "r" (x & -x));
-  return 64 - cnt;
-}
-weak_alias (__ffsll, ffsll)
-#undef ffsl
-weak_alias (__ffsll, ffsl)
diff --git a/sysdeps/s390/ffs.c b/sysdeps/s390/ffs.c
deleted file mode 100644
index 7627bd3346..0000000000
--- a/sysdeps/s390/ffs.c
+++ /dev/null
@@ -1,69 +0,0 @@
-/* ffs -- find first set bit in a word, counted from least significant end.
-   S/390 version.
-   Copyright (C) 2000-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <limits.h>
-#define ffsl __something_else
-#include <string.h>
-
-#undef	ffs
-
-/* ffs: find first bit set. This is defined the same way as
-   the libc and compiler builtin ffs routines, therefore
-   differs in spirit from the above ffz (man ffs).  */
-
-int
-__ffs (int x)
-{
-	int r;
-
-	if (x == 0)
-	  return 0;
-	__asm__("    lr	  %%r1,%1\n"
-		"    sr	  %0,%0\n"
-		"    tml  %%r1,0xFFFF\n"
-		"    jnz  0f\n"
-		"    ahi  %0,16\n"
-		"    srl  %%r1,16\n"
-		"0:  tml  %%r1,0x00FF\n"
-		"    jnz  1f\n"
-		"    ahi  %0,8\n"
-		"    srl  %%r1,8\n"
-		"1:  tml  %%r1,0x000F\n"
-		"    jnz  2f\n"
-		"    ahi  %0,4\n"
-		"    srl  %%r1,4\n"
-		"2:  tml  %%r1,0x0003\n"
-		"    jnz  3f\n"
-		"    ahi  %0,2\n"
-		"    srl  %%r1,2\n"
-		"3:  tml  %%r1,0x0001\n"
-		"    jnz  4f\n"
-		"    ahi  %0,1\n"
-		"4:"
-		: "=&d" (r) : "d" (x) : "cc", "1" );
-	return r+1;
-}
-
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
-#if ULONG_MAX == UINT_MAX
-#undef ffsl
-weak_alias (__ffs, ffsl)
-#endif
diff --git a/sysdeps/x86_64/ffs.c b/sysdeps/x86_64/ffs.c
deleted file mode 100644
index a31202969e..0000000000
--- a/sysdeps/x86_64/ffs.c
+++ /dev/null
@@ -1,38 +0,0 @@
-/* ffs -- find first set bit in a word, counted from least significant end.
-   For AMD x86-64.
-   This file is part of the GNU C Library.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <string.h>
-
-#undef	ffs
-
-int
-__ffs (int x)
-{
-  int cnt;
-  int tmp;
-
-  asm ("bsfl %2,%0\n"		/* Count low bits in X and store in %1.  */
-       "cmovel %1,%0\n"		/* If number was zero, use -1 as result.  */
-       : "=&r" (cnt), "=r" (tmp) : "rm" (x), "1" (-1));
-
-  return cnt + 1;
-}
-weak_alias (__ffs, ffs)
-libc_hidden_def (__ffs)
-libc_hidden_builtin_def (ffs)
diff --git a/sysdeps/x86_64/ffsll.c b/sysdeps/x86_64/ffsll.c
deleted file mode 100644
index a1c13d4906..0000000000
--- a/sysdeps/x86_64/ffsll.c
+++ /dev/null
@@ -1,41 +0,0 @@
-/* ffsll -- find first set bit in a word, counted from least significant end.
-   For AMD x86-64.
-   This file is part of the GNU C Library.
-   Copyright (C) 1991-2023 Free Software Foundation, Inc.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#define ffsl __something_else
-#include <string.h>
-
-#undef	ffsll
-
-int
-ffsll (long long int x)
-{
-  long long int cnt;
-  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.  */
-       : "=&r" (cnt), "=r" (tmp) : "rm" (x), "1" (-1));
-
-  return cnt + 1;
-}
-
-#ifndef __ILP32__
-#undef	ffsl
-weak_alias (ffsll, ffsl)
-#endif
diff --git a/sysdeps/x86_64/math-use-builtins-ffs.h b/sysdeps/x86_64/math-use-builtins-ffs.h
new file mode 100644
index 0000000000..a83bb15414
--- /dev/null
+++ b/sysdeps/x86_64/math-use-builtins-ffs.h
@@ -0,0 +1,2 @@
+#define USE_FFS_BUILTIN   1
+#define USE_FFSLL_BUILTIN 1
diff --git a/sysdeps/x86_64/x32/ffs.c b/sysdeps/x86_64/x32/ffs.c
deleted file mode 100644
index fa7de8b887..0000000000
--- a/sysdeps/x86_64/x32/ffs.c
+++ /dev/null
@@ -1,4 +0,0 @@
-#define ffsl __something_else
-#include <sysdeps/x86_64/ffs.c>
-#undef ffsl
-weak_alias (__ffs, ffsl)
-- 
2.34.1

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Richard Henderson]

On 8/24/23 10:13, Adhemerval Zanella via Libc-alpha wrote:
> diff --git a/string/ffsll.c b/string/ffsll.c
> index 0cc461a1cf..2315fe1bd7 100644
> --- a/string/ffsll.c
> +++ b/string/ffsll.c
> @@ -18,20 +18,26 @@
>   #include <limits.h>
>   #define ffsl __something_else
>   #include <string.h>
> -
>   #undef	ffsll
> +#include <math-use-builtins.h>
> +#include <libc-diag.h>
>   
>   /* Find the first bit set in I.  */
>   int
> -ffsll (long long int i)
> +__ffsll (long long int i)
>   {
> +#if USE_FFSLL_BUILTIN
> +  return __builtin_ffsll (i);
> +#else
>     unsigned long long int x = i & -i;
>   
>     if (x <= 0xffffffff)
>       return ffs (i);
>     else
>       return 32 + ffs (i >> 32);
> +#endif
>   }

It seems like you also need

#if USE_FFS_BUILTIN
# define ffs  __builtin_ffs
#endif

in order to avoid a function call for the 32-bit host case, e.g.


> diff --git a/sysdeps/arm/armv6t2/ffsll.S b/sysdeps/arm/armv6t2/ffsll.S
> deleted file mode 100644
> index bc3cbf81b0..0000000000
> --- a/sysdeps/arm/armv6t2/ffsll.S
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/* ffsll -- find first set bit in a long long, from least significant end.
> -   Copyright (C) 2013-2023 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library.  If not, see
> -   <https://www.gnu.org/licenses/>.  */
> -
> -#include <sysdep.h>
> -
> -	.syntax unified
> -	.text
> -
> -ENTRY (ffsll)
> -	@ If low part is 0, operate on the high part.  Ensure that the
> -	@ word on which we operate is in r0.  Set r2 to the bit offset
> -	@ of the word being considered.  Set the flags for the word
> -	@ being operated on.
> -#ifdef __ARMEL__
> -	cmp	r0, #0
> -	itee	ne
> -	movne	r2, #0
> -	moveq	r2, #32
> -	movseq	r0, r1
> -#else
> -	cmp	r1, #0
> -	ittee	ne
> -	movne	r2, #0
> -	movne	r0, r1
> -	moveq	r2, #32
> -	cmpeq	r0, #0
> -#endif
> -	@ Perform the ffs on r0.
> -	rbit	r0, r0
> -	ittt	ne
> -	clzne	r0, r0
> -	addne	r2, r2, #1
> -	addne	r0, r0, r2
> -	bx	lr
> -END (ffsll)
> diff --git a/sysdeps/arm/math-use-builtins-ffs.h b/sysdeps/arm/math-use-builtins-ffs.h
> new file mode 100644
> index 0000000000..c0f108c264
> --- /dev/null
> +++ b/sysdeps/arm/math-use-builtins-ffs.h
> @@ -0,0 +1,2 @@
> +#define USE_FFS_BUILTIN   1
> +#define USE_FFSLL_BUILTIN 0

Here.  Also, this file is misplaced wrt the armv6t2 files you're removing.

This will work, I suppose, but you'll get the libgcc implementation.
If you're willing to settle for that, you might as well drop all of
this extra use-builtins stuff and unconditionally use __builtin_ffs*.


r~

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Adhemerval Zanella Netto]

On 24/08/23 16:44, Richard Henderson wrote:
> On 8/24/23 10:13, Adhemerval Zanella via Libc-alpha wrote:
>> diff --git a/string/ffsll.c b/string/ffsll.c
>> index 0cc461a1cf..2315fe1bd7 100644
>> --- a/string/ffsll.c
>> +++ b/string/ffsll.c
>> @@ -18,20 +18,26 @@
>>   #include <limits.h>
>>   #define ffsl __something_else
>>   #include <string.h>
>> -
>>   #undef    ffsll
>> +#include <math-use-builtins.h>
>> +#include <libc-diag.h>
>>     /* Find the first bit set in I.  */
>>   int
>> -ffsll (long long int i)
>> +__ffsll (long long int i)
>>   {
>> +#if USE_FFSLL_BUILTIN
>> +  return __builtin_ffsll (i);
>> +#else
>>     unsigned long long int x = i & -i;
>>       if (x <= 0xffffffff)
>>       return ffs (i);
>>     else
>>       return 32 + ffs (i >> 32);
>> +#endif
>>   }
> 
> It seems like you also need
> 
> #if USE_FFS_BUILTIN
> # define ffs  __builtin_ffs
> #endif
> 
> in order to avoid a function call for the 32-bit host case, e.g.

I did analyze it and for 32 bits targets that do define USE_FFSLL_BUILTIN 
to 1 (armv6t2, arc, i386, m68k/mc68020, and powerpc) gcc will lower ffs to 
__builin_ffs. For instance:

armv7a-linux-gnueabihf$ arm-linux-gnueabihf-objdump -d string/ffsll.os
[...]
00000000 <__ffsll>:
   0:   e2703000        rsbs    r3, r0, #0
   4:   e2e13000        rsc     r3, r1, #0
   8:   e1130001        tst     r3, r1
   c:   1a000005        bne     28 <__ffsll+0x28>
  10:   e3500000        cmp     r0, #0
  14:   e6ff0f30        rbit    r0, r0
  18:   e16f0f10        clz     r0, r0
  1c:   03e00000        mvneq   r0, #0
  20:   e2800001        add     r0, r0, #1
  24:   e12fff1e        bx      lr
  28:   e3510000        cmp     r1, #0
  2c:   e6ff1f31        rbit    r1, r1
  30:   e16f1f11        clz     r1, r1
  34:   03e01000        mvneq   r1, #0
  38:   e2810021        add     r0, r1, #33     ; 0x21
  3c:   e12fff1e        bx      lr

I also check that  arc, i386, m68k/mc68020, and powerpc also lower ffs
correctly. So there is no need to this extra define.

> 
> 
>> diff --git a/sysdeps/arm/armv6t2/ffsll.S b/sysdeps/arm/armv6t2/ffsll.S
>> deleted file mode 100644
>> index bc3cbf81b0..0000000000
>> --- a/sysdeps/arm/armv6t2/ffsll.S
>> +++ /dev/null
>> @@ -1,50 +0,0 @@
>> -/* ffsll -- find first set bit in a long long, from least significant end.
>> -   Copyright (C) 2013-2023 Free Software Foundation, Inc.
>> -   This file is part of the GNU C Library.
>> -
>> -   The GNU C Library is free software; you can redistribute it and/or
>> -   modify it under the terms of the GNU Lesser General Public
>> -   License as published by the Free Software Foundation; either
>> -   version 2.1 of the License, or (at your option) any later version.
>> -
>> -   The GNU C Library is distributed in the hope that it will be useful,
>> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> -   Lesser General Public License for more details.
>> -
>> -   You should have received a copy of the GNU Lesser General Public
>> -   License along with the GNU C Library.  If not, see
>> -   <https://www.gnu.org/licenses/>.  */
>> -
>> -#include <sysdep.h>
>> -
>> -    .syntax unified
>> -    .text
>> -
>> -ENTRY (ffsll)
>> -    @ If low part is 0, operate on the high part.  Ensure that the
>> -    @ word on which we operate is in r0.  Set r2 to the bit offset
>> -    @ of the word being considered.  Set the flags for the word
>> -    @ being operated on.
>> -#ifdef __ARMEL__
>> -    cmp    r0, #0
>> -    itee    ne
>> -    movne    r2, #0
>> -    moveq    r2, #32
>> -    movseq    r0, r1
>> -#else
>> -    cmp    r1, #0
>> -    ittee    ne
>> -    movne    r2, #0
>> -    movne    r0, r1
>> -    moveq    r2, #32
>> -    cmpeq    r0, #0
>> -#endif
>> -    @ Perform the ffs on r0.
>> -    rbit    r0, r0
>> -    ittt    ne
>> -    clzne    r0, r0
>> -    addne    r2, r2, #1
>> -    addne    r0, r0, r2
>> -    bx    lr
>> -END (ffsll)
>> diff --git a/sysdeps/arm/math-use-builtins-ffs.h b/sysdeps/arm/math-use-builtins-ffs.h
>> new file mode 100644
>> index 0000000000..c0f108c264
>> --- /dev/null
>> +++ b/sysdeps/arm/math-use-builtins-ffs.h
>> @@ -0,0 +1,2 @@
>> +#define USE_FFS_BUILTIN   1
>> +#define USE_FFSLL_BUILTIN 0
> 
> Here.  Also, this file is misplaced wrt the armv6t2 files you're removing.

Ack.

> 
> This will work, I suppose, but you'll get the libgcc implementation.
> If you're willing to settle for that, you might as well drop all of
> this extra use-builtins stuff and unconditionally use __builtin_ffs*.

It is not that simple, because depending of the target gcc will lower 
__builtin_ffs to fss instead of libgcc (riscv64 for instance).

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Richard Henderson]

On 8/30/23 05:51, Wilco Dijkstra wrote:
> To be clear I'm fine with a follow-on patch that removes these defines again.
> It's just that in previous iterations several people wanted more accurate settings
> for these defines, and it seems that could go on...

I'm happy for the defines not to exist.

I had simply been pointing out that removing the asm from armv6t2 and adding the defines 
to arm was a change, and therefore not a trivial replacement.


r~

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Adhemerval Zanella Netto]

On 30/08/23 09:51, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
>> I would say we are discussing two different things here.  My patch intention
>> is to remove the multiple ffs/ffsll arch-specific and use compiler to generate
>> the optimized instruction were possible.  The idea is not to leverage libgcc,
>> since I have not touched string/ffs{ll}.c.
> 
> "where possible" is the issue. Eg. for Arm USE_FFS_BUILTIN should be set from
> armv5te onwards, and USE_FFSLL_BUILTIN from armv6t2 onwards on GCC14.
> Other targets likely have similar issues. And targets that didn't have optimized
> assembler implementations might still have the right instructions and thus
> should use the builtin.
> 

Sure, that's why it is enabled by arch-specific hooks.  This patch does not
change the optimization selection, that why I mimic the current code to adds 
the arm builtin use only for arm6t2.

> There are 2 ways to fix this properly: a configure check, or always use the builtins.

I do not think the configure check is required, since we do have a working
fallback code at string/ffs.c.  The configure check would be required if
decide to always use the builtin, since some targets still do not route
the builtin to libgcc (alpha, mips, sparc, riscv, and s390x for instance).

> 
>> A possible further cleanup would to indeed remove string/ffs.c, and always use
>> the compiler builtin (and thus libgcc).  But I think this should be a different
>> patch.
> 
> To be clear I'm fine with a follow-on patch that removes these defines again.
> It's just that in previous iterations several people wanted more accurate settings
> for these defines, and it seems that could go on...

Again, my idea of this patch is to simplify a rarely used symbol to leverage the
compiler without changing current optimization selection and avoid the proliferation
of arch-specific implementations for no good reason.  I do agree that there still room 
for improvement (like the ffs arm selection you brought), but also I don't see it as 
hard blocker for this change.

The future cleanup will still need to have USE_FFS_BUILTIN for targets that do not
call libgcc, so I am not really convinced that this would lead to any simplification
(we will need to support a ffs alternative anyway, so it would just slow down some
targets that will just add another function call).

> 
>> In fact, in the glibc weekly discussion Carlos brought that we should also remove 
>> the ffs/ffsll benchtests (since there is no much point in having a benchmark for
>> theses symbols).
> 
> Yes I was going to bring that up too. There are likely other functions which are rarely
> used, and thus have no need to have optimized implementations or benchmarks

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Wilco Dijkstra]

Hi Adhemerval,

> I would say we are discussing two different things here.  My patch intention
> is to remove the multiple ffs/ffsll arch-specific and use compiler to generate
> the optimized instruction were possible.  The idea is not to leverage libgcc,
> since I have not touched string/ffs{ll}.c.

"where possible" is the issue. Eg. for Arm USE_FFS_BUILTIN should be set from
armv5te onwards, and USE_FFSLL_BUILTIN from armv6t2 onwards on GCC14.
Other targets likely have similar issues. And targets that didn't have optimized
assembler implementations might still have the right instructions and thus
should use the builtin.

There are 2 ways to fix this properly: a configure check, or always use the builtins.

> A possible further cleanup would to indeed remove string/ffs.c, and always use
> the compiler builtin (and thus libgcc).  But I think this should be a different
> patch.

To be clear I'm fine with a follow-on patch that removes these defines again.
It's just that in previous iterations several people wanted more accurate settings
for these defines, and it seems that could go on...

> In fact, in the glibc weekly discussion Carlos brought that we should also remove 
> the ffs/ffsll benchtests (since there is no much point in having a benchmark for
> theses symbols).

Yes I was going to bring that up too. There are likely other functions which are rarely
used, and thus have no need to have optimized implementations or benchmarks.

Cheers,
Wilco

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Adhemerval Zanella Netto]

On 28/08/23 10:12, Adhemerval Zanella Netto wrote:
> 
> 
> On 26/08/23 19:18, Wilco Dijkstra wrote:
>> Hi Richard,
>>
>>> With gcc version 12.3.0 (Ubuntu 12.3.0-1ubuntu1~22.04) arm-linux-gnueabihf-gcc,
>>>
>>> int f(long int x) { return __builtin_ffsl(x); }
>>> int g(long long int x) { return __builtin_ffsll(x); }
>>>
>>> The compiler only inlines f.
>>>
>>> That said, it is reasonable to call this a missed optimization and fix the compiler.
>>
>> It was already fixed on trunk. It seems __builtin_ctzll had the same issue.
>>
>> In any case we don't need to waste time trying to optimize the GLIBC ffs/ffsll
>> since they are never called.
> 
> I would say we are discussing two different things here.  My patch intention
> is to remove the multiple ffs/ffsll arch-specific and use compiler to generate
> the optimized instruction were possible.  The idea is not to leverage libgcc,
> since I have not touched string/ffs{ll}.c.
> 
> A possible further cleanup would to indeed remove string/ffs.c, and always use
> the compiler builtin (and thus libgcc).  But I think this should be a different
> patch.

In fact, in the glibc weekly discussion Carlos brought that we should also remove 
the ffs/ffsll benchtests (since there is no much point in having a benchmark for
theses symbols).

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Adhemerval Zanella Netto]

On 26/08/23 19:18, Wilco Dijkstra wrote:
> Hi Richard,
> 
>> With gcc version 12.3.0 (Ubuntu 12.3.0-1ubuntu1~22.04) arm-linux-gnueabihf-gcc,
>>
>> int f(long int x) { return __builtin_ffsl(x); }
>> int g(long long int x) { return __builtin_ffsll(x); }
>>
>> The compiler only inlines f.
>>
>> That said, it is reasonable to call this a missed optimization and fix the compiler.
> 
> It was already fixed on trunk. It seems __builtin_ctzll had the same issue.
> 
> In any case we don't need to waste time trying to optimize the GLIBC ffs/ffsll
> since they are never called.

I would say we are discussing two different things here.  My patch intention
is to remove the multiple ffs/ffsll arch-specific and use compiler to generate
the optimized instruction were possible.  The idea is not to leverage libgcc,
since I have not touched string/ffs{ll}.c.

A possible further cleanup would to indeed remove string/ffs.c, and always use
the compiler builtin (and thus libgcc).  But I think this should be a different
patch.

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Wilco Dijkstra]

Hi Richard,

> With gcc version 12.3.0 (Ubuntu 12.3.0-1ubuntu1~22.04) arm-linux-gnueabihf-gcc,
>
> int f(long int x) { return __builtin_ffsl(x); }
> int g(long long int x) { return __builtin_ffsll(x); }
>
> The compiler only inlines f.
>
> That said, it is reasonable to call this a missed optimization and fix the compiler.

It was already fixed on trunk. It seems __builtin_ctzll had the same issue.

In any case we don't need to waste time trying to optimize the GLIBC ffs/ffsll
since they are never called.

Cheers,
Wilco

Re: [PATCH v2] string: Use builtins for ffs and ffsll

[Richard Henderson]

On 8/26/23 05:31, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
>> I did analyze it and for 32 bits targets that do define USE_FFSLL_BUILTIN
>> to 1 (armv6t2, arc, i386, m68k/mc68020, and powerpc) gcc will lower ffs to
>> __builin_ffs. For instance:
> 
> But there is no point in ever defining USE_FFSLL_BUILTIN 0 - if a target can
> inline a 32-bit ffs, it will also inline 64-bit ffs.

s/will/should/.

With gcc version 12.3.0 (Ubuntu 12.3.0-1ubuntu1~22.04) arm-linux-gnueabihf-gcc,

int f(long int x) { return __builtin_ffsl(x); }
int g(long long int x) { return __builtin_ffsll(x); }

The compiler only inlines f.

That said, it is reasonable to call this a missed optimization and fix the compiler.


r~

[PATCH v2] string: Use builtins for ffs and ffsll

[Wilco Dijkstra]

Hi Adhemerval,

> I did analyze it and for 32 bits targets that do define USE_FFSLL_BUILTIN 
> to 1 (armv6t2, arc, i386, m68k/mc68020, and powerpc) gcc will lower ffs to 
> __builin_ffs. For instance:

But there is no point in ever defining USE_FFSLL_BUILTIN 0 - if a target can
inline a 32-bit ffs, it will also inline 64-bit ffs. And if it doesn't have 32-bit ffs,
one call to __ffsdi2 is better than 2 calls to __ffssi2. So this is wrong:

> +++ b/sysdeps/arm/math-use-builtins-ffs.h
> @@ -0,0 +1,2 @@
> +#define USE_FFS_BUILTIN   1
> +#define USE_FFSLL_BUILTIN 0

I think it's simpler to always "define USE_FFS(LL)_BUILTIN 1" in generic code
and only add exceptions for targets that don't support the builtin correctly.

Cheers,
Wilco