[libcu++] Disable remove_cv builtin for older nvcc - #11031
Conversation
|
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)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add older-NVCC builtin compatibility handling and a CUDA extended-lambda compile test for kernel default launch configurations. ChangesLaunch configuration compatibility
Suggested reviewers: Merge Risk: 🟡 Moderate · up to The PR routes older NVCC versions through the fallback remove_cv implementation, but its regression test does not actually instantiate the dependent launch-configuration checks needed to protect the reported compile-failure path; merge should wait for the test to exercise that behavior or for explicit owner acceptance of the gap. 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2213a094-0c04-4d18-9d5c-f35f0938211a
📒 Files selected for processing (1)
libcudacxx/test/libcudacxx/cuda/ccclrt/launch/default_config_extended_lambda.compile.pass.cpp
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| [[maybe_unused]] auto verify_lambda = [] __device__(auto config) { | ||
| static_assert(cuda::gpu_thread.count(cuda::block, config) == 256); | ||
| static_assert(cuda::block.count(cuda::grid, config) == 4); | ||
| }; | ||
|
|
||
| kernel_with_default_config kernel{cuda::make_config(block, grid, cuda::cooperative_launch())}; | ||
| static_assert(cuda::__is_kernel_config<decltype(kernel.default_config())>); | ||
| static_assert(cuda::__kernel_has_default_config<decltype(kernel)>); | ||
| (void) verify_lambda; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test file ---'
cat -n libcudacxx/test/libcudacxx/cuda/ccclrt/launch/default_config_extended_lambda.compile.pass.cpp
printf '%s\n' '--- applicable references ---'
for f in \
/tmp/coderabbit-repo-knowledge/nvidia-cccl-2f773195/references/libcudacxx.md \
.agent/skills/cccl-test/SKILL.md \
.agent/skills/cccl-style/SKILL.md \
CONTRIBUTING.md \
docs/infrastructure/ci/references/ci_overview.rst
do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
fi
done
printf '%s\n' '--- directly bound launch symbols ---'
rg -n -A8 -B8 \
'kernel_with_default_config|combine_with_default|__is_kernel_config|__kernel_has_default_config|make_config|gpu_thread|block\.count' \
libcudacxx/include libcudacxx/test/libcudacxx/cuda/ccclrt/launch \
-g '*.{h,hpp,cpp,cu,cuh}' \
| head -n 500Repository: NVIDIA/cccl
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- nearby regression and default-config tests ---'
cat -n libcudacxx/test/libcudacxx/cuda/ccclrt/launch/extended_lambda.pass.cpp | sed -n '1,90p'
cat -n libcudacxx/test/libcudacxx/cuda/ccclrt/launch/launch_smoke.cu | sed -n '270,335p'
printf '%s\n' '--- launch header files ---'
fd -t f . libcudacxx/include/cuda | rg 'launch|config|kernel'
printf '%s\n' '--- exact declarations and uses ---'
rg -n -g '*.{h,hpp,cpp,cu,cuh}' \
'combine_with_default|__kernel_has_default_config|__is_kernel_config|default_config\(' \
libcudacxx/include/cuda libcudacxx/test/libcudacxx/cuda/ccclrt/launchRepository: NVIDIA/cccl
Length of output: 7411
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- launch path ---'
cat -n libcudacxx/include/cuda/__launch/launch.h | sed -n '450,515p'
printf '%s\n' '--- configuration contract ---'
cat -n libcudacxx/include/cuda/__launch/configuration.h | sed -n '490,525p;585,625p'
printf '%s\n' '--- compile-test registration ---'
rg -n -A8 -B8 'default_config_extended_lambda|compile.pass.cpp|main\(int, char\*\*\)' \
libcudacxx/test libcudacxx/cmake libcudacxx -g '*.{cpp,cu,cmake,txt}' \
| head -n 240Repository: NVIDIA/cccl
Length of output: 30676
important: Instantiate verify_lambda and exercise cuda::launch. The generic lambda is only declared and discarded, so its dependent static_assert expressions are not instantiated. The test also does not reach __conf.combine_with_default(__kernel) in cuda::launch.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 36-36: syntax error
(syntaxError)
This comment has been minimized.
This comment has been minimized.
We have seen at least one codegen bug related to `__remove_cv` where NVCC 12.9 and NVCC 13.0 generated invalid code. Be safe and disable all new ones for anything below 13.3 which has been fully tested in CI
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
I have pushed a bit more conservative change.
There are similar compiler builtins, so I want to be sure we only enable them if we have full test coverage. So I disabled all the new gcc compiler builtins for NVCC
🥳 CI Workflow Results🟩 Finished in 5h 01m: Pass: 100%/183 | Total: 5d 13h | Max: 5h 01m | Hits: 44%/1532713See results here. AI failure analysis1. cugraph device_scalar construction selects deleted RMM rvalue overload · 1 jobExplanation: cugraph repeatedly constructs `rmm::device_scalar` from temporary literals, but the RMM 26.10 header used by the job deletes the matching rvalue constructor, causing the same error across many translation units. The PR's new workaround targets NVCC versions below 13.3, while this job used 13.3.73; because the log does not record the external RMM and cugraph commit SHAs, a CCCL regression is unproven and RAPIDS revision skew should be checked first. Evidence: Copy this prompt into a coding agentJobs: |
2 participants
Summary
__remove_cvbuiltin path for NVCC before 13.3.cuda::std::remove_cvon the existing fallback implementation for affected older NVCC toolchains.This fixes the cudax launch configuration compile failure observed on upstream/main after #10997, where
cudax.test.launchfailed inlaunch_smoke.cuunder older NVCC plus newer GCC.