-
Notifications
You must be signed in to change notification settings - Fork 159
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
Improve binary function objects and replace thrust implementation #1872
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.
Thank you for this contribution!
Since the thrust operator()
s where unconditionally constexpr
, we should also make them constexpr
in libcudacxx. Please replace the _CCCL_CONSTEXPR_CXX14
by constexpr
on the operator()
s of all functors you pulled in thrust with this PR. Thank you!
@miscco what would you like to do about this:
We have |
Regarding the issue with So my suggestion would be that instead of just mmaking it an alias: using ::cuda::std::plus; We turn it into a struct that inherits from the template<class T>
struct plus : public ::cuda::std::plus<T> {
using first_argument_type _LIBCUDACXX_DEPRECATED_IN_CXX11 = T;
using second_argument_type _LIBCUDACXX_DEPRECATED_IN_CXX11 = T;
using result_type _LIBCUDACXX_DEPRECATED_IN_CXX11 = T;
}; |
@srinivasyadav18 looking at the commits of your PR it seems something went wrong on your latest rebase. Please cleanly rebase your PR on top of |
3f32a7d
to
5515453
Compare
@bernhardmgruber Apologies, I made an error in rebase. I have managed to fix it. The current commits are in place now. |
Thank you for your effort! This is great! |
5515453
to
856f5fc
Compare
I rebased this PR on top of main because the CI seemed stuck. |
/ok to test |
@srinivasyadav18 it seems more fixes are necessary. I had a quick look and had to dive down a rabbit hole, because your deprecations would cause further warnings in a lot of places. That's good, because we have to cleanup anyway for C++17/C++20, since a lot of the functional stuff is deprecated/dropped by then. Here is what I have: #1925 I suggest we wait until #1925 is merged and then rebase this PR again. Sorry for the extra effort, but I think it's worthwhile that we cleanup properly! |
@bernhardmgruber Thanks for looking into it! I will wait for #1925 and rebase the PR again. |
@srinivasyadav18 I rebased your PR since #1925 was merged. |
The CI was stuck somehow, so I rebased and pushed again. |
/ok to test |
It seems we are still not done. Here is another PR removing a use of a |
@@ -81,7 +81,7 @@ struct body | |||
RandomAccessIterator1 my_last = first + offset_to_last; | |||
|
|||
// carefully pass the init value for the interval with raw_reference_cast | |||
using sum_type = typename BinaryFunction::result_type; |
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.
Given @alliepiper's answer on this PR, this may be a breaking change that we have to delay. To make progress with this PR (because I really want the deprecation warnings to come in), please revert this change and surround this line with:
_CCCL_SUPPRESS_DEPRECATED_PUSH
using sum_type = typename BinaryFunction::result_type;
_CCCL_SUPPRESS_DEPRECATED_POP
This gives us more time to find a replacement for this line.
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.
#1998 got merged, and your original replacement is the way we should go. Also since _CCCL_SUPPRESS_DEPRECATED_PUSH
does not seem to work on ICC (I have a workaround locally, but it's ugly and we want to get rid of ICC anyhow). Just going for the proper solution seems the cleanest. I will push the change into your PR.
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.
Thanks for updating the PR.
To make progress, please have a look at the failing CI runs and add |
@bernhardmgruber Thanks a lot for making other PR's to make this PR progress. I will add the necessary deprecated warnings wherever needed soon and push the changes!. |
@srinivasyadav18 please rebase. #1998 removed another user of |
c93fbe9
to
33dee76
Compare
/ok to test |
🟨 CI finished in 14h 27m: Pass: 98%/417 | Total: 7d 09h | Avg: 25m 33s | Max: 1h 21m | Hits: 69%/520842
|
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
CUB | |
+/- | Thrust |
CUDA Experimental | |
pycuda |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | pycuda |
🏃 Runner counts (total jobs: 417)
# | Runner |
---|---|
305 | linux-amd64-cpu16 |
61 | linux-amd64-gpu-v100-latest-1 |
28 | linux-arm64-cpu16 |
23 | windows-amd64-cpu16 |
Replace _CCCL_CONSTEXPR_CXX14 with constexpr in all libcudacxx binary function objects that are imported in thrust.
1fe1cd8
to
48a0dcf
Compare
48a0dcf
to
0f56e9e
Compare
/ok to test |
/ok to test |
🟨 CI finished in 8h 41m: Pass: 99%/417 | Total: 1d 23h | Avg: 6m 51s | Max: 45m 03s | Hits: 97%/522343
|
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
CUB | |
+/- | Thrust |
CUDA Experimental | |
pycuda |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | pycuda |
🏃 Runner counts (total jobs: 417)
# | Runner |
---|---|
305 | linux-amd64-cpu16 |
61 | linux-amd64-gpu-v100-latest-1 |
28 | linux-arm64-cpu16 |
23 | windows-amd64-cpu16 |
🟩 CI finished in 11h 25m: Pass: 100%/417 | Total: 2d 00h | Avg: 6m 55s | Max: 45m 03s | Hits: 97%/525254
|
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
CUB | |
+/- | Thrust |
CUDA Experimental | |
pycuda |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | pycuda |
🏃 Runner counts (total jobs: 417)
# | Runner |
---|---|
305 | linux-amd64-cpu16 |
61 | linux-amd64-gpu-v100-latest-1 |
28 | linux-arm64-cpu16 |
23 | windows-amd64-cpu16 |
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.
Thank you for the contribution!
…IDIA#1872) * Improve binary function objects and replace thrust implementation * simplify use of ::cuda::std binary_function_objects * Replace _CCCL_CONSTEXPR_CXX14 with constexpr in all libcudacxx binary function objects that are imported in thrust. * Determine partial sum type without ::result_type * Ignore _LIBCUDACXX_DEPRECATED_IN_CXX11 for doxygen Co-authored-by: Bernhard Manfred Gruber <bernhardmgruber@gmail.com>
…IDIA#1872) * Improve binary function objects and replace thrust implementation * simplify use of ::cuda::std binary_function_objects * Replace _CCCL_CONSTEXPR_CXX14 with constexpr in all libcudacxx binary function objects that are imported in thrust. * Determine partial sum type without ::result_type * Ignore _LIBCUDACXX_DEPRECATED_IN_CXX11 for doxygen Co-authored-by: Bernhard Manfred Gruber <bernhardmgruber@gmail.com>
Description
This PR replaces most of the thrust binary function objects to use
cuda::std
one's.Changes:
is_commutative
traits to new file is_commutative.h, becauseis_commutative
requires forward definitions, which will restrict the use ofcuda::std
binary function objects.decltype
in tbb/detail/reduce_intervals.h to find theresult_type
of binary function object, because newly replacedcuda::std
one's do not haveresult_type
as embedded type.closes #1664
Checklist