[libc++] std::cmp_less and other integer comparison functions could be improved #151332

kisuhorikka · 2025-07-30T13:23:44Z

github-actions · 2025-07-30T13:24:11Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-07-30T13:24:47Z

@llvm/pr-subscribers-libcxx

Author: None (kisuhorikka)

Changes

Fix #65136

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

1 Files Affected:

(modified) libcxx/include/__utility/cmp.h (+14-2)

diff --git a/libcxx/include/__utility/cmp.h b/libcxx/include/__utility/cmp.h
index 14dc0c154c040..4c29f09628095 100644
--- a/libcxx/include/__utility/cmp.h
+++ b/libcxx/include/__utility/cmp.h
@@ -28,7 +28,13 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <__signed_or_unsigned_integer _Tp, __signed_or_unsigned_integer _Up>
 _LIBCPP_HIDE_FROM_ABI constexpr bool cmp_equal(_Tp __t, _Up __u) noexcept {
-  if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
+  if constexpr (sizeof(_Tp) < sizeof(int) && sizeof(_Up) < sizeof(int)) {
+    __builtin_assume(__t < numeric_limits<int>::max() && __u < numeric_limits<int>::max());
+    return static_cast<int>(__t) == static_cast<int>(__u);
+  } else if constexpr (sizeof(_Tp) < sizeof(long long) && sizeof(_Up) < sizeof(long long)) {
+    __builtin_assume(__t < numeric_limits<long long>::max() && __u < numeric_limits<long long>::max());
+    return static_cast<long long>(__t) == static_cast<long long>(__u);
+  } else if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
     return __t == __u;
   else if constexpr (is_signed_v<_Tp>)
     return __t < 0 ? false : make_unsigned_t<_Tp>(__t) == __u;
@@ -43,7 +49,13 @@ _LIBCPP_HIDE_FROM_ABI constexpr bool cmp_not_equal(_Tp __t, _Up __u) noexcept {
 
 template <__signed_or_unsigned_integer _Tp, __signed_or_unsigned_integer _Up>
 _LIBCPP_HIDE_FROM_ABI constexpr bool cmp_less(_Tp __t, _Up __u) noexcept {
-  if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
+  if constexpr (sizeof(_Tp) < sizeof(int) && sizeof(_Up) < sizeof(int)) {
+    __builtin_assume(__t < numeric_limits<int>::max() && __u < numeric_limits<int>::max());
+    return static_cast<int>(__t) < static_cast<int>(__u);
+  } else if constexpr (sizeof(_Tp) < sizeof(long long) && sizeof(_Up) < sizeof(long long)) {
+    __builtin_assume(__t < numeric_limits<long long>::max() && __u < numeric_limits<long long>::max());
+    return static_cast<long long>(__t) < static_cast<long long>(__u);
+  } else if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
     return __t < __u;
   else if constexpr (is_signed_v<_Tp>)
     return __t < 0 ? true : make_unsigned_t<_Tp>(__t) < __u;

kisuhorikka · 2025-07-30T14:37:44Z

libcxx seems unhappy with __builtin_assume. Should we keep it for performance, or remove it? @ldionne

H-G-Hristov · 2025-07-30T15:17:56Z

libcxx seems unhappy with __builtin_assume. Should we keep it for performance, or remove it? @ldionne

Have you read this discussion: #91612
You should probably use the macro __LIBCPP_ASSUME instead:

llvm-project/libcxx/include/__assert

Line 26 in 098426c

    
           // WARNING: __builtin_assume can currently inhibit optimizations. Only add assumptions with a clear

kisuhorikka · 2025-08-01T05:16:30Z

Checks for last commit failed due to 'interrupted by user', and I found no obvious errors.

The discussion points out that the CI is not done running yet and that job needs to be restarted (which will happen automatically).

So I will try to commit again if the CI doesn't restarts automatically after hours.

philnik777 · 2025-08-01T06:31:19Z

libcxx/include/__utility/cmp.h

@@ -28,7 +29,13 @@ _LIBCPP_BEGIN_NAMESPACE_STD

 template <__signed_or_unsigned_integer _Tp, __signed_or_unsigned_integer _Up>
 _LIBCPP_HIDE_FROM_ABI constexpr bool cmp_equal(_Tp __t, _Up __u) noexcept {
-  if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
+  if constexpr (sizeof(_Tp) < sizeof(int) && sizeof(_Up) < sizeof(int)) {
+    _LIBCPP_ASSUME(__t < numeric_limits<int>::max() && __u < numeric_limits<int>::max());


Why did you add these in the first place?

Previous bench shows slower results when converting parameters from short to long long. The performance is better if parameters are converted to int. https://quick-bench.com/q/q87HT8ePhR1AqI_Gy3Zj6rbElp8

However, the above results are based on clang 15 (I neglected this before unfortunately), and the difference seems vanished on clang 17. https://quick-bench.com/q/q0Q-bgFsVQyqe8QiDTiPHkawIu4

Though the little difference between speed, the assembly for int is shorter than long long, and _LIBCPP_ASSUME seems no work now. https://godbolt.org/z/a5s3zx64W

On Clang 21 (or 22?), _LIBCPP_ASSUME doesn't affect the generated same assembly. I'd like to remove _LIBCPP_ASSUME here.

H-G-Hristov · 2025-08-01T16:03:39Z

Checks for last commit failed due to 'interrupted by user', and I found no obvious errors.

The discussion points out that the CI is not done running yet and that job needs to be restarted (which will happen automatically).

So I will try to commit again if the CI doesn't restarts automatically after hours.

Issues like that can happen. Just restart the failed job if you have access or rebase to restart the CI. Sometimes somebody can restart the job for you. Specifically I have the same issue with buildkite/libcxx-ci/aarch64

kisuhorikka · 2025-08-06T05:13:15Z

Hi @philnik777 There are 3 CI tests waiting for status to be reported for 3 days. The CI might be required to start manually for first PR in similar case . Could you please help to start it? Thank you for your help!

frederick-vs-ja · 2025-08-08T07:16:59Z

libcxx/include/__utility/cmp.h

@@ -28,7 +29,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD

 template <__signed_or_unsigned_integer _Tp, __signed_or_unsigned_integer _Up>
 _LIBCPP_HIDE_FROM_ABI constexpr bool cmp_equal(_Tp __t, _Up __u) noexcept {
-  if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>)
+  if constexpr (sizeof(_Tp) < sizeof(int) && sizeof(_Up) < sizeof(int))


This doesn't cover comparison between int and unsigned short yet.

I wonder whether we can/should do the following (ditto cmp_less).

template <__signed_or_unsigned_integer _Tp, __signed_integer _Ip> inline constexpr bool __comparison_can_promote_to = __signed_integer<_Tp> ? sizeof(_Tp) <= sizeof(_Ip) : sizeof(_Tp) < sizeof(_Ip); template <__signed_or_unsigned_integer _Tp, __signed_or_unsigned_integer _Up> _LIBCPP_HIDE_FROM_ABI constexpr bool cmp_equal(_Tp __t, _Up __u) noexcept { if constexpr (is_signed_v<_Tp> == is_signed_v<_Up>) return __t == __u; else if constexpr (__comparison_can_promote_to<_Tp, int> && __comparison_can_promote_to<_Up, int>) return static_cast<int>(__t) == static_cast<int>(__u); else if constexpr (__comparison_can_promote_to<_Tp, long long> && __comparison_can_promote_to<_Up, long long>) return static_cast<long long>(__t) == static_cast<long long>(__u); else if constexpr (is_signed_v<_Tp>) return __t < 0 ? false : make_unsigned_t<_Tp>(__t) == __u; else return __u < 0 ? false : __t == make_unsigned_t<_Up>(__u); }

This starts to feel like it should be a compiler builtin.

…e improved

…e improved with _LIBCPP_ASSUME

philnik777 · 2025-08-11T07:23:05Z

libcxx/include/__utility/cmp.h

@@ -26,10 +26,18 @@ _LIBCPP_BEGIN_NAMESPACE_STD

 #if _LIBCPP_STD_VER >= 20

+template <typename _Tp, typename _Ip>
+inline constexpr bool __comparison_can_promote_to =


Couldn't we use a concept here?

The concept version was submitted several times, but it always failed the check due to timeout. The typename version, otherwise, passed without a hitch. I guess that when cmp.h is directly or indirectly referenced by other files, instantiation of the concept may take too much time.

concepts take pretty much exactly the same time as variables. You should investigate further whether there is a problem in the compiler or something. I doubt it though.

kisuhorikka requested a review from a team as a code owner July 30, 2025 13:23

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 30, 2025

kisuhorikka force-pushed the improve_integer_comparison branch from cedea5b to b2409ee Compare July 31, 2025 12:02

philnik777 reviewed Aug 1, 2025

View reviewed changes

kisuhorikka force-pushed the improve_integer_comparison branch 2 times, most recently from 680faaa to 00b0d72 Compare August 3, 2025 02:09

frederick-vs-ja reviewed Aug 8, 2025

View reviewed changes

kisuhorikka force-pushed the improve_integer_comparison branch from 00b0d72 to 03f8228 Compare August 8, 2025 12:04

[libc++] std::cmp_less and other integer comparison functions could b…

60850b5

…e improved

kisuhorikka force-pushed the improve_integer_comparison branch 3 times, most recently from c8e0a17 to 534afd4 Compare August 10, 2025 08:39

[libc++] std::cmp_less and other integer comparison functions could b…

f39c2b6

…e improved with _LIBCPP_ASSUME

kisuhorikka force-pushed the improve_integer_comparison branch from 534afd4 to f39c2b6 Compare August 10, 2025 08:54

philnik777 reviewed Aug 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc++] std::cmp_less and other integer comparison functions could be improved #151332

[libc++] std::cmp_less and other integer comparison functions could be improved #151332

Uh oh!

kisuhorikka commented Jul 30, 2025

Uh oh!

github-actions bot commented Jul 30, 2025

Uh oh!

llvmbot commented Jul 30, 2025

Uh oh!

kisuhorikka commented Jul 30, 2025

Uh oh!

H-G-Hristov commented Jul 30, 2025

Uh oh!

kisuhorikka commented Aug 1, 2025

Uh oh!

philnik777 Aug 1, 2025

Uh oh!

kisuhorikka Aug 1, 2025

Uh oh!

frederick-vs-ja Aug 8, 2025

Uh oh!

H-G-Hristov commented Aug 1, 2025

Uh oh!

kisuhorikka commented Aug 6, 2025

Uh oh!

frederick-vs-ja Aug 8, 2025 •

edited

Loading

Uh oh!

philnik777 Aug 8, 2025

Uh oh!

philnik777 Aug 11, 2025

Uh oh!

kissholic Aug 11, 2025

Uh oh!

philnik777 Aug 11, 2025

Uh oh!

Uh oh!

[libc++] std::cmp_less and other integer comparison functions could be improved #151332

Are you sure you want to change the base?

[libc++] std::cmp_less and other integer comparison functions could be improved #151332

Uh oh!

Conversation

kisuhorikka commented Jul 30, 2025

Uh oh!

github-actions bot commented Jul 30, 2025

Uh oh!

llvmbot commented Jul 30, 2025

Uh oh!

kisuhorikka commented Jul 30, 2025

Uh oh!

H-G-Hristov commented Jul 30, 2025

Uh oh!

kisuhorikka commented Aug 1, 2025

Uh oh!

philnik777 Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

kisuhorikka Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

frederick-vs-ja Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

H-G-Hristov commented Aug 1, 2025

Uh oh!

kisuhorikka commented Aug 6, 2025

Uh oh!

frederick-vs-ja Aug 8, 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!

philnik777 Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

kissholic Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frederick-vs-ja Aug 8, 2025 •

edited

Loading