-
-
Notifications
You must be signed in to change notification settings - Fork 491
Integrate NVIDIA's S/R Bulk implementation into HPX #6746
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
base: master
Are you sure you want to change the base?
Conversation
|
Can one of the admins verify this patch? |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
49b7122 to
ff8a35c
Compare
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.
Excellent work! The tests are checking all the correct things, so if we get them passing we are good to merge :)
libs/core/execution_base/include/hpx/execution_base/stdexec_forward.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/explicit_scheduler_executor.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/explicit_scheduler_executor.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler_bulk.hpp
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler.hpp
Outdated
Show resolved
Hide resolved
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.
Really great work!
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.
Let's remove those files from this PR as they are being worked on for the same reason by another contributor
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.
Changes here are not important
| // Continue on | ||
| using stdexec::continue_on; | ||
| using stdexec::continue_on_t; | ||
| // Backward compatibility alias for continues_on |
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.
No need to have backwards compatibility here, you can remove 134-138
|
|
||
| // Additional stdexec concepts and utilities needed for domain customization | ||
| using stdexec::__completes_on; | ||
| using stdexec::__starts_on; |
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.
__completes_on, __starts_on, and sender_expr_for should be moved in the stdexec internals namespace
| // Concept to match bulk sender types | ||
| // Note: We keep bulk_t handling as pragmatic workaround for stdexec template issues | ||
| template <typename Sender> | ||
| concept any_bulk_sender = |
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.
should be renamed to bulk_chunked_or_unchunked_sender
| constexpr void bulk_scheduler_invoke_helper_chunked( | ||
| hpx::util::index_pack<Is...>, F&& f, Start&& start, End&& end, Ts& ts) | ||
| { | ||
| if constexpr (sizeof...(Is) == 0) |
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 can remove the sizeof...(Is) == 0 test here
|
|
||
| auto it = std::next(hpx::util::begin(op_state->shape), i_begin); | ||
| for (std::uint32_t i = i_begin; i != i_end; (void) ++it, ++i) | ||
| auto it = |
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.
this should be moved inside the else branch below
| { | ||
| // Don't spawn tasks if there is no work to be done | ||
| auto const size = | ||
| // static_cast<std::uint32_t>(op_state->shape); |
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 can remove this commented out part
| hpx::is_invocable_v<F, range_value_type, | ||
| std::add_lvalue_reference_t<Ts>...> | ||
| )> | ||
| template <typename... Ts> |
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 should re-add the test as a normal concept (requires) clause
| }; | ||
| } // namespace hpx::execution::experimental::detail | ||
|
|
||
| namespace hpx::execution::experimental { |
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.
Better wrap this in an #if !defined clause instead of removing, so we keep backwards compatibility for now
| using stdexec::on_t; | ||
|
|
||
| // Continue on | ||
| using stdexec::continue_on; |
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.
This (134, 135) should be removed too, since the proper spelling is continues_on
libs/core/executors/include/hpx/executors/thread_pool_scheduler_bulk.hpp
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler_bulk.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler_bulk.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler_bulk.hpp
Outdated
Show resolved
Hide resolved
libs/core/executors/include/hpx/executors/thread_pool_scheduler_bulk.hpp
Outdated
Show resolved
Hide resolved
| #include <hpx/coroutines/thread_enums.hpp> | ||
| #include <hpx/datastructures/tuple.hpp> | ||
| #include <hpx/datastructures/variant.hpp> | ||
| #include <hpx/execution/algorithms/bulk.hpp> |
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.
This should be included in the headers when not using STDEXEC, it seems this is what's causing this failing test:
This PR forwards stdexec::bulk and stdexec::bulk_t inside stdexec_forward.hpp.
These were previously commented out. This change makes bulk available through hpx::execution::experimental::bulk, aligning HPX's Sender/Receiver interface with NVIDIA's stdexec implementation.