Conversation
The environment-based DeviceReduce tests computed their expected kernel pointers with `decltype(d_in)` inside the `[&]` allowlist lambda. With nvcc 13.3 and MSVC as host compiler, decltype() of a variable captured by reference inside a lambda is misevaluated as a reference type, so the tests referenced (and instantiated) `Kernel<..., InputIt&, ...>` instead of the `Kernel<..., InputIt, ...>` the dispatch launches. The two are different instantiations with different host stubs, and the referenced one is never registered with the CUDA runtime, so the white-box kernel-allowlist check at catch2_test_env_launch_helper.h:97 failed. * Hoist the iterator type aliases out of the allowlist lambdas. * Build the allowlists from the same kernel-source types the dispatch uses, so the expected instantiations cannot drift from the launched ones. * Add DeterministicDeviceReduceKernelSource to the RFA dispatch, mirroring the KernelSource pattern used by the other dispatch layers, and pass the kernels into invoke_single_tile/invoke_passes as parameters. Verified on Windows with MSVC 14.50: CUDA 13.3.1 (previously failing) and CUDA 12.9 (regression check) both pass all 49 test cases. Fixes NVIDIA#9643
* Factor the near-identical allowlist lambdas of the two requirements-parametrized tests into one expected_reduce_kernels function template. As a function template, the iterator types are deduced at test scope, which also removes the lambda that triggered the decltype() misevaluation entirely. * Expose detail::rfa::default_kernel_source_t and use it both as the dispatch default and in the test, so the expected RFA kernel instantiations are derived from a single place. * Include <thrust/type_traits/unwrap_contiguous_iterator.h> directly instead of relying on transitive inclusion. * Document the decltype()-inside-lambda pitfall next to allowed_kernels() in the launch helper, where future allowlist tests will look. * Drop the redundant reduction_op_t alias and the stale commented-out UNSCOPED_CAPTURE block. Re-verified on Windows with MSVC 14.50 on both CUDA 13.3.1 and 12.9: reduce_env (lid_0 + lid_2), reduce_deterministic, reduce_nondeterministic, and reduce_env_api all pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/cccl/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughChangesDeterministic reduce kernel source abstraction
Assessment against linked issues
Suggested reviewers: Priority: ➖ Normal Change: Bug fix · Severity of issue fixed: Medium Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cub/test/catch2_test_device_reduce_env.cu (1)
505-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: The sum variant (Lines 550-565) is byte-identical to this block, and both mirror the
run_to_runbranch ofexpected_reduce_kernels. Extract a small helper so future kernel-source changes can't drift between the twonot_guaranteedtests. Optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 82c18609-29b3-482f-a43b-f810cba21503
📒 Files selected for processing (3)
cub/cub/device/dispatch/dispatch_reduce_deterministic.cuhcub/test/catch2_test_device_reduce_env.cucub/test/catch2_test_env_launch_helper.h
Adapt the kernel-source refactor to the deferred problem size support added in NVIDIA#9740: - DeterministicDeviceReduceKernelSource gains a KernelNumItemsT parameter and a DeferredSingleTileSecondKernel getter, mirroring DeviceReduceKernelSource. - invoke_single_tile/invoke_passes now take the whole kernel source instead of individual kernel pointers, matching DispatchReduce, so the deferred/immediate second-pass selection stays inside invoke_passes. - The test allowlists derive the kernel num-items type the same way the dispatch does, instead of respelling it.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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)
cub/cub/device/dispatch/dispatch_reduce_deterministic.cuh (1)
443-457: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winimportant: Pass
KernelSource{}beforefixed_grid_factory_t{}atcub/test/catch2_test_device_reduce_deferred.cu:148. The factory currently binds toKernelSource, which causes compilation to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: abb39f1c-b318-4ef3-816a-33078ce56858
📒 Files selected for processing (4)
cub/cub/device/dispatch/dispatch_reduce_deterministic.cuhcub/test/catch2_test_device_reduce_deferred.cucub/test/catch2_test_device_reduce_env.cucub/test/catch2_test_env_launch_helper.h
🚧 Files skipped from review as they are similar to previous changes (2)
- cub/test/catch2_test_device_reduce_env.cu
- cub/test/catch2_test_env_launch_helper.h
|
@bernhardmgruber If you have some time, could you take a look at this PR? I’d really appreciate it. Thank you! |
|
@edenfunf I believe the issue has been resolved already by @gevtushenko in #11103. I linked the issue and PR now. This PR also contains some refactoring of the test. If you want, you can rebase and we can try to upstream it. |
main fixed NVIDIA#9643 in NVIDIA#11103 by hoisting the decltypes out of the allowlist lambdas. This branch removes those lambdas entirely, so keep its expected_reduce_kernels version. The launch helper was folded into stream_registry_factory.h in NVIDIA#11240; move the allowlist note there.
|
@bernhardmgruber Thanks for linking #11103! Merged main and kept only the test refactoring:
Net -150 lines in |
edenfunf
changed the title
|
Thank you for trying to prevent issues like #9643 in the future! The kernel sources in your dispatch layer are artefacts of the type erasure required for CCCL.C. We are currently in the progress of getting rid of this, by using a full C++ JIT compiler for CCCL.C V2. Once we are there, we no longer need any kernel sources in the source code and would probably like to remove them. Can you think of a way to make the current test more robust without the need to reach for implementation details in the dispatch layer, like the kernel source? |
2 participants
Description
Refs #9643
#11103 fixed the failure itself by hoisting the
decltype()s out of the allowlist lambdas. This PR keeps the test-side refactoring that came out of investigating the same issue, so the pattern cannot come back and the allowlists cannot drift from what the dispatch launches.Background: the env-based
DeviceReducetests used to compute their expected kernel pointers fromdecltype(d_in)inside the[&]allowlist lambdas. With nvcc 13.3 and MSVC as host compiler,decltype()of a variable captured by reference inside a lambda is evaluated as a reference type, so the tests referencedKernel<..., constant_iterator&, ...>while the dispatch launchedKernel<..., constant_iterator, ...>. That is a different instantiation with its own host stub, never registered with the runtime, so the pointer check failed.Changes:
expected_reduce_kernels, shared by the two requirements-parametrized tests. Iterator types are deduced at function scope, the two near-identical lambdas are gone, and with them thedecltype()-of-a-capture pattern.DeterministicDeviceReduceKernelSourceplus adefault_kernel_source_talias todispatch_reduce_deterministic.cuh, following the pattern of the other dispatch layers.invoke_single_tileandinvoke_passestake the kernel source likeDispatchReducedoes. No functional change.allowed_kernels(), which moved tostream_registry_factory.hin Merge CUB launch wrapper headers #11240.Net effect on the test file is about -150 lines.
Verified on Windows / MSVC 14.50 / CUDA 13.3 / C++17 / sm_120:
reduce_env,reduce_deterministic,reduce_deferredandreduce_env_apiall pass.The
decltype()misevaluation itself still looks like a compiler regression worth tracking on the nvcc side. A trivial repro does not trigger it; it seems to need the surrounding template test function and a class-type iterator.Checklist