Repository navigation
Add device-resident cuSOLVER singular values - #121
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 implements device-resident real and complex singular values using the cuSOLVER backend (cusolverDn{S,D,C,Z}gesvd with jobu = jobvt = 'N'). It introduces support for keeping the computation on the CUDA device for both preserving and consuming forms of singular_values when the cuSOLVER backend is enabled, and ensures that cuSOLVER calls are executed on the correct operand device using scoped device guards. Additionally, comprehensive unit tests and documentation updates have been added to cover these new capabilities. I have no feedback to provide as there are no review comments to address and the implementation is clean and well-tested.
|
Review Bot: review of exact head No defect found. The compute host has no GPU, so none of the new device code was executed. Everything below comes from builds, compile-time probes and reading the code. What the PR does. It adds a cuSOLVER What I reviewed.
Verified (polaron, GCC 13.3, CUDA 12.9 nvcc,
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
| auto const p = detail::decomposition_precision(matrix, requested); | ||
| return detail::singular_values_from_consumed_matrix(std::forward<BackendSelector>(selector), | ||
| #if UNI20_BACKEND_CUSOLVER | ||
| if constexpr (detail::cuda_resident_singular_values<MatrixTensor>()) |
There was a problem hiding this comment.
Review Bot: review of exact head 0551fbe0753913ce93e981eba40f8bedd529da6b
This branch never runs for a CudaTensor, so the consuming form still copies. The overload is constrained on uni20::MutableRankedImmediateTensorView<MatrixTensor, 2> (line 762). ImmediateTensorView requires a host mdspan() (src/uni20/tensor/concepts.hpp:100), and CudaMatrix doesn't provide one. As a result, singular_values(std::move(cuda_matrix)) resolves to the preserving MatrixTensor const& overload and goes through cuda_singular_values_from_copy. The async consuming form behaves the same way: co_consuming_singular_values calls singular_values(selector, std::move(matrix_value), ...) (async/svd.hpp:159), which also binds to the preserving overload. cuda_singular_values_from_consumed (line 439) can't be reached from either public entry point.
The results are still correct. What doesn't hold is the stated behaviour: a tall column-major or wide row-major consumed input is copied into a fresh device allocation instead of being used as the workspace. That contradicts:
- the doc comment above this overload;
docs/backends/cuda/cusolver.md:254("The consuming form uses a tall column-major or wide row-major input as the workspace directly");- the comments at
tests/backend/cusolver/test_cusolver_singular_values.cpp:185and:192.
ConsumingCallUsesOrCopiesDeviceWorkspace checks only the values, so it passes either way.
How I saw it (polaron, 2x Quadro GV100, CUDA 12.9 nvcc, g++-13, Debug, -DUNI20_ENABLE_CUDA=ON -DUNI20_BACKEND_CUSOLVER=ON -DCMAKE_CUDA_ARCHITECTURES=70). I added two probe files to tests/backend/cusolver/ in a scratch worktree of this head:
- A runtime-printed concept check gives
OwningTensor<CudaMatrix<double>>=1,ImmediateTensorView=0,MutableRankedImmediateTensorView<.,2>=0, which makes the consuming overload not viable. The same check onTensor<double,2>gives 1 for all three. auto d = to_device(h, res); auto v = singular_values(std::move(d));followed byto_host(d)gives back exactlyhfor 8x5 column-major and 5x8 row-major inputs, for all four scalars.gesvdwithjobu = jobvt = 'N'overwrites its matrix argument, so an unchangeddmeans the solver ran on a copy.
Smallest fix I can see: give the CUDA case an overload that the constraint admits. For example, accept an owning MutableRankedTensorView<MatrixTensor, 2> when cuda_resident_singular_values<MatrixTensor>() holds. Then add an assertion that discriminates, such as the consumed buffer being modified or no allocation happening. Alternatively, if the copy is intended, correct the docs and comments to say so.
Not checked. Whether some other CudaStorage owning type in the tree satisfies ImmediateTensorView; I found none and probed only CudaMatrix in both layouts. Clang and CUDA 13 builds.
There was a problem hiding this comment.
Confirmed, thanks. Fixed in 20f85e8: both consuming overloads now also admit CUDA-resident owning tensors, so singular_values(std::move(cuda_matrix)) and the async consuming form reach cuda_singular_values_from_consumed. ConsumingCallUsesOrCopiesDeviceWorkspace now also checks whether the caller's tensor was overwritten (expected for tall column-major and wide row-major, unchanged otherwise, for shapes with min(rows, cols) > 1). Against the previous constraint that test fails for all four scalar types; with the fix uni20_cusolver_tests passes 24/24 on polaron and memcheck is clean.
There was a problem hiding this comment.
Review Bot: recheck of exact head 20f85e803be7cdbe014787ab60e170d94453deba
Confirmed: the fix works. Consumed CUDA matrices now reach cuda_singular_values_from_consumed, and the updated test catches the old behaviour.
-
Your test fails without the fix. I put the
0551fbeops/svd.hppinto a copy of this head and kept the new test.ConsumingCallUsesOrCopiesDeviceWorkspacethen fails for all four scalars. That is 32 failed assertions: the 8 in-place cases per type (4x4, 6x3, 8x5 and 70x33 column-major; 4x4, 3x6, 5x8 and 33x70 row-major). The other 14 singular-values tests still pass. On the real head,uni20_cusolver_testspasses 24/24. -
Which call forms run in place. I added counters to the in-place and copy branches in a scratch copy and called each form, for all four scalars:
- lvalue,
constrvalue, and an rvalueconj(m)view: copy path, input unchanged; singular_values(std::move(m))and the async consuming form: in place.
So only owning, non-const rvalues are consumed.
- lvalue,
Setup and remaining gaps are in the general comment for this head.
|
Review Bot: review of exact head Found 1 issue; details in the inline comment on What changed since The finding. The CUDA branch in the consuming What I reviewed.
Verified (polaron, 2x Quadro GV100
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
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>
|
Review Bot: review of exact head No defect found. My earlier finding (unreachable consuming CUDA path) is fixed; the details are in the reply on that thread. What changed since
What I reviewed.
Verified. Setup: polaron, 2x Quadro GV100 (
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
linalg::singular_valueson aCudaTensorused to copy the matrix to the host and run LAPACK, returning a hostTensor.make_tensor<ColumnMajor>(view)always materializesHostStorage, so the GPU call silently became a device-to-host transfer plus a CPU SVD.This adds a cuSOLVER
singular_values_opbackend for real and complexfloatanddouble, and makessingular_valueskeep CUDA-buffer inputs on their device, returning aCudaTensorthere.Backend.
gesvdwithjobu = jobvt = 'N'and null U/VT, which LAPACK and cuSOLVER leave unreferenced for job'N';rworkis allocated rather than passed as 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. Wide column-major and tall row-major decline asunsupported_layout. Empty matrices succeed without device work. (Cytnx crashes on null U/V withgesvdj/gesvdpno-vector modes, Cytnx-dev/Cytnx#1102 and #831;gesvdwith'N'is the documented route.)Tensor API. The preserving form makes one same-device workspace copy: column-major for tall input, row-major for wide, so cuSOLVER always sees a tall matrix and no transpose kernel is needed. 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.
Multi-device fix. cuSOLVER launches into the calling thread's current device, and the adapter never selected the operand device. The existing full SVD therefore failed with
CUSOLVER_STATUS_EXECUTION_FAILEDfor operands on any device other than the current one; the existing tests only used device 0. Both cuSOLVER execution paths now select the operand device around the workspace query, solver call anddevInfocopy. The new device-1 tests reproduce the failure before the fix.Validation on polaron (2x Quadro GV100, sm_70, CUDA 12.9, GCC 13 Debug):
uni20_cusolver_tests: 24/24 passed, none skipped. Singular values are checked against matrices built as Householder × diag × Householder with known values (no second SVD path), for all four scalars, shapes 1x1 through 70x33/33x70, column- and row-major inputs, preserving/consuming/async forms, explicit selector, empty matrices, kernel declines, and values on device 1.compute-sanitizer --tool memcheck --leak-check fullover the 18 singular-values tests: 0 errors, 0 bytes leaked.ctest: 2403/2405. The two failures are unrelated:uni20_common_testsdoes not build in CUDA configurations (cuda_runtime_api.hviaprecision_cases.hpp→tensor.hpp→linalg/backends/cuda/copy.hpp; no file in that chain changed), andCudaTensorTest.StreamOrderedAccessCanBeCoAwaitedAsTensorViewfailed once underctest -j4and passed 5/5 alone.UNI20_BACKEND_CUSOLVER=OFF:uni20_linalg_cuda_tests58 passed (4 peer-copy tests skipped on one visible GPU),uni20_linalg_tests384 passed.uni20_linalg_tests384 anduni20_linalg_decompositions_tests3 passed.Follow-up (separate PR): audit and remove implicit CUDA→host copies. The
Tensor(TensorView const&)converting constructor, its CTAD, andmake_tensor(view)accept CUDA sources and targetHostStorage, and the other linalg value APIs (svd_left,svd_right,svd,truncated_svd,eigh,qr,lq,solve,lu) materialize host work through them. Restricting those entry points to same-domain sources would turn every remaining implicit transfer into a compile error, includingsingular_valuesin builds without cuSOLVER.🤖 Generated with Claude Code