Skip to content

Extract environment boilerplate code from within the device interfaces to a separate header - #6622

Merged
gonidelis merged 4 commits into
NVIDIA:mainfrom
gonidelis:refactor_env_boilerplate
Dec 17, 2025
Merged

gonidelis merged 4 commits into
NVIDIA:mainfrom
gonidelis:refactor_env_boilerplate

Conversation

@gonidelis

Copy link
Copy Markdown
Member

fixes #5606

Boilerplate code for extracting types information (stream, mr, tuning_t etc.) is too big and repetitive across the new device environment based interfaces we introduced. This PR extracts the code into a separate function and re-uses it in the existing environment based device APIs that we have (DeviceScan and DeviceReduce).

Some consideration about the design for the reviewers:

  • Each device primitive has its own quirks regarding which deterministm_t is supported. For example DeviceReduce::Reduce can support both gpu_to_gpu and run_to_run determinism, while DeviceReduce::ArgMax/Min or DeviceScan only support run_to_run at the moment. That means the determinism heuristics cannot be incorporated into the boilerplate code. Future environment-based APIs must individually evaluate each algorithm to determine and support the appropriate deterministic types.
  • The existing boilerplate code uses a lambda callable to pass the specific deterministic algorithm implementation by packing the arguments.
      auto reduce_callable = [&](auto tuning, void* storage, size_t& bytes, auto... args) {
        using tuning_t = decltype(tuning);
        return reduce_impl<tuning_t>(storage, bytes, args...);
      };

      // Dispatch with environment - handles all boilerplate
      return detail::dispatch_with_env(
        env, determinism_t{}, reduce_callable, d_in, d_out, num_items, reduction_op, ::cuda::std::identity{}, init);
    }

I need some feedback on whether this interface on the dispatch_with_env() looks sane.

@gonidelis
gonidelis requested a review from a team as a code owner November 13, 2025 19:18
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Nov 13, 2025
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Nov 13, 2025

// Initial value
OutputExtremumT initial_value{::cuda::std::numeric_limits<InputValueT>::max()};
OutputExtremumT initial_value{::cuda::std::numeric_limits<InputValueT>::lowest()};

@gonidelis gonidelis Nov 13, 2025 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I need to figure out why this bug was not caught from the tests

@github-actions

This comment has been minimized.

@gonidelis
gonidelis force-pushed the refactor_env_boilerplate branch from c72dd09 to 03c1ee9 Compare November 13, 2025 20:38
@github-actions

This comment has been minimized.

Comment thread cub/cub/device/device_reduce.cuh Resolved
Comment thread cub/cub/device/device_reduce.cuh Resolved
Comment thread cub/cub/detail/env_dispatch.cuh Resolved
Comment thread cub/cub/detail/env_dispatch.cuh Resolved
Comment thread cub/cub/device/device_scan.cuh Resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gonidelis
gonidelis force-pushed the refactor_env_boilerplate branch from 0ae2fc1 to 020e232 Compare December 11, 2025 17:17
@github-actions

This comment has been minimized.

@gonidelis

gonidelis commented Dec 12, 2025 •

Copy link
Copy Markdown
Member Author

LLVMs 15-18 are not friendly with complex C++20 aggregate expressions codegen hence this PR breaks CCCL for these compilers/C++ version.

Investigating what's the best course of action. I see three options:

a) simplify the code in this pr
b) file a bug to llvm
c) reject pr

digging deeper to see what's the exact issue.

Comment thread cub/cub/detail/env_dispatch.cuh Resolved
Comment thread cub/cub/detail/env_dispatch.cuh Resolved
Comment thread cub/cub/device/device_reduce.cuh Resolved
@bernhardmgruber
bernhardmgruber force-pushed the refactor_env_boilerplate branch from 57d2bd5 to 401b345 Compare December 15, 2025 21:07
using tuning_t =
::cuda::std::execution::__query_result_or_t<EnvT, ::cuda::execution::__get_tuning_t, ::cuda::std::execution::env<>>;

// clang 15-19 ICE when using __query_result_or_t, so we just use the underlying machinery as a workaround

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i would promise we tried the same thing with @miscco

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gonidelis
gonidelis force-pushed the refactor_env_boilerplate branch from 96b593a to 8bef46f Compare December 16, 2025 09:40
@gonidelis
gonidelis enabled auto-merge (squash) December 16, 2025 09:40
@gonidelis

Copy link
Copy Markdown
Member Author

Once green this should be good to go

@github-actions

This comment has been minimized.

//! - Memory resource querying from environment
//!
//! @param env The execution environment
//! @param algorithm_callable Callable that invokes the algorithm implementation with determinism specified

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.

Suggested change
//! @param algorithm_callable Callable that invokes the algorithm implementation with determinism specified
//! @param algorithm_callable Callable that invokes the algorithm

Comment on lines +1155 to +1157
using tuning_t =
::cuda::std::execution::__query_result_or_t<EnvT, ::cuda::execution::__get_tuning_t, ::cuda::std::execution::env<>>;

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.

Why do we need this here?

Comment on lines +1904 to +1906
using tuning_t =
::cuda::std::execution::__query_result_or_t<EnvT, ::cuda::execution::__get_tuning_t, ::cuda::std::execution::env<>>;

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.

Also, I think this is unused.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

will patch it up

@github-actions

Copy link
Copy Markdown
Contributor

🥳 CI Workflow Results

🟩 Finished in 14h 56m: Pass: 100%/95 | Total: 5d 01h | Max: 5h 51m | Hits: 69%/91590

See results here.

@gonidelis
gonidelis merged commit 3ddaaf5 into NVIDIA:main Dec 17, 2025
211 of 214 checks passed
@github-project-automation github-project-automation Bot moved this from In Review to Done in CCCL Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Refactor and cleanup env-based CUB APIs and underlying implementation

4 participants