[STF] cuda_safe_call<fun>(args...): introspected call forms shared with cuda_try - #11215
[STF] cuda_safe_call<fun>(args...): introspected call forms shared with cuda_try#11215andralex wants to merge 8 commits into
Conversation
…ith cuda_try The compile-time introspection behind cuda_try<fun> (direct / first-output / last-output forms, ambiguity rejection) is independent of what happens to the status, so it moves into a shared engine, reserved::checked_api_call, parameterized on the checker. cuda_try<fun> becomes a thin front and cuda_safe_call<fun> joins it as the aborting sibling, deliberately parallel so migrating a call site between the aborting and throwing regimes is a one-token change: cuda_safe_call<f>(a) <-> cuda_try<f>(a). Same limitations as before, now documented in one place: overloaded names cannot be template arguments, and the checker's source_location fires in this header rather than at the user's call site (pack followed by defaulted parameter is non-deduced in C++17; revisit with C++20). Unit tests extended with the cuda_safe_call siblings, run at C++17 and C++20. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/ok to test f2153aa |
|
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:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change updates CUDA call ambiguity detection and first-output constraints. Directly invocable calls are accepted, while first-output forms require writable pointer outputs. ChangesChecked CUDA calls
Merge Risk: 🟡 Moderate · up to Parameterless CUDA APIs may fail to compile through cuda_try or cuda_safe_call because output-parameter traits are instantiated for calls with no parameters. This should be resolved before merge. 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
🧹 Nitpick comments (3)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh (3)
456-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Mark
reserved::checked_api_call,cuda_try<fun>, andcuda_safe_call<fun>with_CCCL_HOST_API. Each function uses host-only error handling. The project rule requires an API annotation on all functions. As per coding guidelines, “Functions must be marked with_CCCL_HOST_API,_CCCL_DEVICE_API,_CCCL_HOST_DEVICE_API,_CCCL_TILE_API, or_CCCL_API.”Also applies to: 502-502, 521-521
Source: Coding guidelines
505-505: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Declare both lambda
statusparameters asconst auto. Neither lambda modifies the status value. As per coding guidelines, “All variables that are not modified must be declaredconst.”Also applies to: 524-524
Source: Coding guidelines
499-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Use
_CCCL_DOXYGEN_INVOKEDin this#endifcomment. The comment currently uses a negated spelling instead of the condition text from line 446. Based on learnings, annotated#endifcomments must repeat the exact condition text from the corresponding#if.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61229d17-25c9-4687-bb6d-6d89281f66f4
📒 Files selected for processing (1)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
…location The introspected forms previously reported errors at this header's line: with a trailing parameter pack, a defaulted source_location parameter is a non-deduced context, so no variadic front can capture the call site. The fronts are now a bounded-arity overload set (arities 0..8, stamped out mechanically), each carrying a trailing defaulted source_location::current() that fires where the user wrote the call; the shared engine threads it through to the status checker. Call syntax is unchanged. No CUDA entry point is known to take more than eight direct parameters. Verified by probe: a failing introspected call now reports the caller's file, line, and function rather than cuda_safe_call.cuh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh (1)
462-470: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winimportant: Parameterless entry points still fail to compile.
first_output_formandlast_output_forminstantiatereserved::first_param<fun>andreserved::last_param<fun>eagerly. Both operands of&&must be well-formed, sodirect_formdoes not shield them. For a zero-parameter function such ascudaDeviceSynchronizeorcudaGetLastError,first_param_impl<cudaError_t (*)()>andlast_param_impl<>are declared but not defined, so the header hard-errors on an incomplete type.Add the nullary specializations, then add coverage for a parameterless function and for the
<fun>direct form in bothUNITTEST("cuda_try2")andUNITTEST("cuda_safe_call2").Trait specializations
template <typename R, typename P, typename... Ps> struct first_param_impl<R (*)(P, Ps...)> { using type = P; }; + +// A parameterless function has no first parameter; report `void` so the +// pointer/const checks in `checked_api_call` evaluate to `false`. +template <typename R> +struct first_param_impl<R (*)()> +{ + using type = void; +}; template <typename...> struct last_param_impl; + +// Same rationale for a parameterless function's last parameter. +template <> +struct last_param_impl<> +{ + using type = void; +};#!/bin/bash set -euo pipefail f=$(fd -t f 'cuda_safe_call.cuh' cudax) echo "file: $f" # Do nullary specializations exist on the PR head? ast-grep run --lang cpp --pattern 'struct first_param_impl<$$$> { $$$ }' "$f" ast-grep run --lang cpp --pattern 'struct last_param_impl<$$$> { $$$ }' "$f" # Any existing caller that would instantiate the broken path today. rg -nP 'cuda_(try|safe_call)<\s*(cudaDeviceSynchronize|cudaGetLastError|cudaPeekAtLastError|cudaThreadExit|cuInit)\s*>' \ --glob '*.{h,hpp,cuh,cu,cpp,cxx}' .
🧹 Nitpick comments (1)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh (1)
508-527: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Generate the arity fronts with a macro.
Arities 0..8 are repeated twice, once per API, for about 320 lines. The bodies differ only in the checker call. A local macro plus
#undefkeeps the two overload sets in sync and makes an arity bump a one-line change.Sketch
+#define _CCCL_STF_CHECKED_FRONT(name) \ + template <auto fun> \ + auto name(const ::cuda::std::source_location loc = ::cuda::std::source_location::current()) \ + { \ + return reserved::checked_api_call<fun>( \ + [](auto status, auto l) { \ + name(status, l); \ + }, \ + loc); \ + } \ + /* ... one block per arity, or a nested arity macro ... */ + +_CCCL_STF_CHECKED_FRONT(cuda_try) +_CCCL_STF_CHECKED_FRONT(cuda_safe_call) +#undef _CCCL_STF_CHECKED_FRONT
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df5a81aa-8d28-44c8-9be5-554b321862c7
📒 Files selected for processing (1)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
|
Nice ! |
This comment has been minimized.
This comment has been minimized.
Supersedes the fixed-arity overload sets of the previous commit with the with_location design. The introspected forms reported errors at this header's line: with a trailing parameter pack, a defaulted source_location is a non-deduced context, so no variadic front can capture the call site. The first user argument (when there is one) is now taken as with_location<T>, where T is COMPUTED from fun's signature rather than deduced. The parameter type is therefore concrete, the raw argument converts to it, and with_location's constructor captures the location at the point of conversion: the user's call site. Call syntax is unchanged and the fronts stay variadic. Two argument-taking overloads per name: the user's first argument aligns with fun's first parameter for the direct and last-output forms, and with its second parameter for the first-output form, where parameter one is the synthesized output pointer. A third overload handles calls with no user arguments, where a plain defaulted source_location suffices (no pack to block it). forward<declared type> preserves the parameter's value category, so reference parameters pass through unchanged; the engine's form detection now uses a pointee trait that stays well-formed for reference parameters, where remove_pointer_t<T>* was ill-formed for T = X&. Tests: a regression unittest pinning the reported line to the caller's, a check that an lvalue-reference parameter is written through, and an external probe covering all four call shapes at C++17 and C++20. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/ok to test 84b5611 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh (2)
701-702: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: These lines use
::std::stringand::std::to_string, but the file does not include<string>. Add the include so the test does not depend on a transitive include.`#include` <cstdlib> `#include` <exception> +#include <string>As per coding guidelines: "Include all headers needed by the symbols being used; do not rely on transitive includes."
Source: Coding guidelines
250-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Remove the unused defaulted template parameter from
first_param_impl. Its specializations and uses provide only the function-type argument, and no code in this header uses the second parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d968075f-e80d-4326-be43-02d6e4dc0c88
📒 Files selected for processing (1)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuh
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.
…_location Three fixes on top of the location-capture design: Ambiguity rejection is back to a curated static_assert. With the front-based design, a function whose first and last parameters are both non-const output pointers (cudaMemGetInfo) made the two argument-taking fronts ambiguous, so the diagnostic became an overload race and the cuda_try_ambiguous XFAIL test, which greps for 'static assertion', failed. A reserved::ambiguous_output_forms trait now excludes those cases from every front and selects a catch-all whose body carries the original message. The doc block above the introspected forms still declared @PARAM ps from the previous variadic front and now sat above the zero-argument overload, which the documentation build rejects. It describes the overload set instead, without per-parameter tags. with_location's copy and move assignment are deleted: when T is a reference, assigning would write through the referent rather than rebind it, silently modifying the caller's object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/ok to test 25f089c |
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: b0e03874-aaf4-45c6-9983-fc6c3fd088d0
📒 Files selected for processing (2)
cudax/include/cuda/experimental/__stf/utility/cuda_safe_call.cuhcudax/include/cuda/experimental/__stf/utility/source_location.cuh
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
…onst pointees Three defects in the location-capturing fronts, all found by the CI build failures and review: The ambiguity trait ignored the direct form and was evaluated without the argument the front had already consumed. CUDA handle types are themselves pointers, so cudaFreeAsync(void*, cudaStream_t) looked like it had output pointers at both ends and every call was rejected as ambiguous; this broke 37 build jobs. The trait now takes the arguments as the engine will see them and tests direct invocability first: if fun(args...) is a valid call, nothing is synthesized and nothing can be ambiguous. The first-output fronts did not carry the engine's guards on the synthesized parameter. For f(const int*, int), cuda_try<f>(1) created a const int, passed its address as an input pointer, and returned the never-written default. They now require a non-void, non-const pointee, matching first_output_form exactly. Tests: a call-shapes unittest covering derived-pointer-to-void conversions, handle out-parameters, enum-keyed queries, nullary APIs and the direct forms, plus documented negative shapes. Fifteen call shapes taken from real in-tree call sites compile and run at C++17 and C++20. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/ok to test 5e25e29 |
CTK 12.0's front end hits Internal Compiler Error (codegen): "unexpected expression with aggregate type!" on the location-capturing fronts once the full STF header set is in the translation unit. Every isolated reproduction compiles cleanly on 12.0 (the wrapper by value, the parameter pack, the _CCCL_TEMPLATE constraints, the engine with its checker lambda, pointer conversions, all of them together), so the trigger needs more of the stack than can be reduced here; 12.1 and every later toolkit through 13.3 are unaffected. On nvcc 12.0 only, the introspected forms use the plain variadic fronts instead: identical call syntax and semantics, with a failing call reporting this header's line rather than the caller's, which is the behavior that shipped before this PR. Host compilers and every other toolkit keep the caller's location. The location-capture unittest is gated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/ok to test 6b68580 |
🥳 CI Workflow Results🟩 Finished in 1h 17m: Pass: 100%/63 | Total: 1d 07h | Max: 51m 35s | Hits: 13%/192585See results here. |
2 participants
cuda_try<fun>(args...)supports three introspected call shapes (direct, first-output-parameter, last-output-parameter) selected at compile time. That machinery is independent of what happens to the failing status, so this PR extracts it into a shared engine (reserved::checked_api_call, parameterized on the checker) and givescuda_safe_callthe same spelling:The two fronts are deliberately parallel so that migrating a call site between the aborting and throwing regimes is a one-token change:
cuda_safe_call<f>(a)<->cuda_try<f>(a). This is groundwork for the planned incrementalcuda_safe_call->cuda_trymigration.Limitations are unchanged and now documented in one place: overloaded names (
cudaMallocand the other templatedcuda_runtime.hwrappers) cannot beautotemplate arguments, and the checker's defaultedsource_locationfires in the header rather than at the user's call site (a parameter pack followed by a defaulted parameter is non-deduced in C++17; noted for a C++20 revisit).Unit tests extended with
cuda_safe_callsiblings of thecuda_trycases; run at C++17 and C++20.🤖 Generated with Claude Code