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 |
This comment has been minimized.
This comment has been minimized.
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
|
@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. |
|
/ok to test 451b96a |
|
/ok to test 5aead21 |
This comment has been minimized.
This comment has been minimized.
|
/ok to test d01603d |
|
The renaming aspect of this has now been extracted into #10922. |
d01603d to
18acbb3
Compare
|
/ok to test 18acbb3 |
This comment has been minimized.
This comment has been minimized.
|
/ok to test 7b27341 |
This comment has been minimized.
This comment has been minimized.
7b27341 to
8ca244c
Compare
|
/ok to test 8ca244c |
This comment has been minimized.
This comment has been minimized.
|
I have 2 comments regarding improvements we should make when we are touching this:
|
|
/ok to test 1e6afb5 |
|
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
WalkthroughChangesThe PR replaces separate CUDA and host atomic implementations with shared backend dispatch. It adds PTX order handling, generic CAS-based fallbacks, typed local-memory operations, updated generated intrinsics, and ordered SASS validation. Unified atomic backend
Suggested reviewers: Merge Risk: 🟠 High · up to This refactor changes atomic backend dispatching, but generated headers may currently be emitted with unbalanced compilation guards, which can prevent consumers from compiling; related operand handling and validation issues could also cause incorrect results or reject valid code generation. These concrete correctness and build risks should be fixed 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libcudacxx/codegen/generators/header.h (1)
75-88: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick wincritical: The emitted
#if _CCCL_CUDA_COMPILATION()guards now form a chain across generators, andFormatTailno longer emits the closing#endif. If the chain does not close, the generated header does not compile.
libcudacxx/codegen/generators/header.h#L75-L88: re-add the closing#endif // _CCCL_CUDA_COMPILATION()inFormatTail, or confirm that the last generator closes the final guard.libcudacxx/codegen/generators/fetch_ops.h#L134-L142: confirm that the per-operation#ifemitted here is matched by the#endifat the start offetch_bind_invokefor every operation, including the last one.
🧹 Nitpick comments (2)
libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx.h (1)
24-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Add the missing direct includes. This file uses
is_same_v(lines 42-47, 70-72) andNV_DISPATCH_TARGET(line 49), but it includes neither<cuda/std/__type_traits/is_same.h>nor<nv/target>. Both symbols arrive only through transitive includes today.`#include` <cuda/std/__type_traits/enable_if.h> `#include` <cuda/std/__type_traits/is_integral.h> +#include <cuda/std/__type_traits/is_same.h> `#include` <cuda/std/__type_traits/is_void.h> `#include` <cuda/std/__type_traits/make_unsigned.h> + +#include <nv/target>As per coding guidelines: "Include all headers needed by the symbols being used; do not rely on transitive includes."
Source: Coding guidelines
libcudacxx/codegen/generators/ld_st.h (1)
259-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: The generated
__cuda_atomic_storetakes the value as a non-const lvalue reference (__unv<_Type>& __val). The value is an input, and the binder at line 398 holds it by value, so the reference only works becauseoperator()is not const-qualified. Take the value by value, as the exchange and fetch intrinsics do, so a const binder or a temporary also binds.Also applies to: 278-279, 283-283
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 06587c78-beb3-461c-9d6e-8ddb6008182d
📒 Files selected for processing (32)
libcudacxx/codegen/generators/compare_and_swap.hlibcudacxx/codegen/generators/definitions.hlibcudacxx/codegen/generators/exchange.hlibcudacxx/codegen/generators/fence.hlibcudacxx/codegen/generators/fetch_ops.hlibcudacxx/codegen/generators/header.hlibcudacxx/codegen/generators/ld_st.hlibcudacxx/include/cuda/__atomic/atomic.hlibcudacxx/include/cuda/std/__atomic/functions.hlibcudacxx/include/cuda/std/__atomic/functions/backend.hlibcudacxx/include/cuda/std/__atomic/functions/common.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_local.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_backend.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated_helper.hlibcudacxx/include/cuda/std/__atomic/functions/dispatch.hlibcudacxx/include/cuda/std/__atomic/functions/generic.hlibcudacxx/include/cuda/std/__atomic/functions/generic_rmw.hlibcudacxx/include/cuda/std/__atomic/functions/host.hlibcudacxx/include/cuda/std/__atomic/functions/host_backend.hlibcudacxx/include/cuda/std/__atomic/scopes.hlibcudacxx/include/cuda/std/__atomic/types/base.hlibcudacxx/include/cuda/std/__atomic/types/common.hlibcudacxx/include/cuda/std/__atomic/types/small.hlibcudacxx/test/atomic_codegen/sass/bitwise_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/bitwise_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/compare_exchange_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/exchange_volatile_types_8_16_atomic_ref.cu
💤 Files with no reviewable changes (2)
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.h
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated_helper.h
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
This comment has been minimized.
This comment has been minimized.
| _CCCL_BEGIN_NAMESPACE_CUDA_STD | ||
|
|
||
| template <typename _Tp> | ||
| using __unv = remove_volatile_t<_Tp>; |
There was a problem hiding this comment.
__unv is... a very cryptic name, and this isn't F77 anymore. IMO, we should just keep this as remove_volatile_t, we don't save much on readability by shortening this.
There was a problem hiding this comment.
There is A LOT of remove_volatile_t throughout the backend. Yes, the name is cryptic at first glance, but having stared at the code with both the full spelling and with this alias in place, it (1) makes already really long lines sometimes 24 characters shorter, and (2) at the very least to my eyes it presents in such a way that I immediately scan the argument to it and peg it as the important part of the parameter definition, which I fail to do with the full spelling.
I get that this seems unnecessary, but I suggest you look at some of the denser pieces of the code here as it is in this PR, and then separately after doing s/__unv/remove_volatile_t/g to see the difference. At least to me it is massive. I wouldn't normally do this, but I had actual trouble following what was going on in the code, which prompted me to shorten the name down to this.
|
@Jacobfaib addressed most of your review comments. |
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: d5ebbfcf-3758-43b3-a109-7873baf8cb12
📒 Files selected for processing (17)
libcudacxx/codegen/generators/compare_and_swap.hlibcudacxx/codegen/generators/exchange.hlibcudacxx/codegen/generators/fence.hlibcudacxx/codegen/generators/fetch_ops.hlibcudacxx/codegen/generators/ld_st.hlibcudacxx/include/cuda/std/__atomic/functions/backend.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_local.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_backend.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.hlibcudacxx/include/cuda/std/__atomic/functions/dispatch.hlibcudacxx/include/cuda/std/__atomic/functions/generic.hlibcudacxx/include/cuda/std/__atomic/functions/generic_rmw.hlibcudacxx/test/atomic_codegen/sass/arithmetic_minmax_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/arithmetic_minmax_volatile_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/arithmetic_types_8_16_atomic_ref.culibcudacxx/test/atomic_codegen/sass/arithmetic_volatile_types_8_16_atomic_ref.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_backend.h
- libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx.h
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
|
@coderabbitai update your merge risk assessment. Your earlier "unterminated #ifs" comment is incorrect. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Candidate files ==='
fd -a '^(header|dispatch|backend)\.h$' libcudacxx 2>/dev/null | sed -n '1,160p'
echo '=== Generator header conditional directives ==='
if [ -f libcudacxx/codegen/generators/header.h ]; then
nl -ba libcudacxx/codegen/generators/header.h | grep -E '^[[:space:]]*[0-9]+[[:space:]]+.*#[[:space:]]*(if|ifdef|ifndef|elif|else|endif)\b' || true
fi
echo '=== Atomic function header conditional directives ==='
fd -a -e h . libcudacxx/include/cuda/std/__atomic/functions -x sh -c '
for f do
if grep -qE "^[[:space:]]*#[[:space:]]*(if|ifdef|ifndef|elif|else|endif)\b" "$f"; then
echo "--- $f"
grep -nE "^[[:space:]]*#[[:space:]]*(if|ifdef|ifndef|elif|else|endif)\b" "$f"
fi
done
' sh {} \;
echo '=== Relevant generated-header include/guard structure ==='
fd -a 'atomic.*\\.h|.*generated.*\\.h' libcudacxx/include 2>/dev/null | sed -n '1,120p'
echo '=== PR and blocking issue state ==='
gh pr view 10908 --repo NVIDIA/cccl --json number,state,isDraft,mergeStateStatus,reviewDecision,statusCheckRollup,headRefName,baseRefName,url
gh issue view 10506 --repo NVIDIA/cccl --json number,state,title,urlLength of output: 77032 suggestion: The earlier “unterminated The merge risk is now medium because this is a broad atomic dispatch refactor. I found no current preprocessor-structure blocker. The atomic PTX code-generation checks and Issue The current merge gates are the required review and the CI jobs that are still running or queued. You are interacting with an AI system. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libcudacxx/include/cuda/std/__atomic/functions/dispatch.h (1)
251-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Qualify the order-dispatch calls with
::cuda::std::.Line 252 calls
__cuda_atomic_load_order_dispatchunqualified. The same pattern repeats at lines 299, 349, 388, 439, 479, 519, 559, 599, and 639. Other free-function calls in this file are fully qualified, for example::cuda::std::__cuda_atomic_rmw_order_dispatchat line 132 and::cuda::std::__cuda_atomic_load_dispatchat line 261. Unqualified calls take arguments that are class types in this namespace, so ADL runs on every call.Proposed change
- __cuda_atomic_load_order_dispatch( + ::cuda::std::__cuda_atomic_load_order_dispatch( __backend, __bound_load, __order, __scope, __proxy_tag{}, __cuda_atomic_mmio_disable{});As per coding guidelines: "In headers, apply the required global qualification rules for free-function calls, including
::cuda::...qualification where mandated by the subproject."Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7303ff20-326f-4af2-aa55-d0b5326c8727
📒 Files selected for processing (7)
libcudacxx/codegen/generators/compare_and_swap.hlibcudacxx/codegen/generators/exchange.hlibcudacxx/codegen/generators/fetch_ops.hlibcudacxx/codegen/generators/header.hlibcudacxx/codegen/generators/ld_st.hlibcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.hlibcudacxx/include/cuda/std/__atomic/functions/dispatch.h
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx.h (1)
140-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Mark
__cuda_atomic_signal_fenceasnoexcept._CCCL_DEVICE_APIdoes not providenoexcept, and the function body cannot throw.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1fb885f3-08ff-47d0-9ff3-c31016f827ee
📒 Files selected for processing (2)
libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx.hlibcudacxx/include/cuda/std/__atomic/functions/dispatch.h
💤 Files with no reviewable changes (1)
- libcudacxx/include/cuda/std/__atomic/functions/dispatch.h
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This comment has been minimized.
This comment has been minimized.
…d-refactor-review # Conflicts: # libcudacxx/codegen/generators/compare_and_swap.h # libcudacxx/codegen/generators/exchange.h # libcudacxx/codegen/generators/fence.h # libcudacxx/codegen/generators/fetch_ops.h # libcudacxx/codegen/generators/ld_st.h # libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_derived.h # libcudacxx/include/cuda/std/__atomic/functions/cuda_ptx_generated.h
This comment has been minimized.
This comment has been minimized.
miscco
left a comment
miscco
left a comment
There was a problem hiding this comment.
Overal the refactor looks fine.
I have some concerns about compile time traps with a lot of the templated types we are introducing here.
I would greatly help if we could split this up into smalller commits and post this as separate PRs
|
@miscco some of your comments were clearly on an old revision, please recheck the areas of concern on top of branch. Most specifically, the binders have had as much of the template parameters pushed down to the |
|
@miscco I believe your review is now addressed, between the new commit and comments on your findings. |
🥳 CI Workflow Results🟩 Finished in 2h 07m: Pass: 100%/195 | Total: 5d 11h | Max: 1h 50m | Hits: 44%/1235318See results here. |
4 participants
Description
This PR massively refactors the internals of the implementation of
cuda::{,std::}atomic{,_ref}. Main points are as follows: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.
Checklist