[libcu++] Rename max to highest in arguments framework#9246
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
OverviewThis change renames the upper bound terminology in the CUDA argument framework from "max" to "highest" to improve API symmetry and avoid Windows-specific macro interference. The term "max" is replaced consistently throughout the framework and its tests, while "lowest" remains unchanged as the lower bound. Key Changeslibcudacxx/include/cuda/__argument/argument.h (Primary changes)
libcudacxx/include/cuda/__argument/argument_bounds.h
Test Files Updated
All tests updated to reflect the renaming: CUB usages updated
Upper-bound references and compile-time decisions were switched from ImpactThis is a pure refactoring with no functional changes—the behavior of the arguments framework remains identical. The change improves API naming consistency ("lowest"/"highest" symmetry) and eliminates Windows macro name collision risks. important: WalkthroughThis PR renames the upper-bound API and internals from "max" to "highest" across libcudacxx argument-bounds, updates deduction guides and traits, replaces the ChangesArgument-bounds "max" to "highest" terminology refactor
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)libcudacxx/test/libcudacxx/cuda/argument/deferred_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/deferred_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found ... [truncated 2200 characters] ... ed from ClangFrontend__CTrans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38 libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found ... [truncated 1169 characters] ... al-isystem" "/usr/local/include" "-internal-isystem" libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found ... [truncated 2200 characters] ... from ClangFrontend__CTrans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38
Comment |
This comment has been minimized.
This comment has been minimized.
pciolkosz
force-pushed
the
rename_max_to_highest
branch
from
2ecd653 to
8ef2fe5
Compare
![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
🧹 Nitpick comments (2)
cub/cub/device/dispatch/dispatch_batched_topk.cuh (1)
92-92: ⚡ Quick winsuggestion: Qualify the touched free-function calls from the global namespace. The edited call sites still use
wrap_select_direction(...)/params::get_param(...)unqualified, which does not match the header rule for free-function calls.As per coding guidelines, "All calls to free functions must be fully qualified starting from the global namespace, e.g.,
::cuda::ceil_div."Also applies to: 151-151, 200-200, 256-256, 300-300
cub/cub/agent/agent_batched_topk.cuh (1)
219-220: ⚡ Quick winsuggestion: Cache
params::get_param(k_param, segment_id)once before themin. This path currently evaluates the per-segmentkparameter twice, which is an extra device-side fetch in the hot small-segment path and makes the type expression harder to read.As per coding guidelines, "
cub/**/*: Focus on algorithm correctness, temporary-storage protocol, dispatch/policy selection, stream behavior, CUDA error handling, synchronization, memory access safety, performance regressions, and test coverage."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cf19ffe-5073-4bca-abea-55105e000cbf
📒 Files selected for processing (11)
cub/cub/agent/agent_batched_topk.cuhcub/cub/device/dispatch/dispatch_batched_topk.cuhcub/cub/device/dispatch/kernels/kernel_batched_topk.cuhlibcudacxx/include/cuda/__argument/argument.hlibcudacxx/include/cuda/__argument/argument_bounds.hlibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/deferred_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_bounds_conversion.fail.cpplibcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
✅ Files skipped from review due to trivial changes (2)
- libcudacxx/include/cuda/__argument/argument_bounds.h
- libcudacxx/test/libcudacxx/cuda/argument/deferred_argument.pass.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- libcudacxx/test/libcudacxx/cuda/argument/static_bounds_conversion.fail.cpp
- libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp
- libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
- libcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
- libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp
- libcudacxx/include/cuda/__argument/argument.h
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cub/cub/device/dispatch/dispatch_batched_topk.cuh (1)
92-92: ⚡ Quick winsuggestion: Qualify the touched free-function calls from the global namespace. The edited call sites still use
wrap_select_direction(...)/params::get_param(...)unqualified, which does not match the header rule for free-function calls.As per coding guidelines, "All calls to free functions must be fully qualified starting from the global namespace, e.g.,
::cuda::ceil_div."Also applies to: 151-151, 200-200, 256-256, 300-300
cub/cub/agent/agent_batched_topk.cuh (1)
219-220: ⚡ Quick winsuggestion: Cache
params::get_param(k_param, segment_id)once before themin. This path currently evaluates the per-segmentkparameter twice, which is an extra device-side fetch in the hot small-segment path and makes the type expression harder to read.As per coding guidelines, "
cub/**/*: Focus on algorithm correctness, temporary-storage protocol, dispatch/policy selection, stream behavior, CUDA error handling, synchronization, memory access safety, performance regressions, and test coverage."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cf19ffe-5073-4bca-abea-55105e000cbf
📒 Files selected for processing (11)
cub/cub/agent/agent_batched_topk.cuhcub/cub/device/dispatch/dispatch_batched_topk.cuhcub/cub/device/dispatch/kernels/kernel_batched_topk.cuhlibcudacxx/include/cuda/__argument/argument.hlibcudacxx/include/cuda/__argument/argument_bounds.hlibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/deferred_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_bounds_conversion.fail.cpplibcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
✅ Files skipped from review due to trivial changes (2)
- libcudacxx/include/cuda/__argument/argument_bounds.h
- libcudacxx/test/libcudacxx/cuda/argument/deferred_argument.pass.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- libcudacxx/test/libcudacxx/cuda/argument/static_bounds_conversion.fail.cpp
- libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp
- libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
- libcudacxx/test/libcudacxx/cuda/argument/usage_example.pass.cpp
- libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp
- libcudacxx/include/cuda/__argument/argument.h
🛑 Comments failed to post (1)
cub/cub/device/dispatch/dispatch_batched_topk.cuh (1)
242-243:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winsuggestion: Fix the static_assert text to mention
num_segments, not segment sizes. The predicate here is onNumSegmentsParameterT, so the current message points users at the wrong argument when this fails.
🥳 CI Workflow Results🟩 Finished in 2h 33m: Pass: 100%/340 | Total: 3d 19h | Max: 1h 09m | Hits: 93%/503670See results here. |
|
Not really a fan given that |
4 participants
Initially lowest and max were selected in the arguments framework to mirror numeric_limits, but max has some issues on Windows and I don't necessarily like the asymmetry. Since the arguments are mostly just created by the users and consumed by our APIs, I think we can deviate from numeric_limits and use lowest and highest instead, which is symmetrical and avoids windows issues