[LAPACK][cuSOLVER] Fix incorrect QR results on repeated runs (#626) - #748
Open
zjin-lcf wants to merge 4 commits into
Open
[LAPACK][cuSOLVER] Fix incorrect QR results on repeated runs (#626)#748zjin-lcf wants to merge 4 commits into
zjin-lcf wants to merge 4 commits into
Conversation
The Householder-family cuSOLVER routines (geqrf, orgqr, ormqr, gebrd, orgbr, orgtr, ormtr and the complex ung*/unm* variants) passed nullptr for cuSOLVER's devInfo argument and skipped the lapack_info_check that every other routine (getrf, potrf, ...) performs. Besides losing all error reporting, this omitted the implicit synchronization that lapack_info_check performs (it reads devInfo back via a blocking queue.wait()). Without it, the SYCL event returned from the native-command submission could signal before the cuSOLVER kernels had finished, so a subsequent memcpy read partially-computed data. This produced nondeterministic, size-dependent wrong results - e.g. QR of a diagonal matrix returning Q diagonals stuck at the input value on the second and later runs for n >= 256 (issue uxlfoundation#626). Allocate a real devInfo and call lapack_info_check in all of these routines (buffer and USM paths), matching the established pattern. This both restores error checking and removes the race. Also align CusolverScopedContextHandler::get_stream with the cuBLAS backend by returning the interop handle's native queue (ih.get_native_queue()) instead of the queue's default stream, so cuSOLVER work is enqueued on the stream the SYCL runtime tracks for native-command completion. Fixes uxlfoundation#626.
andrewtbarker
approved these changes
Aug 10, 2026
andrewtbarker
left a comment
andrewtbarker
left a comment
Contributor
There was a problem hiding this comment.
This looks fine to me.
Suggestion: add a functional test that runs geqrf+orgqr repeatedly on a known-diagonal matrix to guard against regressions.
Can you add this test either here or in another PR?
Adds a functional test that repeatedly runs geqrf + orgqr on a known diagonal matrix (whose Q is the identity) and verifies Q's diagonal is 1 on every run. This guards against a regression of the cuSOLVER synchronization bug from uxlfoundation#626, where run 2+ returned corrupted results. Co-authored-by: Cursor <cursoragent@cursor.com>
sknepper
reviewed
Aug 12, 2026
sknepper
left a comment
sknepper
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the fix and the new test! A couple of suggestions
| if (m < n) | ||
| throw unimplemented("lapack", "gebrd", "cusolver gebrd does not support m < n"); | ||
|
|
||
| int* devInfo = (int*)malloc_device(sizeof(int), queue); |
Contributor
There was a problem hiding this comment.
Ideally we'd check that malloc was successful (devInfo not null), to avoid the situation we're in now where we are passing a null ptr but there are issues with repeated runs.
| #ifdef CALL_RT_API | ||
| oneapi::math::lapack::geqrf(queue, m, n, A_dev, lda, tau_dev, scratchpad_dev, | ||
| geqrf_scratchpad_size); | ||
| oneapi::math::lapack::orgqr(queue, m, n, k, A_dev, lda, tau_dev, scratchpad_dev, |
Contributor
There was a problem hiding this comment.
I believe orgqr should either wait for geqrf to finish, or pass in the output event from geqrf
…LVER A failed malloc_device silently degraded into passing a null devInfo to cuSOLVER, which disables the routine's error reporting - exactly the situation that hid the bug from uxlfoundation#626. Route every USM devInfo allocation through a create_devinfo helper that throws device_bad_alloc instead. Co-authored-by: Cursor <cursoragent@cursor.com>
…sion test The test creates a default, out-of-order queue, so the USM orgqr call was not ordered after geqrf. Pass the geqrf event as a dependency; the buffer path keeps relying on accessor ordering. The test needs no reference implementation, so stop dropping the whole LAPACK domain when Netlib LAPACKE is absent - only the tests that compare against a reference are skipped now. Co-authored-by: Cursor <cursoragent@cursor.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #626.
QR decomposition (
geqrf+orgqr) on the cuSOLVER backend returned incorrect results on repeated runs: for a diagonal input matrix the diagonal ofQshould be1, but from the second run onwards (forn >= 256) some entries stayed at the input value2. The corruption was nondeterministic and size-dependent, and did not occur on the Intel backends.Root cause
The Householder-family cuSOLVER routines (
geqrf,orgqr,ormqr,gebrd,orgbr,orgtr,ormtr, and the complexung*/unm*variants) passednullptrfor cuSOLVER'sdevInfoargument and skipped thelapack_info_checkcall that every other routine (getrf,potrf, …) performs.Beyond losing all error reporting, this also skipped the implicit synchronization that
lapack_info_checkperforms: it readsdevInfoback through a blockingqueue.wait(). Without that wait, the SYCL event returned from the native-command submission could signal before the cuSOLVER kernels had actually finished, so the subsequentmemcpyread partially-computed data — producing the observed run-2+ corruption.Fix
devInfoand calllapack_info_checkin all of these routines (both buffer and USM paths), matching the established pattern used by the working routines. This restores error checking and removes the race.CusolverScopedContextHandler::get_streamwith the cuBLAS backend by returning the interop handle's native queue (ih.get_native_queue()) instead of the queue's default stream.Test plan — validated end-to-end on NVIDIA device, CUDA 13.2
Built oneMath with
-DENABLE_CUSOLVER_BACKEND=ON -DENABLE_CUBLAS_BACKEND=ON -DTARGET_DOMAINS=lapackusing an open-source DPC++ toolchain (intel/llvm nightly, CUDA backend), and ran the #626 reproducer (QR of a diagonal matrix, checkingQdiagonals across repeated runs):develop(unpatched)Also verified
n = 256,512,1024all pass with the fix.Attribution note for reviewers
I isolated the two changes on the device:
devInfo/lapack_info_checkchange only (stream change reverted): PASS.get_streamchange only (devInfo change reverted): FAIL — still races.So the
devInfo/lapack_info_checkaddition is what actually resolves #626 (via the synchronization it introduces and by matching the working routines). Theget_streamchange is included as a correctness/consistency alignment with the cuBLAS backend, not as the fix itself; happy to drop it if maintainers prefer a minimal change.devInfo/lapack_info_checkcounts balanced across buffer and USM paths.Suggestion: add a functional test that runs
geqrf+orgqrrepeatedly on a known-diagonal matrix to guard against regressions.