Skip to content

[cudax] Fix group_as accessing range through brackets - #12582

Open
davebayer wants to merge 1 commit into
NVIDIA:mainfrom
davebayer:fix_group_as
Open

davebayer wants to merge 1 commit into
NVIDIA:mainfrom
davebayer:fix_group_as

Conversation

@davebayer

Copy link
Copy Markdown
Contributor

No description provided.

@davebayer
davebayer requested a review from a team as a code owner November 10, 2026 19:54
@davebayer
davebayer requested a review from caugonnet November 10, 2026 19:54
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Nov 10, 2026
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Nov 10, 2026
@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved group mapping compatibility with forward-only ranges.
  • Tests
    • Expanded coverage for exhaustive and partial group mappings across runtime and compile-time ranges.

__group_as_map now retrieves unit counts by advancing an iterator. The tests add exhaustive and non-exhaustive mapping checks for forward-only ranges.

Changes: Forward-only group mapping

Layer / File(s) Summary
Iterator-based count mapping
cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh, cudax/test/coop/group/mapping/group_as.cu
__group_as_map reads unit counts sequentially through an iterator. Tests cover exhaustive and non-exhaustive mappings for {4, 12, 16}, and a non-exhaustive mapping for {4, 6, 8}.

Possibly related issues

Possibly related PRs

  • NVIDIA/cccl#12223: This PR changes group_as to accept viewable ranges; the current change replaces indexed count access with iterator-based access.

Change: Bug fix

Merge Risk: Low · up to 279c8

Certain supported custom count ranges may fail to compile or be read after exhaustion. These compatibility gaps need correction or owner awareness.

Security architecture risk: Low · up to 279c8

The intended forward-only inputs preserve the existing mapping behavior. A narrow contract concern remains: mapping traverses the counts repeatedly, but its input requirements do not enforce reusable iterators. No privilege expansion or isolation bypass is demonstrated.

Retained concerns

  • Low · architecture · inferred: The widened runtime range acceptance does not express the multipass invariant required by mapping. Accumulation can consume a sized single-pass range before the new iterator traversal reads it, potentially producing invalid iterator access or incorrect mapping within the calling device kernel. This is an inferred failure-containment concern under the declared constraints; documented indexing requirements and the absence of a concrete single-pass consumer limit the exposure established here.
Security review details

Security Blast Radius

  • inferred — The supported effect is on subgroup membership and lane-mask metadata in calling device kernels. The available evidence does not establish a downstream authorization, tenant-isolation, credential, or service boundary that consumes these results.

Trust Boundaries and Controls

  • observed — The named TestKernel and operator() routing subjects are inside an anonymous namespace and are launched by a test case with fixed count fixtures. Their changes do not constitute a new externally exposed production entrypoint.

Hardening Proposals

  • proposed — Make the reusable-iteration requirement explicit in the dynamic range contract, for example through an appropriate forward-range constraint, and align the documentation with the intended forward-only support.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review info

Run configuration

  • Source mode: Auto
  • Repository: NVIDIA/cccl
  • Branch: main
  • File: .coderabbit.yaml
  • Commit: dd063b7
  • Layers: Repository YAML
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 877c280f-a1df-4386-9cfd-116def47bd47
Commits

Reviewing files that changed from the base of the PR and between dd063b7 and 279c80b.

Files selected for processing: 2

  • cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh
  • cudax/test/coop/group/mapping/group_as.cu

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

}

::cuda::std::uint32_t __sum = 0;
auto __it = ::cuda::std::ranges::cbegin(__unit_counts);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Stability & Availability · Quick win

Supported by static analysis

Script executed:

sed -n '1,240p' cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh
rg -n 'group_as|__unit_counts|_CCCL_ASSERT' cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh

Repository: NVIDIA/cccl

Length of output: 13483


Script executed:

git diff dd063b73e63e18f911182f79879fbf7d558ebe79 279c80bebbf42e46b24c012727c7a4a4e13b5f48 -- cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh
rg -n -F -e '#define _CCCL_ASSERT' -e '#  define _CCCL_ASSERT' -e '#    define _CCCL_ASSERT' libcudacxx/include
rg -n -F -e 'sized_range' -e 'forward_range' libcudacxx/include/cuda/std/__ranges/concepts.h
rg -n -F -e 'group_as' cudax/test/coop cudax/include/cuda/experimental/coop/__group/mapping 2>/dev/null

Repository: NVIDIA/cccl

Length of output: 15944


Script executed:

cat -n libcudacxx/include/cuda/std/__cccl/assert.h | sed -n '34,170p'
cat -n libcudacxx/include/cuda/std/__numeric/accumulate.h | sed -n '20,110p'
cat -n libcudacxx/include/cuda/std/__ranges/concepts.h | sed -n '55,120p'
cat -n libcudacxx/include/cuda/std/__ranges/all.h | sed -n '35,155p'
cat -n cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh | sed -n '53,153p'
cat -n cudax/test/coop/group/mapping/group_as.cu | sed -n '28,105p;325,350p'
rg -n -F -e 'group_as' docs/cudax/coop/group
rg -n -F -e 'weakly_incrementable' -e 'input_iterator' libcudacxx/include/cuda/std/__iterator/concepts.h

Repository: NVIDIA/cccl

Length of output: 27079


Script executed:

cat -n docs/cudax/coop/group/mapping/group_as.rst | sed -n '1,105p'
cat -n libcudacxx/include/cuda/std/__iterator/concepts.h | sed -n '92,158p'
cat -n libcudacxx/include/cuda/std/__ranges/ref_view.h | sed -n '35,150p'
cat -n libcudacxx/include/cuda/std/__ranges/access.h | sed -n '230,320p'

Repository: NVIDIA/cccl

Length of output: 13390


important: Process single-pass counts in one traversal.

With device assertions disabled, non-exhaustive validation can consume a single-pass range before the new traversal dereferences it. With assertions enabled, the earlier assertion can also consume the range and cause a valid mapping to return invalid. Combine validation and mapping in one traversal while preserving the existing range contract.

for (::cuda::std::uint32_t __i = 0; __i < __curr_ngroups; ++__i)
{
const auto __i_count = static_cast<::cuda::std::uint32_t>(__unit_counts[__i]);
const auto __i_count = static_cast<::cuda::std::uint32_t>(*__it++);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

sed -n '1,240p' cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh
rg -n 'group_as|sized_range|forward_range|input_range' cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh cudax/test/coop/group/mapping/group_as.cu

Repository: NVIDIA/cccl

Length of output: 20410


Script executed:

cat -n cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh | sed -n '48,155p;245,275p'
git diff dd063b73e63e18f911182f79879fbf7d558ebe79 279c80bebbf42e46b24c012727c7a4a4e13b5f48 -- cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh
rg -n -F -e 'weakly_incrementable' -e 'input_or_output_iterator' -e 'input_iterator' libcudacxx/include/cuda/std/__iterator/concepts.h
rg -n -F -e 'sized_range' -e 'viewable_range' -e 'input_range' libcudacxx/include/cuda/std/__ranges/concepts.h
rg -n -F -e 'accumulate(' libcudacxx/include/cuda/std/__numeric/accumulate.h
cat -n libcudacxx/include/cuda/std/__numeric/accumulate.h | sed -n '20,85p'
cat -n libcudacxx/include/cuda/std/__iterator/concepts.h | sed -n '70,200p'
cat -n libcudacxx/include/cuda/std/__ranges/concepts.h | sed -n '45,170p'
cat -n cudax/test/coop/group/mapping/group_as.cu | sed -n '125,207p'

Repository: NVIDIA/cccl

Length of output: 26280


important: Avoid dereferencing the postfix result.

A sized input range can have fresh iterators whose postfix increment returns void. For such a range, group_as::map can fail to compile. Read the value before using prefix increment.

Fix

--- "a/cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh"
+++ "b/cudax/include/cuda/experimental/coop/__group/mapping/group_as.cuh"
@@ -100,7 +100,8 @@
   _CCCL_PRAGMA_UNROLL_FULL()
   for (::cuda::std::uint32_t __i = 0; __i < __curr_ngroups; ++__i)
   {
-    const auto __i_count = static_cast<::cuda::std::uint32_t>(*__it++);
+    const auto __i_count = static_cast<::cuda::std::uint32_t>(*__it);
+    ++__it;
     if (__prev_unit_rank < __sum + __i_count)
     {
       const auto __group_rank = __prev_mapping_result.group_rank() * __curr_ngroups + __i;
Committable suggestion

IMPORTANT
Review the replacement and indentation, then test before committing.

Suggested change
const auto __i_count = static_cast<::cuda::std::uint32_t>(*__it++);
const auto __i_count = static_cast<::cuda::std::uint32_t>(*__it);
++__it;

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

3 participants