libcu++: atomic backend dispatch refactor - #10908
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test a4384f0 |
|
/ok to test 99bb8b9 |
|
/ok to test 7ed88e2 |
😬 CI Workflow Results🟥 Finished in 1h 32m: Pass: 75%/117 | Total: 2d 13h | Max: 1h 31m | Hits: 54%/486751See results here. AI failure analysis1. Windows macro expansion removes the `__bound` callable · 19 jobsExplanation: Nineteen Windows nvcc builds fail while parsing the new fetch fallback dispatch. The compiler output shows the callable argument disappearing, leaving an empty argument between commas. Evidence: CUB nvcc MSVC / [CTK13.3 MSVC14.50 C++20] BuildNoLaunch(amd64): sm{86}, step 4 cudax nvcc MSVC / [CTK13.3 MSVC14.50 C++20] Build(amd64): sm{75}, step 4 CUB nvcc MSVC / [CTK13.3 MSVC14.50 C++20] BuildHostLaunch(amd64): sm{86}, step 4 Root cause: The new local variable is named `__bound`, which collides with a Windows toolchain macro and preprocesses to nothing. Both affected calls therefore contain an empty second argument. Sources: libcudacxx/include/cuda/std/__atomic/functions/dispatch.h:69, libcudacxx/include/cuda/std/__atomic/functions/dispatch.h:71, libcudacxx/include/cuda/std/__atomic/functions/dispatch.h:98, libcudacxx/include/cuda/std/__atomic/functions/dispatch.h:100. Suggested next steps: Rename the two exact `__bound` locals to collision-resistant names such as `__bound_fetch_fallback` and `__bound_fetch_sub`, then rerun one Windows nvcc header build before the broader Windows matrix. Copy this prompt into a coding agentJobs:
2. GCC 7 maps 32-bit atomics to the 128-bit proxy · 2 jobsExplanation: Both GCC 7 public-header builds instantiate the experimental 128-bit host implementation while compiling ordinary `atomic<int>` operations. This makes merely including and instantiating the public atomic API fail without the experimental opt-in. Evidence: libcu++ nvcc GCC / [CTK12.0 GCC7 C++17] Build(amd64), step 4 libcu++ nvcc GCC / [CTK12.9 GCC7 C++17] Build(amd64), step 4 Root cause: The new `__cuda_atomic_deduce_bitwise` implementation uses `__type_switch` with a 128-bit default, and under GCC 7 the observed instantiation for `_Type = int` resolves to `__cuda_atomic_longlong2`. That erroneous proxy selection reaches the intentional host 128-bit static assertion; the logs do not establish whether this is a GCC 7 alias-template limitation or a defect in the switch expression itself. Sources: libcudacxx/include/cuda/std/__atomic/functions/backend.h:279, libcudacxx/include/cuda/std/__atomic/functions/backend.h:286, libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.h:1828, libcudacxx/include/cuda/std/__atomic/functions/host.h:48. Suggested next steps: Add a GCC 7 compile-time check that the bitwise deduction for 1/2/4/8-byte types selects the matching proxy, then replace or adjust the `__type_switch` formulation if it fails. Validate with `cmake --build <gcc7-build-dir> --target libcudacxx.test.public_headers_host_only.base`. Copy this prompt into a coding agentJobs: 3. Generated fence code leaves `__memorder` unused under Clang CUDA · 3 jobsExplanation: All three Clang 21 CUDA configurations fail their first lit compilations because warnings are errors and the generated thread-fence function declares an unused `__memorder`. The failure affects thousands of tests that include the atomic headers. Evidence: libcu++ ClangCUDA / [CTK12.9 Clang21 C++20] Build(amd64): sm{75;80;90;100;120}, step 4 libcu++ ClangCUDA / [CTK12.9 Clang21 C++17] Build(amd64): sm{75;80;90;100;120}, step 4 libcu++ ClangCUDA / [CTK12.9 Clang21 C++23] Build(amd64): sm{75;80;90;100;120}, step 4 Root cause: For some Clang CUDA compilation passes, `NV_DISPATCH_TARGET` removes the branches that reference `__memorder`, but the variable remains declared. Because libcudacxx lit builds use `-Werror`, this warning stops compilation. Sources: libcudacxx/codegen/generators/fence.h:83, libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.h:78. Suggested next steps: Mark the generated variable `[[maybe_unused]]` or move order conversion into the branches that consume it, then regenerate `cuda_ptx_generated.h`. Validate with `cmake --build <clang21-build-dir> --target libcudacxx.test.lit.precompile`. Copy this prompt into a coding agentJobs: 4. 16-byte atomic-ref runtime exceeds the lit timeout · 1 jobExplanation: The H100 test build completes most of the suite, but the 16-byte integral `atomic_ref` executable never finishes before the per-test timeout. This leaves the test unresolved and fails the test target. Evidence: libcu++ nvcc GCC / FU / [CTK13.3 GCC15 C++20] Test(amd64, H100 2-GPU): sm{90}, step 4 Root cause: The refactored 128-bit atomic path causes the test executable to hang or regress beyond the timeout, plausibly in the new generic CAS retry loop. The killed process produced no operation-level trace, so the logs cannot identify which load, exchange, compare-exchange, or fetch operation fails to make progress. Sources: libcudacxx/test/libcudacxx/std/atomics/atomics.types.generic/integral/16b_integral_ref.pass.cpp:105, libcudacxx/test/libcudacxx/std/atomics/atomics.types.generic/integral/16b_integral_ref.pass.cpp:175, libcudacxx/include/cuda/std/__atomic/functions/generic_rmw.h:181, libcudacxx/include/cuda/std/__atomic/functions/generic_rmw.h:191. Suggested next steps: Run only this lit test on an H100 with a shorter timeout and temporary progress markers to isolate the first hanging operation; do not increase the timeout as the primary fix. Use `LIBCUDACXX_SITE_CONFIG=<build>/libcudacxx/test/libcudacxx/lit.site.cfg lit -v libcudacxx/test/libcudacxx/std/atomics/atomics.types.generic/integral/16b_integral_ref.pass.cpp`. Copy this prompt into a coding agentJobs: |
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
Suggestion: I would start with a PR that just does the rename to __cuda_atomic then followed by some of the cleanups like adding _CCCL_HOST_DEVICE_API and so on
That would massively limit the scope of this PR
| out << R"XXX( | ||
| template <class _Fn, class _Sco> | ||
| static inline _CCCL_DEVICE bool __cuda_atomic_compare_swap_memory_order_dispatch(_Fn& __cuda_cas, int __success_memorder, int __failure_memorder, _Sco) { | ||
| static inline _CCCL_DEVICE bool __cuda_atomic_compare_exchange_order_dispatch( |
There was a problem hiding this comment.
Nitpick: This could be
static _CCCL_DEVICE_API bool
| template <typename _Atomic_Memorder> | ||
| inline _CCCL_DEVICE bool operator()(_Atomic_Memorder) { | ||
| return __cuda_atomic_compare_exchange(__ptr, *__exp, *__exp, *__des, _Atomic_Memorder{}, _Tag{}, _Sco{}); | ||
| _CCCL_HOST_DEVICE_API bool operator()(_Atomic_Memorder __order) { |
There was a problem hiding this comment.
Nitpick: We should add [[nodiscard]] to the internal APIs
| { | ||
| using __proxy_t = typename __atomic_cuda_deduce_bitwise<_Type>::__type; | ||
| using __proxy_tag = typename __atomic_cuda_deduce_bitwise<_Type>::__tag; | ||
| using __proxy_t = typename __cuda_atomic_deduce_bitwise<_Type>::__type; |
There was a problem hiding this comment.
Nitpick: The compiler greatly prefers type aliases. We should introduce __cuda_atomic_deduce_bitwise_t
| template <class _Order> | ||
| struct __cuda_atomic_initial_load_order | ||
| { | ||
| using type = __cuda_atomic_order_relaxed; | ||
|
|
||
| [[nodiscard]] _CCCL_HOST_DEVICE_API static constexpr type __make() | ||
| { | ||
| return {}; | ||
| } | ||
| }; | ||
|
|
||
| template <> | ||
| struct __cuda_atomic_initial_load_order<__cuda_atomic_order_volatile> | ||
| { | ||
| using type = __cuda_atomic_order_volatile; | ||
|
|
||
| [[nodiscard]] _CCCL_HOST_DEVICE_API static constexpr type __make() | ||
| { | ||
| return {}; | ||
| } | ||
| }; | ||
|
|
||
| template <> | ||
| struct __cuda_atomic_initial_load_order<memory_order> | ||
| { | ||
| using type = memory_order; | ||
|
|
||
| [[nodiscard]] _CCCL_HOST_DEVICE_API static constexpr type __make() | ||
| { | ||
| return memory_order_relaxed; | ||
| } | ||
| }; | ||
|
|
||
| template <> | ||
| struct __cuda_atomic_initial_load_order<__cuda_atomic_runtime_cas_order> | ||
| { | ||
| using type = memory_order; | ||
|
|
||
| [[nodiscard]] _CCCL_HOST_DEVICE_API static constexpr type __make() | ||
| { | ||
| return memory_order_relaxed; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Important: This should just be a consteval function that returns the appropriate enum value in a if constexpr chain
That would be much cheaper to compile
| template <class _Type> | ||
| using __cuda_atomic_deduce_bitwise = | ||
| __type_switch<sizeof(_Type), | ||
| __type_case<1, __cuda_atomic_operand_deduction<uint8_t, __cuda_atomic_operand_b8>>, | ||
| __type_case<2, __cuda_atomic_operand_deduction<uint16_t, __cuda_atomic_operand_b16>>, | ||
| __type_case<4, __cuda_atomic_operand_deduction<uint32_t, __cuda_atomic_operand_b32>>, | ||
| __type_case<8, __cuda_atomic_operand_deduction<uint64_t, __cuda_atomic_operand_b64>>, | ||
| __type_default<__cuda_atomic_operand_deduction<__cuda_atomic_longlong2, __cuda_atomic_operand_b128>>>; | ||
|
|
||
| template <class _Type> | ||
| using __cuda_atomic_deduce_arithmetic = _If< | ||
| is_floating_point_v<_Type>, | ||
| _If<sizeof(_Type) == 4, | ||
| __cuda_atomic_operand_deduction<float, __cuda_atomic_operand_f32>, | ||
| __cuda_atomic_operand_deduction<double, __cuda_atomic_operand_f64>>, | ||
| _If<is_signed_v<_Type>, | ||
| __type_switch<sizeof(_Type), | ||
| __type_case<1, __cuda_atomic_operand_deduction<int8_t, __cuda_atomic_operand_s8>>, | ||
| __type_case<2, __cuda_atomic_operand_deduction<int16_t, __cuda_atomic_operand_s16>>, | ||
| __type_case<4, __cuda_atomic_operand_deduction<int32_t, __cuda_atomic_operand_s32>>, | ||
| __type_default<__cuda_atomic_operand_deduction<int64_t, __cuda_atomic_operand_u64>>>, // There is no | ||
| // atom.add.s64 | ||
| __type_switch<sizeof(_Type), | ||
| __type_case<1, __cuda_atomic_operand_deduction<uint8_t, __cuda_atomic_operand_u8>>, | ||
| __type_case<2, __cuda_atomic_operand_deduction<uint16_t, __cuda_atomic_operand_u16>>, | ||
| __type_case<4, __cuda_atomic_operand_deduction<uint32_t, __cuda_atomic_operand_u32>>, | ||
| __type_default<__cuda_atomic_operand_deduction<uint64_t, __cuda_atomic_operand_u64>>>>>; | ||
|
|
||
| template <class _Type> | ||
| using __cuda_atomic_deduce_minmax = _If< | ||
| is_floating_point_v<_Type>, | ||
| _If<sizeof(_Type) == 4, | ||
| __cuda_atomic_operand_deduction<float, __cuda_atomic_operand_f32>, | ||
| __cuda_atomic_operand_deduction<double, __cuda_atomic_operand_f64>>, | ||
| _If<is_signed_v<_Type>, | ||
| __type_switch<sizeof(_Type), | ||
| __type_case<1, __cuda_atomic_operand_deduction<int8_t, __cuda_atomic_operand_s8>>, | ||
| __type_case<2, __cuda_atomic_operand_deduction<int16_t, __cuda_atomic_operand_s16>>, | ||
| __type_case<4, __cuda_atomic_operand_deduction<int32_t, __cuda_atomic_operand_s32>>, | ||
| __type_default<__cuda_atomic_operand_deduction<int64_t, __cuda_atomic_operand_s64>>>, // atom.min|max.s64 | ||
| // supported | ||
| __type_switch<sizeof(_Type), | ||
| __type_case<1, __cuda_atomic_operand_deduction<uint8_t, __cuda_atomic_operand_u8>>, | ||
| __type_case<2, __cuda_atomic_operand_deduction<uint16_t, __cuda_atomic_operand_u16>>, | ||
| __type_case<4, __cuda_atomic_operand_deduction<uint32_t, __cuda_atomic_operand_u32>>, | ||
| __type_default<__cuda_atomic_operand_deduction<uint64_t, __cuda_atomic_operand_u64>>>>>; | ||
|
|
||
| _CCCL_END_NAMESPACE_CUDA_STD |
There was a problem hiding this comment.
Nitpick: I believe all of those are more efficient and more readable if turned into consteval functions with if constexpr chains
|
@miscco I can pull out the renaming into its own commit in the series; that should be fairly reviewable, and not having to track multiple dependent PRs would make my life easier. But if you'd really prefer, I can extract it to a separate PR. |
2 participants
Description
This PR massively refactors the internals of the implementation of
cuda::{,std::}atomic{,_ref}. Main points are as follows:__cuda_atomic_*, not the prior ensemble of historical debt.This is a large PR; I have attempted to split all the changes into smaller commits that should be at least somewhat reviewable. Reviewing this commit by commit is recommended.
Do not merge prior to closing of #10506.
Checklist