Repository navigation
Never copy CUDA tensors to the host implicitly - #122
Conversation
linalg::singular_values on a CUDA matrix used to copy the matrix to the host and run LAPACK, returning a host tensor. make_tensor materializes host storage regardless of the source domain, so the GPU call silently became a device-to-host transfer plus a CPU SVD. Add a cuSOLVER singular_values_op backend for real and complex float and double. It calls gesvd with jobu = jobvt = 'N' and null U/VT, which LAPACK and cuSOLVER leave unreferenced for job 'N', and allocates rwork rather than passing null. A matrix and its transpose share singular values, so the backend accepts any workspace whose provider shape is tall: tall column-major, or wide row-major read as its transpose. singular_values now keeps CUDA-buffer inputs on their device and returns a CudaTensor there. The preserving form copies to column-major work for tall input and row-major work for wide input on the device; the consuming form uses a compatible input as the workspace directly and otherwise makes the same device copy. Async forms route the same way. Builds without cuSOLVER keep the previous path. Select the operand device around the cuSOLVER provider calls. cuSOLVER launches into the calling thread's current device, so the existing full SVD failed with CUSOLVER_STATUS_EXECUTION_FAILED for operands on any device other than the current one. The new device-1 tests reproduce that failure before the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restricts implicit storage-domain transfers during tensor materialization and linear-algebra operations. Eager materialization via Tensor(view) and make_tensor(view) now preserves the source's storage domain (host or CUDA device), and host-only linear-algebra operations (such as SVD, QR, and LU) now reject CUDA operands at compile time rather than implicitly copying them. The review feedback suggests robustifying the newly introduced C++ concepts, SourceAllocationContext and HostReadableTensor, by normalizing their InputTensor template arguments with std::remove_cvref_t to correctly handle reference types.
| template <class Policy, class InputTensor> | ||
| concept SourceAllocationContext = requires { typename Policy::context_type; } && requires(InputTensor const& input) { | ||
| { mdspec_of(input).data_descriptor().buffer().resources() } -> std::same_as<typename Policy::context_type&>; | ||
| }; |
There was a problem hiding this comment.
When designing C++ concepts that query observers or nested types, it is important to normalize the template arguments using std::remove_cvref_t to ensure they correctly handle reference types (such as those obtained via decltype). In SourceAllocationContext, InputTensor should be normalized to std::remove_cvref_t<InputTensor> so that input is consistently treated as a const lvalue reference, preventing collapsing to a non-const reference when a reference type is passed.
template <class Policy, class InputTensor>
concept SourceAllocationContext = requires { typename Policy::context_type; } && requires(std::remove_cvref_t<InputTensor> const& input) {
{ mdspec_of(input).data_descriptor().buffer().resources() } -> std::same_as<typename Policy::context_type&>;
};References
- When designing C++ concepts or type traits that query nested types or observers, normalize the template arguments (e.g., using std::remove_cvref_t) to ensure they correctly handle reference types (such as those obtained via decltype).
There was a problem hiding this comment.
Applied in b785797. No current call site passes a reference type, so this is hardening rather than a fix.
| template <class InputTensor> | ||
| concept HostReadableTensor = | ||
| TensorView<InputTensor> && AccessDomainsCompatible<stdex::default_accessor<tensor_element_t<InputTensor> const>, | ||
| typename tensor_mdspec_t<InputTensor>::accessor_type>; |
There was a problem hiding this comment.
To ensure C++ concepts correctly handle reference types (such as those obtained via decltype), normalize the template arguments using std::remove_cvref_t. In HostReadableTensor, InputTensor should be normalized to std::remove_cvref_t<InputTensor> before querying its nested types and passing it to other traits.
| template <class InputTensor> | |
| concept HostReadableTensor = | |
| TensorView<InputTensor> && AccessDomainsCompatible<stdex::default_accessor<tensor_element_t<InputTensor> const>, | |
| typename tensor_mdspec_t<InputTensor>::accessor_type>; | |
| template <class InputTensor> | |
| concept HostReadableTensor = | |
| TensorView<std::remove_cvref_t<InputTensor>> && AccessDomainsCompatible<stdex::default_accessor<tensor_element_t<std::remove_cvref_t<InputTensor>> const>, | |
| typename tensor_mdspec_t<std::remove_cvref_t<InputTensor>>::accessor_type>; |
References
- When designing C++ concepts or type traits that query nested types or observers, normalize the template arguments (e.g., using std::remove_cvref_t) to ensure they correctly handle reference types (such as those obtained via decltype).
There was a problem hiding this comment.
Applied in b785797. No current call site passes a reference type, so this is hardening rather than a fix.
|
Review Bot: review of exact head Found 1 issue: a documentation sentence (inline comment on What the PR does. Materializing a tensor now stays in the source's storage domain. CTAD, What I reviewed.
Verified (polaron, GCC 13.3 Debug, CUDA 12.9,
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
|
|
||
| An explicit-output form may be useful for allocation reuse and asynchronous | ||
| execution: | ||
| The explicit-output form reuses an allocation and accepts strided views: |
There was a problem hiding this comment.
Review Bot: review of exact head b0a4c65ecdaf1b1dd2d4e0357c770ce80330f4d7
This new sentence says copy(destination, source) "accepts strided views", but a host↔device copy refuses any pair that is not contiguous with the same physical order. Now that copy is one of the three allowed transfers, readers will reach for it to transfer a strided or opposite-layout CUDA view.
- Code:
src/uni20/linalg/backends/cuda/copy.hpp:214-224setsruntime_copy_eligibleonly when both mappings are unique and exhaustive andphysical_orders_match(output, input). If not, only device-to-device falls back to the elementwise kernel. Host↔device returnsKernelAttempt::unsupported_layout. - Reproduced (CUDA 12.9, GCC 13.3 Debug,
-DUNI20_ENABLE_CUDA=ON -DUNI20_BACKEND_CUSOLVER=ON, Quadro GV100). The source is a column-majorCudaTensor<double, 2>(2×3) and the destination a preallocatedTensor<double, 2, HostStorage, RowMajor>, so both are contiguous and only the order differs. The reverse direction (column-major host source into a row-majorCudaTensor) behaves the same:The next paragraph ("D2H colmajor->rowmajor threw: kernel dispatch failed for operation 'copy': all runtime candidates declined H2D colmajor->rowmajor threw: kernel dispatch failed for operation 'copy': all runtime candidates declinedto_hostandto_deviceare implemented for canonical contiguous Tensor layouts. Fixed-outputcopyuses the same transfer backend") already implies this limit, so the two paragraphs contradict each other.
Suggested fix: drop "and accepts strided views". Or say that host↔device copy needs contiguous operands with matching physical order, and that a strided CUDA view goes through make_tensor (same device) and then to_host.
Not checked. I did not test a padded layout_stride mapping that is otherwise in matching order (the code reading above covers it). I did not check whether a staging fallback is planned.
There was a problem hiding this comment.
Agreed, thanks. Fixed in b785797: the paragraph now says host/device copy requires contiguous operands with the same physical order, that only CUDA-to-CUDA copies accept strided or reordered views, and that a strided CUDA view goes through make_tensor on its device and then to_host.
There was a problem hiding this comment.
Review Bot: recheck of exact head b7857971fc6051f9296e64e1e245541082de930c
Confirmed fixed. The new paragraph in docs/tensor/scalar_tensors_and_transfer.md (lines 250–261) matches src/uni20/linalg/backends/cuda/copy.hpp:214-226. Host↔device copies need both mappings contiguous with matching physical order; only device-to-device copies fall back to the elementwise kernel.
I also ran the route the doc now recommends. Setup: CUDA 12.9, GCC 13.3 Debug, -DUNI20_ENABLE_CUDA=ON -DUNI20_BACKEND_CUSOLVER=ON, 2× Quadro GV100. The source is a layout_stride 2×3 CUDA view (MdspecTensorView over a CudaTensor<double, 1> on device 1, buffer offset 1, strides {1,3} and {3,1}).
make_tensor(view)returned aCudaStoragetensor on device 1.to_hostof that tensor gave the expected values in both cases.- A direct
copy(host_tensor, view)was declined with "cuda_reference … unsupported layout", as the paragraph now says. Note that this aborted the process (exit 134) rather than throwing an exception I could catch. This behaviour is older than this PR, and the doc does not promise an exception.
Not checked. A padded but matching-order host↔device pair at runtime (I only read the code for that case). Clang and Release builds of the probe.
The consuming singular_values overloads required an immediate host tensor view. CudaMatrix has no host mdspan, so singular_values on an rvalue CUDA matrix, and the async consuming form, bound to the preserving overload and always copied the input. The consuming CUDA path was unreachable, contrary to the documentation. Admit CUDA-resident owning tensors in both consuming overloads. A tall column-major or wide row-major consumed matrix is now the gesvd workspace; other layouts are copied on the device as documented. The consuming test checked values only, so it passed either way. It now also checks whether the caller's tensor was overwritten: gesvd with jobu = jobvt = 'N' destroys its matrix, so the tensor changes exactly when it was used in place. Without this fix the test fails for all four scalar types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owning-tensor materialization ignored the source's storage domain. make_tensor(view) and Tensor CTAD always produced HostStorage, and the Tensor converting constructor accepted any readable view, so a CUDA source could become a host tensor without an explicit transfer. The linear-algebra value operations materialize their workspaces through make_tensor, so svd, eigh, qr, lq, solve and LU on a CudaTensor silently copied it to the host and ran LAPACK. Materialization now stays in the source's storage domain. Accessors already declare where they may be evaluated (host_access_domain, cuda_access_domain); a source may be materialized into storage whose accessor shares one of its declared domains. Accessors that declare no domain keep the previous behaviour. - The converting constructor rejects cross-domain sources, so host and CUDA tensors no longer convert into each other. A CUDA result is allocated on the source buffer's device, so construction never implies a peer transfer either. - make_tensor(view) and CTAD from a view deduce CUDA storage for CUDA sources. make_tensor of an owning CudaTensor makes a same-device copy; owning CUDA tensors remain non-copyable, so CTAD from one still does not compile. Bare-mdspan make_tensor requires a host-readable mdspan. - Async make_tensor follows the same rule; async to_host no longer delegates to make_tensor. - Host-only linear-algebra value operations build their workspace through make_host_work_tensor, which static_asserts that the operand is host-readable and points the caller at to_host. to_host, to_device and copy(destination, source) are the only transfers between host and CUDA storage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Host/device copy(destination, source) requires contiguous operands with the same physical order; only CUDA-to-CUDA copies accept strided or reordered views. Document that, and the make_tensor-then-to_host route for a strided CUDA view, instead of claiming that copy accepts strided views. Normalize SourceAllocationContext and HostReadableTensor with remove_cvref_t so reference template arguments are classified like their referred-to tensor types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b0a4c65 to
b785797
Compare
|
Review Bot: review of exact head No defect found. The earlier documentation finding (#122 (comment)) is fixed; the recheck is in that thread. What changed since
What I reviewed.
Verified (polaron, GCC 13.3 Debug, CUDA 12.9,
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
Stacked on #121; review that first. The base branch will be retargeted to
mainonce #121 merges.Owning-tensor materialization ignored the source's storage domain.
make_tensor(view)andTensorCTAD always producedHostStorage, and theTensor(TensorView const&)converting constructor accepted any readable view, so a CUDA source could become a host tensor without an explicit transfer. The linear-algebra value operations build their workspaces throughmake_tensor, sosvd,svd_left,svd_right,truncated_svd,eigh,qr,lq,solveand LU on aCudaTensorsilently copied it to the host and ran LAPACK.Rule.
to_host,to_deviceandcopy(destination, source)are the only transfers between host and CUDA storage. Materialization stays in the source's domain, using the existing accessor-domain model: a source may be materialized into storage whose accessor shares one of its declared domains (host_access_domain,cuda_access_domain). Generated accessors are valid in both. Accessors that declare no domain keep the previous behaviour.Changes
DenseMatrix<T>(cuda)andCudaMatrix<T>(host)are no longer constructible. A CUDA result is allocated on the source buffer's device (previously the default device), so construction never implies a peer transfer either.make_tensor(view)and CTAD from a view deduce CUDA storage for CUDA sources.make_tensor(cuda_tensor)makes a same-device copy. Owning CUDA tensors stay non-copyable, soTensor t(cuda_tensor)(CTAD from an owning tensor picks the copy constructor) still does not compile. Bare-mdspanmake_tensorrequires a host-readable mdspan.make_tensorfollows the same rule and allocates on the source device. Asyncto_hostno longer delegates tomake_tensor.detail::make_host_work_tensor, whosestatic_assertreads: "this linear-algebra operation has no implementation for CUDA operands; transfer the operand explicitly with uni20::to_host() to compute on the host".singular_valueskeeps its cuSOLVER path from Add device-resident cuSOLVER singular values #121; without cuSOLVER it now gets the samestatic_assert.least_squaresnever materialized and already failed to compile for CUDA operands, so it is unchanged.Audit. After the restriction, a full
-k 0build of every target (library, tests, examples) found exactly one user of the implicit routes:test_cuda_copy.cppasserted that asyncsvd_left/svd_right/svd/eigh/truncated_svdwere well-formed forAsync<CudaMatrix>. Those assertions are replaced with checks of the new rules. Nothing else in the tree relied on them.Tests. New static assertions: cross-domain construction is not constructible;
make_tensor, CTAD and asyncmake_tensorof CUDA sources give CUDA types; asyncto_hostgives a host type;singular_valuesonAsync<CudaMatrix>givesAsync<CudaTensor<double, 1>>. New runtime tests: materialization of an owning tensor, a conjugated view and CTAD stays on device 1 with correct values, and asyncmake_tensorstays on the device while asyncto_hosttransfers. Thestatic_assertdiagnostic was checked by compilingsvd_left,eigh,qr,solveandlu_factoron aCudaMatrix; the repo has no compile-failure test harness, so that check is not automated.Validation:
ctest2545/2545.ctest2405/2407. Failures:uni20_common_tests_NOT_BUILT(that target does not build in CUDA configurations onmaineither: missingcuda_runtime_api.hinclude path), andCudaTensorTest.StreamOrderedDescriptorAccessCanBeCoAwaitedAsMdspanLease.ctest2373/2375. Same NOT_BUILT target, andCudaTensorTest.StreamOrderedAccessCanBeCoAwaitedAsTensorView.CudaTensorTest.StreamOrdered*failures are pre-existing flakiness: a pristinemainbuild ofuni20_cuda_runtime_testsfails the same way (test_cuda_tensor.cpp:588,submittedfalse, then an active-task destruction CHECK) at roughly iteration 149 of a 200-repetition run.🤖 Generated with Claude Code