Repository navigation
[cudax] Implement cudax::group_view - #10787
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds ChangesGroup view implementation
Assessment against linked issues
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cudax/include/cuda/experimental/__group/group_view.cuh (1)
156-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
static_countnames its__parentparameter but does not use it, which can trigger unused-parameter warnings in system-header-disabled builds. Drop the name to matchcount_asandrank_asin this same class.- [[nodiscard]] _CCCL_DEVICE_API static constexpr ::cuda::std::size_t static_count(const _ParentGroup& __parent) noexcept + [[nodiscard]] _CCCL_DEVICE_API static constexpr ::cuda::std::size_t static_count(const _ParentGroup&) noexcept
_ParentMappingResultat line 60 is also unused in this class; remove it unless a follow-up commit needs it.cudax/include/cuda/experimental/__group/synchronizer/level_synchronizer.cuh (1)
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the generic implementation now uses
::cuda::std::is_same_v,::cuda::std::__always_false_v, andNV_IF_ELSE_TARGETdirectly, but this header includes only<cuda/hierarchy>and<cuda/std/__limits/numeric_limits.h>. Add the precise headers instead of relying on transitive includes.`#include` <cuda/hierarchy> `#include` <cuda/std/__limits/numeric_limits.h> +#include <cuda/std/__type_traits/always_false.h> +#include <cuda/std/__type_traits/is_same.h> + +#include <nv/target>The coding guidelines state: "Include all headers needed by the symbols being used; do not rely on transitive includes" and "Use the most precise available header". Confirm the exact header that defines
__always_false_vin the current tree.Also applies to: 92-92
Source: Coding guidelines
cudax/test/group/group_view.cu (1)
11-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: line 92 uses
cuda::std::dynamic_extent, but the file does not include<cuda/std/span>. Add the direct include rather than relying oncuda/experimental/group.cuh.`#include` <cuda/std/type_traits> `#include` <cuda/std/utility> +#include <cuda/std/span> `#include` <cuda/stream>The coding guidelines state: "Include all headers needed by the symbols being used; do not rely on transitive includes."
Also applies to: 92-92
Source: Coding guidelines
cudax/test/group/synchronizer/barrier_synchronizer.cu (1)
106-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the updated calls match the new instance API, but this test does not cover the two other new members,
view()anddeinit(...).group_viewdepends onview()returning a usable instance, anddeinitis the paired teardown for a barrier-backed instance. Add assertions for both, in the style of the existingdo_syncchecks.static_assert(noexcept(synchronizer_instance.do_sync_aligned(mapping_result, hierarchy))); synchronizer_instance.do_sync_aligned(mapping_result, hierarchy); + + // Test view(). + static_assert(noexcept(synchronizer_instance.view())); + const auto instance_view = synchronizer_instance.view(); + instance_view.do_sync(mapping_result, hierarchy);The cudax path instruction directs review focus to "correctness, lifetime/resource ownership, ... experimental API clarity, tests".
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27e01c67-c46e-42b5-b4da-b8255f8765fa
📒 Files selected for processing (14)
cudax/include/cuda/experimental/__group/group.cuhcudax/include/cuda/experimental/__group/group_view.cuhcudax/include/cuda/experimental/__group/synchronizer/barrier_synchronizer.cuhcudax/include/cuda/experimental/__group/synchronizer/lane_synchronizer.cuhcudax/include/cuda/experimental/__group/synchronizer/level_synchronizer.cuhcudax/include/cuda/experimental/__group/this_group.cuhcudax/include/cuda/experimental/__group/traits.cuhcudax/include/cuda/experimental/group.cuhcudax/test/CMakeLists.txtcudax/test/group/group_view.cucudax/test/group/synchronizer/barrier_synchronizer.cucudax/test/group/synchronizer/lane_synchronizer.cucudax/test/group/synchronizer/level_synchronizer.culibcudacxx/include/cuda/__hierarchy/hierarchy_level_base.h
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I assume #10820 is how we want the changes in that PR to be merged, and then we want to merge that into here? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cudax/include/cuda/experimental/__group/group.cuh (1)
163-176: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winimportant: Delete both copy operations for
group. A copied valid group calls__synchronizer_instance_.deinit()twice. A synchronizer that performs completion or barrier cleanup can then observe duplicate participation. Replace the TODO with deleted copy constructor and copy-assignment declarations. As per path instructions, focus on “lifetime/resource ownership” forcudax/**/*.Source: Path instructions
🧹 Nitpick comments (1)
cudax/test/group/group_view.cu (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Add
sync_aligned()coverage.group_view::sync_aligned()callsdo_sync_aligned, but this test only callssync(). Add calls for the same-unit, changed-unit, nested, and generic-group view cases.Also applies to: 104-104, 150-150, 199-199
🔇 Additional comments (6)
cudax/include/cuda/experimental/__group/group_view.cuh (3)
83-117: important: Store__hier_by value or reject rvalue parent groups. Each constructor binds__hier_to storage owned by__group. A temporary parent leaves the view with a dangling hierarchy reference.
136-144: important: Skip synchronization when__mapping_result_is invalid. Non-member threads otherwise calldo_sync()ordo_sync_aligned()with an invalid mapping result.
87-93: 🎯 Functional Correctnessimportant: Verify that
group_viewsatisfiesis_groupand can construct a further-splitgroup.group_viewdeclaressynchronizer_typebut does not providesynchronizer(), unlikegroupand__this_group_base. The PR objective requires a viewed group to support further splitting.cudax/include/cuda/experimental/__group/this_group.cuh (1)
158-240: LGTM!cudax/include/cuda/experimental/__group/fwd.cuh (1)
72-73: LGTM!cudax/test/group/group_view.cu (1)
225-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.important: Make the grid dimension divisible by the cluster dimension. CCCL defines
grid_dimsin blocks, and CUDA requires each grid dimension to be divisible by its cluster dimension. This configuration requests two blocks with clusters of three blocks, so the capability-gated launch is invalid. (nvidia.github.io)- cuda::grid_dims<2>(), cuda::cluster_dims<3>(), cuda::block_dims<128>(), cuda::cooperative_launch{}); + cuda::grid_dims<3>(), cuda::cluster_dims<3>(), cuda::block_dims<128>(), cuda::cooperative_launch{});
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c535bad3-93a8-4f6d-9c77-0e916b330434
📒 Files selected for processing (6)
cudax/include/cuda/experimental/__group/fwd.cuhcudax/include/cuda/experimental/__group/group.cuhcudax/include/cuda/experimental/__group/group_view.cuhcudax/include/cuda/experimental/__group/this_group.cuhcudax/test/CMakeLists.txtcudax/test/group/group_view.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cudax/test/CMakeLists.txt
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review.
This comment has been minimized.
This comment has been minimized.
Yeah, I split this PR into multiple smaller PRs, sorry about that! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 63d82601-5306-4a2f-a48c-aea8cb3b37ac
📒 Files selected for processing (4)
cudax/include/cuda/experimental/__group/group.cuhcudax/include/cuda/experimental/__group/group_view.cuhcudax/include/cuda/experimental/__group/this_group.cuhcudax/include/cuda/experimental/__group/traits.cuh
🚧 Files skipped from review as they are similar to previous changes (2)
- cudax/include/cuda/experimental/__group/this_group.cuh
- cudax/include/cuda/experimental/__group/group_view.cuh
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8426bc8 to
75b36cb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
⏱️ CCCL compile-time benchmark comparison: Public headers compile-time benchResult: 0 regression row(s), 2 improvement row(s) above threshold.
Artifacts: reports and traces Direct file processing
🟢 Direct file processing — Improvements
|
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 1h 37m: Pass: 100%/158 | Total: 1d 05h | Max: 1h 03m | Hits: 100%/361175See results here. |
Fixes #10754