-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport std integer comparison functions to C++11 #2805
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether it is actually worth backporting this to C++11
or moreso if we backport it to C++11 whether we should make it constexpr
Using multiple function definitions instead of if constexpr is much more expensive due to the larger overload set.
We could get away with marking it as _CCCL_CONSTEXPR_14
so that we do not need the single line implementation for C++11 and use _CCCL_IF_CONSTEXPR
to let the compiler deal with all the false branches
It is also an option. I am just worried that it will trigger some warning about comparing signed and unsigned integers, because all of the branches will get instantiated if |
Would it help if I remove the |
Not really, the issue is that the overload set becomes 4 times larger than before which is the expensive part |
Oh I see.. But actually, does it even matter compared to e. g. instantiations of |
Generally speaking type instantiations and overload resolution are the two big hitters with respect to compile time. Being a foundational library means we need to ensure that we reduce the cost as much as possible because it does add up. This might be more in the realm of microoptimizations, but if we change it we can be mindful of the consequences. Especially if it comes to backporting things to an earlier standard. We want to avoid making the original thing worse |
Sure, so what about leaving this implementation for C++11 and C++14 and for C++17 and newer having separate implementation used if struct __cmp_eq_pre_if_constexpr_impl
{
template <class _Tp, class _Up, enable_if_t<is_signed<_Tp>::value && is_signed<_Up>::value, int> = 0>
_LIBCUDACXX_HIDE_FROM_ABI static constexpr bool __do_cmp(_Tp __t, _Up __u) noexcept
{
return __t == __u;
}
template <
class _Tp,
class _Up,
enable_if_t<(is_signed<_Tp>::value && !is_signed<_Up>::value) || (!is_signed<_Tp>::value && is_signed<_Up>::value),
int> = 0>
_LIBCUDACXX_HIDE_FROM_ABI static constexpr bool __do_cmp(_Tp __t, _Up __u) noexcept
{
return __t < 0 ? false : make_unsigned_t<_Tp>(__t) == __u;
}
template <class _Tp, class _Up, enable_if_t<!is_signed<_Tp>::value && !is_signed<_Up>::value, int> = 0>
_LIBCUDACXX_HIDE_FROM_ABI static constexpr bool __do_cmp(_Tp __t, _Up __u) noexcept
{
return __u < 0 ? false : __t == make_unsigned_t<_Up>(__u);
}
};
template <class _Tp,
class _Up,
enable_if_t<__is_safe_integral_cmp<_Tp>::value && __is_safe_integral_cmp<_Up>::value, int> = 0>
_LIBCUDACXX_HIDE_FROM_ABI constexpr bool cmp_equal(_Tp __t, _Up __u) noexcept
{
#if _CCCL_STD_VER >= 2017 && __cpp_if_constexpr
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;
}
else
{
return __u < 0 ? false : __t == make_unsigned_t<_Up>(__u);
}
#else
return __cmp_eq_pre_if_constexpr_impl::__do_cmp(__t, __u);
#endif // _CCCL_STD_VER >= 2017 && __cpp_if_constexpr
} But we will duplicate some code. What do you think about that? |
Without having read through the entire discussion in detail, I think this is the most important question for me. Thrust and CUB are dropping C++11 in favor of C++17 soon. libcu++ requires C++14 IIUC, but contains lots of C++11 backports for Thrust and CUB. I would love to see all C++11 code paths and workarounds gone after the upgrade in Thrust and CUB, which renders the effort of this PR meaningless. A backport to C++17 (or maybe C++14) would make more sense. |
/ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to disable for C++11 now
...cxx/test/libcudacxx/std/utilities/utility/utility.intcmp/intcmp.cmp_equal/cmp_equal.pass.cpp
Show resolved
Hide resolved
...test/libcudacxx/std/utilities/utility/utility.intcmp/intcmp.cmp_greater/cmp_greater.pass.cpp
Show resolved
Hide resolved
...cxx/std/utilities/utility/utility.intcmp/intcmp.cmp_greater_equal/cmp_greater_equal.pass.cpp
Show resolved
Hide resolved
...dacxx/test/libcudacxx/std/utilities/utility/utility.intcmp/intcmp.cmp_less/cmp_less.pass.cpp
Show resolved
Hide resolved
...ibcudacxx/std/utilities/utility/utility.intcmp/intcmp.cmp_less_equal/cmp_less_equal.pass.cpp
Show resolved
Hide resolved
.../libcudacxx/std/utilities/utility/utility.intcmp/intcmp.cmp_not_equal/cmp_not_equal.pass.cpp
Show resolved
Hide resolved
libcudacxx/test/libcudacxx/std/utilities/utility/utility.intcmp/intcmp.fail.cpp
Show resolved
Hide resolved
...dacxx/test/libcudacxx/std/utilities/utility/utility.intcmp/intcmp.in_range/in_range.pass.cpp
Show resolved
Hide resolved
/ok to test |
/ok to test |
Perfect, thank you for finalizing this PR. Right now I am on vacation, I would get back to it on Monday 😅 thank you! |
One more thing - would it make sense to replace SFINAE with upcoming _CCCL_TEMPLATE and _CCCL_REQUIRES? |
|
@davebayer I was able to find a version that works in all standard modes and is maximally efficient in C++17 onwards. as a cherry we can also enable C++11 support. The issue is that it needs #2832 |
Great job! |
/ok to test |
/ok to test |
🟨 CI finished in 4h 37m: Pass: 97%/400 | Total: 9d 20h | Avg: 35m 25s | Max: 3h 25m | Hits: 33%/21432
|
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
CUB | |
Thrust | |
+/- | CUDA Experimental |
python | |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 400)
# | Runner |
---|---|
326 | linux-amd64-cpu16 |
31 | linux-amd64-gpu-v100-latest-1 |
28 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
We have emulation for concepts in LIBCUDACXX that was guarded behind C++14 But there is nothing that requires C++14 for just the template headers and we want to use them universally throughout the codebase Consequently move them to CCCL proper and enable them unconditionally. To ensure that we do not add any hidden dependencies this also adds a barebones implementation of `enable_if_t` and a trailing `enable_if_t`
/ok to test |
/ok to test |
🟨 CI finished in 2h 11m: Pass: 95%/400 | Total: 8d 11h | Avg: 30m 31s | Max: 1h 40m | Hits: 33%/21432
|
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
CUB | |
Thrust | |
+/- | CUDA Experimental |
python | |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 400)
# | Runner |
---|---|
326 | linux-amd64-cpu16 |
31 | linux-amd64-gpu-v100-latest-1 |
28 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
Description
This PR implements backport of C++20 integer comparison functions P0586R2 to C++11.
Checklist