libcu++: atomics SASS FileCheck suite for basic atomic operations - #10722
libcu++: atomics SASS FileCheck suite for basic atomic operations#10722griwes wants to merge 11 commits into
Conversation
|
/ok to test eacf3c3 |
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds CV-unqualified ChangesAtomic codegen and API
Assessment against linked issues
Possibly related PRs
Suggested reviewers: Mergeability Score: 🟡 Moderate · up to The new atomic code-generation tests can target unsupported architectures for 128-bit atomic_ref load/store operations, causing CI failures before the expected assembly is produced. The architecture gating should be corrected or explicitly accepted before merging. 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: 8
🧹 Nitpick comments (4)
libcudacxx/test/cmake/CodegenTest.cmake (2)
359-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: The variant and prefix detection reads the test source at configure time, but nothing marks the source as a configure dependency. If a developer edits
%PARAM%lines or adds a new SASS check prefix, CMake does not re-run and the generated targets go stale. Add the test files toCMAKE_CONFIGURE_DEPENDS.foreach (test_path IN LISTS arg_TESTS) file(READ "${test_path}" test_contents) + set_property( + DIRECTORY + APPEND + PROPERTY CMAKE_CONFIGURE_DEPENDS "${test_path}" + )
245-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Handle
-Wno-commentper CUDA compiler. Pass it through-Xcompiler=-Wno-commentfor NVCC, keep the direct flag for Clang-CUDA, and build representative PTX and SASS targets with both compilers.libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.cu (2)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Use angle-bracket syntax for
atomic_codegen_helpers.h. Ensure the SASS test target exposes this directory as an include directory before changing the includes.
libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.cu#L17-L17: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.cu#L16-L16: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.cu#L20-L20: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.cu#L20-L20: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/load_volatile_apis.cu#L16-L16: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/load_volatile_types_128_atomic_ref.cu#L17-L17: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/store_volatile_apis.cu#L16-L16: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/store_volatile_types.cu#L17-L17: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/store_volatile_types_128_atomic_ref.cu#L17-L17: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic.cu#L20-L20: Change the local helper include to angle-bracket syntax.libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.cu#L20-L20: Change the local helper include to angle-bracket syntax.As per coding guidelines, “All header inclusions must use angle-bracket syntax.”
Source: Coding guidelines
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Declare each unmodified by-value operand
const.
libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.cu#L19-L21: Declaredesiredasint32_t const.libcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.cu#L18-L20: Declarevalueasint32_t const.libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.cu#L22-L24: DeclarevalueasTYPE const.libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.cu#L22-L24: DeclarevalueasTYPE const.libcudacxx/test/atomic_codegen/sass/store_volatile_apis.cu#L18-L20: Declarevalueasint32_t const.libcudacxx/test/atomic_codegen/sass/store_volatile_types.cu#L19-L21: DeclarevalueasTYPE const.libcudacxx/test/atomic_codegen/sass/store_volatile_types_128_atomic_ref.cu#L19-L21: DeclarevalueasTYPE const.libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic.cu#L22-L24: DeclarevalueasTYPE const.libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.cu#L22-L24: DeclarevalueasTYPE const.As per coding guidelines, “All variables that are not modified must be declared
const.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6ae497f-de44-4f9c-b202-39d99adc1800
📒 Files selected for processing (78)
.github/actions/workflow-build/build-workflow.pyCMakePresets.jsonci/build_libcudacxx.shci/matrix.yamlcmake/CCCLTestParams.cmakecub/test/test_param_tuple_fail.cudocs/cccl/development/testing.rstlibcudacxx/codegen/generators/definitions.hlibcudacxx/codegen/generators/ld_st.hlibcudacxx/include/cuda/__atomic/atomic.hlibcudacxx/include/cuda/std/__atomic/api/common.hlibcudacxx/include/cuda/std/__atomic/api/owned.hlibcudacxx/include/cuda/std/__atomic/api/reference.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.hlibcudacxx/include/cuda/std/__atomic/types/base.hlibcudacxx/include/cuda/std/atomiclibcudacxx/test/CMakeLists.txtlibcudacxx/test/atomic_codegen/CMakeLists.txtlibcudacxx/test/atomic_codegen/atomic_add_non_volatile.culibcudacxx/test/atomic_codegen/atomic_cas_non_volatile.culibcudacxx/test/atomic_codegen/atomic_exch_non_volatile.culibcudacxx/test/atomic_codegen/atomic_load_non_volatile.culibcudacxx/test/atomic_codegen/atomic_store_non_volatile.culibcudacxx/test/atomic_codegen/atomic_sub_non_volatile.culibcudacxx/test/atomic_codegen/dump_and_check.bashlibcudacxx/test/atomic_codegen/sass/CMakeLists.txtlibcudacxx/test/atomic_codegen/sass/atomic_codegen_helpers.hlibcudacxx/test/atomic_codegen/sass/compare_exchange_apis.culibcudacxx/test/atomic_codegen/sass/compare_exchange_single_order.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_apis.culibcudacxx/test/atomic_codegen/sass/exchange_types.culibcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_apis.culibcudacxx/test/atomic_codegen/sass/load_types.culibcudacxx/test/atomic_codegen/sass/load_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/load_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_volatile_apis.culibcudacxx/test/atomic_codegen/sass/load_volatile_types.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_apis.culibcudacxx/test/atomic_codegen/sass/store_types.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_volatile_apis.culibcudacxx/test/atomic_codegen/sass/store_volatile_types.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.culibcudacxx/test/cmake/CodegenTest.cmakelibcudacxx/test/codegen/dump_and_check.bashlibcudacxx/test/libcudacxx/std/atomics/atomics.types.generic/atomic_ref_volatile.pass.cpplibcudacxx/test/simd_codegen/CMakeLists.txtlibcudacxx/test/simd_codegen/load_store/CMakeLists.txt
💤 Files with no reviewable changes (2)
- libcudacxx/test/atomic_codegen/dump_and_check.bash
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.h
eacf3c3 to
ea162db
Compare
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.
ea162db to
cc657fb
Compare
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)
libcudacxx/test/cmake/CodegenTest.cmake (1)
165-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winimportant: Preserve numeric
-PLUSchecks foraandfarchitectures. Line 171 clearsSM100-PLUSfor100aand100f. The later registration logic then skips a test that has only this prefix. Extract the numeric architecture before the comparison instead of discardingplus_prefixes.+ string(REGEX MATCH "^[0-9]+" arch_number "${arch}") - if ("${arch}" MATCHES "[af]$") - set(plus_prefixes) - endif() foreach (plus_prefix IN LISTS plus_prefixes) string(REGEX REPLACE ".*SM([0-9]+)-PLUS.*" "\\1" plus_arch "${plus_prefix}") - if (arch GREATER_EQUAL plus_arch) + if (arch_number GREATER_EQUAL plus_arch) list(APPEND check_prefixes "SM${plus_arch}-PLUS") endif() endforeach()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a4335fd-7a95-4795-b532-6a0bd14b9877
📒 Files selected for processing (15)
.github/actions/workflow-build/build-workflow.pyci/matrix.yamllibcudacxx/include/cuda/std/__atomic/types/reference.hlibcudacxx/test/atomic_codegen/atomic_add_non_volatile.culibcudacxx/test/atomic_codegen/atomic_exch_non_volatile.culibcudacxx/test/atomic_codegen/atomic_load_non_volatile.culibcudacxx/test/atomic_codegen/atomic_sub_non_volatile.culibcudacxx/test/atomic_codegen/sass/CMakeLists.txtlibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_apis.culibcudacxx/test/atomic_codegen/sass/store_types.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.culibcudacxx/test/cmake/CodegenTest.cmake
🚧 Files skipped from review as they are similar to previous changes (13)
- libcudacxx/test/atomic_codegen/atomic_load_non_volatile.cu
- libcudacxx/test/atomic_codegen/atomic_sub_non_volatile.cu
- libcudacxx/test/atomic_codegen/atomic_exch_non_volatile.cu
- libcudacxx/test/atomic_codegen/sass/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/store_types.cu
- libcudacxx/test/atomic_codegen/sass/store_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/atomic_add_non_volatile.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.cu
- ci/matrix.yaml
- .github/actions/workflow-build/build-workflow.py
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/store_apis.cu
|
And also sneaks in 8bit ld/st. This looks good so far. I'm wary that the SASS checks will ever be clean, but maybe if we make them loose enough with the regex it'll work. |
|
Right. I did a couple of obvious codegen fixes too, I will edit the description to point them out. |
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.
I'm about to push a version that does not include the contents of #10678, since that is now merged. Please further specify what you'd like separated, if anything. |
Keep the changes relevant to just the PR title/linked issue of adding the SASS FileCheck suite and any changes to the actual code should be made in other PRs. |
cac19be to
94d21a7
Compare
|
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.
🔇 Additional comments (5)
libcudacxx/test/codegen/dump_and_check.bash (2)
9-12: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.important: Line 9 treats the first FileCheck option as a cuobjdump mode when the caller omits the optional mode. For example,
--enable-var-scopebecomes thecuobjdumpargument and the command fails. Consume an argument only when it is a supported dump mode, or require an explicit separator before FileCheck options.-dump_mode="${1:---dump-ptx}" -if (( $# > 0 )); then - shift -fi +dump_mode="--dump-ptx" +case "${1-}" in + --dump-ptx|--dump-sass) + dump_mode="${1}" + shift + ;; +esac
16-25: LGTM!libcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic_ref.cu (1)
1-40: LGTM!libcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic.cu (1)
1-38: LGTM!ci/matrix.yaml (1)
115-124: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.important: Verify the
codegen_targetcontract end to end.These entries pass
codegen_targetas both a scalar and a list. Confirm that the workflow expands list values into separate jobs, forwards each selected value toci/build_libcudacxx.sh, and resolves it throughcodegen_targetsto the matching CMake target. Otherwise a job can run all suites, pass a list as one selector, or ignore the selection.As per path instructions, validate CI matrix correctness and targeted build/test behavior.
Also applies to: 299-310, 420-431, 646-646, 696-701, 829-833
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43d1eac1-6afc-412e-9416-52390195b0c8
📒 Files selected for processing (75)
.github/actions/workflow-build/build-workflow.pyCMakePresets.jsonci/build_libcudacxx.shci/matrix.yamllibcudacxx/codegen/generators/definitions.hlibcudacxx/codegen/generators/ld_st.hlibcudacxx/include/cuda/__atomic/atomic.hlibcudacxx/include/cuda/std/__atomic/api/common.hlibcudacxx/include/cuda/std/__atomic/api/owned.hlibcudacxx/include/cuda/std/__atomic/api/reference.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.hlibcudacxx/include/cuda/std/__atomic/types/base.hlibcudacxx/include/cuda/std/__atomic/types/reference.hlibcudacxx/include/cuda/std/atomiclibcudacxx/test/CMakeLists.txtlibcudacxx/test/atomic_codegen/CMakeLists.txtlibcudacxx/test/atomic_codegen/atomic_add_non_volatile.culibcudacxx/test/atomic_codegen/atomic_cas_non_volatile.culibcudacxx/test/atomic_codegen/atomic_exch_non_volatile.culibcudacxx/test/atomic_codegen/atomic_load_non_volatile.culibcudacxx/test/atomic_codegen/atomic_store_non_volatile.culibcudacxx/test/atomic_codegen/atomic_sub_non_volatile.culibcudacxx/test/atomic_codegen/sass/CMakeLists.txtlibcudacxx/test/atomic_codegen/sass/atomic_codegen_helpers.hlibcudacxx/test/atomic_codegen/sass/compare_exchange_apis.culibcudacxx/test/atomic_codegen/sass/compare_exchange_single_order.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_apis.culibcudacxx/test/atomic_codegen/sass/exchange_types.culibcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_apis.culibcudacxx/test/atomic_codegen/sass/load_types.culibcudacxx/test/atomic_codegen/sass/load_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/load_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_volatile_apis.culibcudacxx/test/atomic_codegen/sass/load_volatile_types.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_apis.culibcudacxx/test/atomic_codegen/sass/store_types.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_volatile_apis.culibcudacxx/test/atomic_codegen/sass/store_volatile_types.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.culibcudacxx/test/cmake/CodegenTest.cmakelibcudacxx/test/codegen/dump_and_check.bashlibcudacxx/test/libcudacxx/std/atomics/atomics.types.generic/atomic_ref_volatile.pass.cpplibcudacxx/test/simd_codegen/CMakeLists.txtlibcudacxx/test/simd_codegen/load_store/CMakeLists.txt
💤 Files with no reviewable changes (1)
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.h
🚧 Files skipped from review as they are similar to previous changes (70)
- libcudacxx/include/cuda/std/atomic
- libcudacxx/codegen/generators/definitions.h
- libcudacxx/include/cuda/std/__atomic/api/owned.h
- ci/build_libcudacxx.sh
- libcudacxx/test/atomic_codegen/atomic_add_non_volatile.cu
- libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/atomic_cas_non_volatile.cu
- libcudacxx/test/atomic_codegen/atomic_store_non_volatile.cu
- libcudacxx/test/atomic_codegen/atomic_exch_non_volatile.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic_ref.cu
- libcudacxx/include/cuda/__atomic/atomic.h
- libcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/store_types.cu
- libcudacxx/test/atomic_codegen/sass/load_volatile_apis.cu
- libcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/atomic_sub_non_volatile.cu
- libcudacxx/test/atomic_codegen/sass/load_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/atomic_load_non_volatile.cu
- libcudacxx/include/cuda/std/__atomic/types/reference.h
- libcudacxx/test/atomic_codegen/sass/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types.cu
- libcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_single_order.cu
- libcudacxx/test/atomic_codegen/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/load_apis.cu
- libcudacxx/test/simd_codegen/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/compare_exchange_apis.cu
- libcudacxx/test/simd_codegen/load_store/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types.cu
- libcudacxx/test/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/load_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_types.cu
- libcudacxx/test/atomic_codegen/sass/load_volatile_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/exchange_apis.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/store_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/load_volatile_types.cu
- libcudacxx/test/atomic_codegen/sass/atomic_codegen_helpers.h
- libcudacxx/test/atomic_codegen/sass/store_volatile_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/store_volatile_types.cu
- libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.cu
- libcudacxx/include/cuda/std/__atomic/api/reference.h
- libcudacxx/test/libcudacxx/std/atomics/atomics.types.generic/atomic_ref_volatile.pass.cpp
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.cu
- libcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types.cu
- libcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/store_apis.cu
- .github/actions/workflow-build/build-workflow.py
- libcudacxx/include/cuda/std/__atomic/api/common.h
- libcudacxx/include/cuda/std/__atomic/types/base.h
- libcudacxx/codegen/generators/ld_st.h
- libcudacxx/test/atomic_codegen/sass/store_volatile_apis.cu
- libcudacxx/test/cmake/CodegenTest.cmake
- CMakePresets.json
- libcudacxx/test/atomic_codegen/sass/load_types.cu
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.h
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.cu
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… volatile codegen tests.
94d21a7 to
fb4728b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libcudacxx/test/cmake/CodegenTest.cmake (1)
256-279: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winsuggestion:
standalone_prefixescan become empty when every active prefix is only aPREFIX_COMBINEcomponent and no combination is active. Thencheck_suffixis empty, the check target name degrades to<path>..check, andfilecheck_prefixesmay also be empty, which makes FileCheck fail with an unhelpful error. Add a guard that reports the test path and the offending prefix set at configure time.Also applies to: 302-309
libcudacxx/test/atomic_codegen/sass/exchange_apis.cu (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: The exchange SASS checks encode register allocation, which is not the property under test. A register-allocation change in a new CTK or a new architecture will break these checks without any real codegen regression. Match the operands with capture patterns instead.
libcudacxx/test/atomic_codegen/sass/exchange_apis.cu#L37-L38: replacePTwith{{P(T|[0-9]+)}}andR4/R6with{{R[0-9]+}}.libcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.cu#L39-L40: apply the same replacement.libcudacxx/test/atomic_codegen/sass/exchange_volatile_types.cu#L40-L41: replaceR4/R6with{{R[0-9]+}}; the predicate is already flexible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d837d30e-1780-472b-9da5-3bfc8b7bf3a1
📒 Files selected for processing (48)
libcudacxx/test/atomic_codegen/sass/CMakeLists.txtlibcudacxx/test/atomic_codegen/sass/compare_exchange_apis.culibcudacxx/test/atomic_codegen/sass/compare_exchange_single_order.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_apis.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_apis.culibcudacxx/test/atomic_codegen/sass/exchange_types.culibcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_apis.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_apis.culibcudacxx/test/atomic_codegen/sass/load_types.culibcudacxx/test/atomic_codegen/sass/load_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/load_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_volatile_apis.culibcudacxx/test/atomic_codegen/sass/load_volatile_types.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/load_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_apis.culibcudacxx/test/atomic_codegen/sass/store_types.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_volatile_apis.culibcudacxx/test/atomic_codegen/sass/store_volatile_types.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_128_atomic_ref.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic.culibcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.culibcudacxx/test/cmake/CodegenTest.cmakelibcudacxx/test/codegen/dump_and_check.bash
🚧 Files skipped from review as they are similar to previous changes (24)
- libcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/store_volatile_apis.cu
- libcudacxx/test/atomic_codegen/sass/CMakeLists.txt
- libcudacxx/test/atomic_codegen/sass/store_types.cu
- libcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/store_volatile_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_types.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/load_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/load_volatile_types.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_single_order.cu
- libcudacxx/test/atomic_codegen/sass/load_apis.cu
- libcudacxx/test/atomic_codegen/sass/store_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/load_types_8_16_atomic.cu
- libcudacxx/test/atomic_codegen/sass/store_types_128_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic_ref.cu
- libcudacxx/test/atomic_codegen/sass/load_types_128_atomic.cu
- libcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types.cu
- libcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic.cu
⏱️ CCCL compile-time benchmark comparison: Public headers compile-time benchResult: 1 regression row(s), 1 improvement row(s) above threshold.
Artifacts: reports and traces Direct file processing
🔴 Direct file processing — Regressions
🟢 Direct file processing — Improvements
|
😬 CI Workflow Results🟥 Finished in 2h 31m: Pass: 98%/597 | Total: 6d 03h | Max: 2h 30m | Hits: 81%/795244See results here. AI failure analysis1. Compare-exchange SASS check incorrectly requires adjacent acquire invalidation · 8 jobsExplanation: All eight atomic SASS matrix jobs fail because the new `NON_BLOCK_ACQUIRE-NEXT` assertion requires `CCTL.IVALL` immediately after `ATOM.E.CAS`. Generated SASS consistently places comparison or uniform-load instructions between them, so valid output is rejected across CUDA versions and architectures. Evidence: Root cause: The PR introduced an over-constrained FileCheck directive at line 40. Acquire compare-exchange still emits `CCTL.IVALL`, but result-processing instructions legitimately prevent it from being the next SASS line after the CAS instruction. Sources: libcudacxx/test/atomic_codegen/sass/compare_exchange_apis.cu:40. Suggested next steps: Change `NON_BLOCK_ACQUIRE-NEXT` to `NON_BLOCK_ACQUIRE`, then validate both architectures with `./ci/build_libcudacxx.sh -codegen-tests -std 20 -arch 75 -target libcudacxx.test.atomics.sass -cmake-options -DLIBCUDACXX_CODEGEN_FILECHECK_TESTS=atomics-sass` and the equivalent `-arch 80` command. Copy this prompt into a coding agentJobs:
2. Two Windows example subprocesses exit nonzero without diagnostic output · 1 jobExplanation: The partition-object and struct-reduction examples succeed when imported, but fail when the test harness launches their files as child processes. The harness reports empty stderr and omits the child return code and stdout, preventing identification of the underlying Windows process failure. Evidence: Root cause: Both child Python processes returned nonzero on Windows with Python 3.14, but the saved log contains no return code or child output that identifies whether this was an assertion, native CUDA crash, or environment issue. The PR does not modify the Python examples or their test harness. Sources: python/cuda_cccl/tests/test_examples.py:119, python/cuda_cccl/tests/test_examples.py:125, python/cuda_cccl/tests/test_examples.py:126. Suggested next steps: First include `result.returncode`, `result.stdout`, and `result.stderr` in the raised exception, then rerun only the two failing tests through `./ci/windows/test_cuda_cccl_examples_python.ps1 -py-version 3.14 -ctk-mode sysctk`. Use the resulting Windows status code or traceback to determine the actual example or runtime fix. Copy this prompt into a coding agentJobs: 3. NVBench configuration cannot clone public fmt dependency · 1 jobExplanation: NVBench helper configuration stops while CPM/FetchContent clones `fmt` 11.2.0. Git retries three times but attempts an unavailable interactive GitHub credential flow, so CMake cannot populate the dependency. Evidence: NVBench Helper nvcc GCC / [CTK13.3 GCC15] Test(amd64, T4), step 4 Root cause: The runner's Git transport, credential configuration, proxy, or GitHub access failed for a public dependency; the log does not distinguish among those environment mechanisms. This occurs before compilation and is unrelated to the PR's atomic changes. Suggested next steps: Retry `./ci/test_nvbench_helper.sh`; if it remains reproducible, inspect runner Git URL rewrites and credentials and run `git ls-remote https://github.com/fmtlib/fmt.git`. Correct the runner access configuration rather than changing NVBench code unless public Git clones are intentionally unsupported. Copy this prompt into a coding agentJobs: |
4 participants
Description
Resolves #10664.
This PR introduces a first SASS FileCheck suite for the atomic codegen, which includes a wide coverage of volatile and non-volatile codegen results for load, store, exchange, compare_exchange_weak, and compare_exchange_strong. It currently also incorporates the contents of #10671 and #10678, which should be merged first.
This PR is larger in scope than follow ups for other operations will be; this is because it also establishes the general shape of the SASS FileCheck suite for atomics, as well as the new CI jobs for libcu++ codegen testing, separated from regular libcu++ testing jobs.
The volatile changes will be removed from this PR once #10727 is merged.
Note for reviewers: the relevant changes to be reviewed within this PR are in the last three commits. The rest is contained in the aforementioned other PRs.
This PR introduces a non-trivial number of new CI jobs, though most of them are themselves quite trivial. Here's a breakdown of the relevant numbers:
Checklist