[libc] implement `strings/ffs` #129892

c8ef · 2025-03-05T15:39:37Z

This patch adds the strings/ffs function.
ref: https://pubs.opengroup.org/onlinepubs/9799919799/functions/ffs.html
Closes: #122054.

llvmbot · 2025-03-05T15:40:29Z

@llvm/pr-subscribers-libc

Author: None (c8ef)

Changes

This patch adds the strings/ffs function.
ref: https://pubs.opengroup.org/onlinepubs/9799919799/functions/ffs.html
Closes: #122054.

Full diff: https://github.com/llvm/llvm-project/pull/129892.diff

14 Files Affected:

(modified) libc/config/linux/aarch64/entrypoints.txt (+3)
(modified) libc/config/linux/x86_64/entrypoints.txt (+3)
(modified) libc/include/strings.yaml (+18)
(modified) libc/src/strings/CMakeLists.txt (+24)
(added) libc/src/strings/ffs.cpp (+17)
(added) libc/src/strings/ffs.h (+20)
(added) libc/src/strings/ffsl.cpp (+17)
(added) libc/src/strings/ffsl.h (+20)
(added) libc/src/strings/ffsll.cpp (+17)
(added) libc/src/strings/ffsll.h (+20)
(modified) libc/test/src/strings/CMakeLists.txt (+30)
(added) libc/test/src/strings/ffs_test.cpp (+25)
(added) libc/test/src/strings/ffsl_test.cpp (+25)
(added) libc/test/src/strings/ffsll_test.cpp (+25)

diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 6c2be4d3b0b99..c7beb3ef3fdfc 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -93,6 +93,9 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.strings.bcmp
     libc.src.strings.bcopy
     libc.src.strings.bzero
+    libc.src.strings.ffs
+    libc.src.strings.ffsl
+    libc.src.strings.ffsll
     libc.src.strings.index
     libc.src.strings.rindex
     libc.src.strings.strcasecmp
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 779654422e35b..12dc87bf945fd 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -93,6 +93,9 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.strings.bcmp
     libc.src.strings.bcopy
     libc.src.strings.bzero
+    libc.src.strings.ffs
+    libc.src.strings.ffsl
+    libc.src.strings.ffsll
     libc.src.strings.index
     libc.src.strings.rindex
     libc.src.strings.strcasecmp
diff --git a/libc/include/strings.yaml b/libc/include/strings.yaml
index b6aa8f6d60b27..012805269f775 100644
--- a/libc/include/strings.yaml
+++ b/libc/include/strings.yaml
@@ -29,6 +29,24 @@ functions:
     arguments:
       - type: void *
       - type: size_t
+  - name: ffs
+    standards: 
+    - POSIX
+    return_type: int
+    arguments:
+      - int
+  - name: ffsl
+    standards: 
+    - POSIX
+    return_type: int
+    arguments:
+      - long
+  - name: ffsll
+    standards: 
+    - POSIX
+    return_type: int
+    arguments:
+      - long long
   - name: index
     standards:
       - BSDExtensions
diff --git a/libc/src/strings/CMakeLists.txt b/libc/src/strings/CMakeLists.txt
index 5e84c7be1f7d6..aa0f8f51a5f59 100644
--- a/libc/src/strings/CMakeLists.txt
+++ b/libc/src/strings/CMakeLists.txt
@@ -54,6 +54,30 @@ add_entrypoint_object(
     bcopy.h
 )
 
+add_entrypoint_object(
+  ffs
+  SRCS
+    ffs.cpp
+  HDRS
+    ffs.h
+)
+
+add_entrypoint_object(
+  ffsl
+  SRCS
+    ffsl.cpp
+  HDRS
+    ffsl.h
+)
+
+add_entrypoint_object(
+  ffsll
+  SRCS
+    ffsll.cpp
+  HDRS
+    ffsll.h
+)
+
 add_entrypoint_object(
   index
   SRCS
diff --git a/libc/src/strings/ffs.cpp b/libc/src/strings/ffs.cpp
new file mode 100644
index 0000000000000..4cf565fc335e7
--- /dev/null
+++ b/libc/src/strings/ffs.cpp
@@ -0,0 +1,17 @@
+//===-- Implementation of ffs ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/strings/ffs.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, ffs, (int i)) { return __builtin_ffs(i); }
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/strings/ffs.h b/libc/src/strings/ffs.h
new file mode 100644
index 0000000000000..bf43c43caedb2
--- /dev/null
+++ b/libc/src/strings/ffs.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for ffs ---------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STRINGS_FFS_H
+#define LLVM_LIBC_SRC_STRINGS_FFS_H
+
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+int ffs(int i);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STRINGS_FFS_H
diff --git a/libc/src/strings/ffsl.cpp b/libc/src/strings/ffsl.cpp
new file mode 100644
index 0000000000000..1373699d974ef
--- /dev/null
+++ b/libc/src/strings/ffsl.cpp
@@ -0,0 +1,17 @@
+//===-- Implementation of ffsl --------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/strings/ffsl.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, ffsl, (long i)) { return __builtin_ffsl(i); }
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/strings/ffsl.h b/libc/src/strings/ffsl.h
new file mode 100644
index 0000000000000..1feca010b2ebb
--- /dev/null
+++ b/libc/src/strings/ffsl.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for ffsl --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STRINGS_FFSL_H
+#define LLVM_LIBC_SRC_STRINGS_FFSL_H
+
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+int ffsl(long i);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STRINGS_FFSL_H
diff --git a/libc/src/strings/ffsll.cpp b/libc/src/strings/ffsll.cpp
new file mode 100644
index 0000000000000..a0b6858c1a9ee
--- /dev/null
+++ b/libc/src/strings/ffsll.cpp
@@ -0,0 +1,17 @@
+//===-- Implementation of ffsll -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/strings/ffsll.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, ffsll, (long long i)) { return __builtin_ffsll(i); }
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/strings/ffsll.h b/libc/src/strings/ffsll.h
new file mode 100644
index 0000000000000..f059b8ab89b4e
--- /dev/null
+++ b/libc/src/strings/ffsll.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for ffsll -------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STRINGS_FFSLL_H
+#define LLVM_LIBC_SRC_STRINGS_FFSLL_H
+
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+int ffsll(long long i);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_STRINGS_FFSLL_H
diff --git a/libc/test/src/strings/CMakeLists.txt b/libc/test/src/strings/CMakeLists.txt
index 10f96b8531f68..baa22bb449c6c 100644
--- a/libc/test/src/strings/CMakeLists.txt
+++ b/libc/test/src/strings/CMakeLists.txt
@@ -12,6 +12,36 @@ add_libc_test(
     LibcMemoryHelpers
 )
 
+add_libc_test(
+  ffs_test
+  SUITE
+    libc-strings-tests
+  SRCS
+    ffs_test.cpp
+  DEPENDS
+    libc.src.strings.ffs
+)
+
+add_libc_test(
+  ffsl_test
+  SUITE
+    libc-strings-tests
+  SRCS
+    ffsl_test.cpp
+  DEPENDS
+    libc.src.strings.ffsl
+)
+
+add_libc_test(
+  ffsll_test
+  SUITE
+    libc-strings-tests
+  SRCS
+    ffsll_test.cpp
+  DEPENDS
+    libc.src.strings.ffsll
+)
+
 add_libc_test(
   index_test
   SUITE
diff --git a/libc/test/src/strings/ffs_test.cpp b/libc/test/src/strings/ffs_test.cpp
new file mode 100644
index 0000000000000..5270837ec2ac0
--- /dev/null
+++ b/libc/test/src/strings/ffs_test.cpp
@@ -0,0 +1,25 @@
+//===-- Unittests for ffs -------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/strings/ffs.h"
+
+#include "src/__support/macros/config.h"
+#include "test/UnitTest/Test.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+TEST(LlvmLibcFfsTest, SimpleFfs) {
+  ASSERT_EQ(ffs(0), 0);
+  ASSERT_EQ(ffs(1), 1);
+  ASSERT_EQ(ffs(0xfbe71), 1);
+  ASSERT_EQ(ffs(0xfbe70), 5);
+  ASSERT_EQ(ffs(0x10), 5);
+  ASSERT_EQ(ffs(0x100), 9);
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/test/src/strings/ffsl_test.cpp b/libc/test/src/strings/ffsl_test.cpp
new file mode 100644
index 0000000000000..1fcd2943fbb15
--- /dev/null
+++ b/libc/test/src/strings/ffsl_test.cpp
@@ -0,0 +1,25 @@
+//===-- Unittests for ffsl ------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/strings/ffsl.h"
+
+#include "src/__support/macros/config.h"
+#include "test/UnitTest/Test.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+TEST(LlvmLibcFfslTest, SimpleFfsl) {
+  ASSERT_EQ(ffsl(0L), 0);
+  ASSERT_EQ(ffsl(1L), 1);
+  ASSERT_EQ(ffsl(0xfbe71L), 1);
+  ASSERT_EQ(ffsl(0xfbe70L), 5);
+  ASSERT_EQ(ffsl(0x10L), 5);
+  ASSERT_EQ(ffsl(0x100L), 9);
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/test/src/strings/ffsll_test.cpp b/libc/test/src/strings/ffsll_test.cpp
new file mode 100644
index 0000000000000..1888e3161dad6
--- /dev/null
+++ b/libc/test/src/strings/ffsll_test.cpp
@@ -0,0 +1,25 @@
+//===-- Unittests for ffsll -----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/strings/ffsll.h"
+
+#include "src/__support/macros/config.h"
+#include "test/UnitTest/Test.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+TEST(LlvmLibcFfsllTest, SimpleFfsll) {
+  ASSERT_EQ(ffsll(0LL), 0);
+  ASSERT_EQ(ffsll(1LL), 1);
+  ASSERT_EQ(ffsll(0xfbe71LL), 1);
+  ASSERT_EQ(ffsll(0xfbe70LL), 5);
+  ASSERT_EQ(ffsll(0x10LL), 5);
+  ASSERT_EQ(ffsll(0x100LL), 9);
+}
+
+} // namespace LIBC_NAMESPACE_DECL

lntue · 2025-03-05T17:48:25Z

libc/src/strings/ffs.cpp

+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, ffs, (int i)) { return __builtin_ffs(i); }


Aren't these functions the same spec as stdc_first_trailing_one? If so then reuse its implementation at https://github.com/llvm/llvm-project/blob/main/libc/src/stdbit/stdc_first_trailing_one_ui.cpp#L18, in case the builtins are not available, or making callback to the function itself.

We use the ctz builtins and the find first one function seems to just invert the mask and use ctz. ffs is basically just ctz + 1 AFAIK.

As @jhuber6 points out, first_trailing_one calls cpp::countr_zero, which in turn calls __builtin_ctzg. Ultimately, we're using built-in functions, so perhaps we could directly use the ffs built-in here instead?

Okay, I see that it uses __has_builtin to check if the built-in is available. In that case, maybe we can use cpp::countr_zero(i) + 1 here instead?

Just be aware the codegen is slightly different. I would guess that the builtin is always faster, but I haven't benchmarked anything https://godbolt.org/z/8KohTj5Md.

Are there other constraints that make them different? If not then it seems like some missed optimization by the compiler(s).

And the codegen is still not very good:

0: 55 push %rbp 1: 48 89 e5 mov %rsp,%rbp 4: 8d 4f 01 lea 0x1(%rdi),%ecx 7: 85 ff test %edi,%edi 9: 74 06 je 11 <_ZN22__llvm_libc_21_0_0_git3ffsEi+0x11> b: f3 0f bc d7 tzcnt %edi,%edx f: eb 05 jmp 16 <_ZN22__llvm_libc_21_0_0_git3ffsEi+0x16> 11: ba 20 00 00 00 mov $0x20,%edx 16: ff c2 inc %edx 18: 31 c0 xor %eax,%eax 1a: 83 f9 02 cmp $0x2,%ecx 1d: 0f 43 c2 cmovae %edx,%eax 20: 5d pop %rbp 21: c3 ret

I'm confused: https://godbolt.org/z/TYK3abdY5

As mentioned in #130155, we need to handle two special cases: when the input is 0 and when the input is the unsigned maximum value. This requirement codegen less elegant than we'd like.

If you follow the godbolt link https://godbolt.org/z/xafvqha4c , (value == 0) ? 0 : countr_zero(value) + 1 is identical to __builtin_ffs in both 0 and unsigned + signed maximum values.

What I really meant is that our current implementations and test cases for stdc_first_trailing_one are wrong on both 0 and unsigned maximum values.

Got it finally. Sorry for being careless. I'll update this tonight.

jhuber6

We should add the ffs functions (using ffsg if available) to cpp::bit and then make these functions use that.

c8ef · 2025-03-06T17:59:12Z

Can you fix the first_trailing_one implementation and their tests

Please take a look at #130155.

Fix this minor bug. See more context at #129892.

lntue · 2025-03-07T16:46:15Z

libc/test/src/strings/ffsl_test.cpp

+  ASSERT_EQ(ffsl(0x00200000L), 22);
+  ASSERT_EQ(ffsl(0x04000000L), 27);
+  ASSERT_EQ(ffsl(0x80000000L), 32);
+  ASSERT_EQ(ffsl(0x0000000100000000L), 33);


Can you guard this part, because long might be 32-bit on some platforms.

Ah, I almost forgot about this. Thanks for reminding me!

lntue

Thanks for the patch! On the side note, probably we will need to revise other stdbit functions to see if there are any other issues with them, as what you found with first_trailing_one.

c8ef · 2025-03-08T01:30:57Z

Thanks for your detailed and patient review!

On the side note, probably we will need to revise other stdbit functions to see if there are any other issues with them, as what you found with first_trailing_one.

Will do in a sec.

Fix this minor bug. See more context at llvm#129892.

implement ffs

6f2d8ef

llvmbot added the libc label Mar 5, 2025

c8ef added 2 commits March 5, 2025 15:46

fix ci

a0e76ae

fix ci

f83e2c8

c8ef requested review from nickdesaulniers, lntue and michaelrj-google March 5, 2025 16:03

lntue reviewed Mar 5, 2025

View reviewed changes

c8ef requested review from jhuber6 and lntue March 6, 2025 15:35

jhuber6 reviewed Mar 6, 2025

View reviewed changes

c8ef mentioned this pull request Mar 6, 2025

[libc] first_trailing_one(0) should be 0. #130155

Merged

c8ef added a commit that referenced this pull request Mar 7, 2025

[libc] first_trailing_one(0) should be 0. (#130155)

dd9a2f0

Fix this minor bug. See more context at #129892.

c8ef added 2 commits March 8, 2025 00:14

Merge branch 'main' into ffs

5409f31

use first trailing one

ccc7f83

lntue reviewed Mar 7, 2025

View reviewed changes

guard using LP64 since long could be 32bit

63bbca5

lntue approved these changes Mar 7, 2025

View reviewed changes

c8ef merged commit a5588b6 into llvm:main Mar 8, 2025
16 checks passed

c8ef deleted the ffs branch March 8, 2025 01:31

jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025

[libc] first_trailing_one(0) should be 0. (llvm#130155)

7d381e2

Fix this minor bug. See more context at llvm#129892.


		namespace LIBC_NAMESPACE_DECL {

		LLVM_LIBC_FUNCTION(int, ffs, (int i)) { return __builtin_ffs(i); }

[libc] implement strings/ffs #129892

[libc] implement strings/ffs #129892

Uh oh!

Conversation

c8ef commented Mar 5, 2025

Uh oh!

llvmbot commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c8ef Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c8ef Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

c8ef commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lntue left a comment

Choose a reason for hiding this comment

Uh oh!

c8ef commented Mar 8, 2025

Uh oh!

Uh oh!

Uh oh!

[libc] implement `strings/ffs` #129892

[libc] implement `strings/ffs` #129892

c8ef Mar 6, 2025 •

edited

Loading

c8ef Mar 7, 2025 •

edited

Loading