[LAPACK][cuSOLVER] Fix incorrect QR results on repeated runs (#626)

[zjin-lcf]

Summary

Fixes #626.

QR decomposition (geqrf + orgqr) on the cuSOLVER backend returned incorrect results on repeated runs: for a diagonal input matrix the diagonal of Q should be 1, but from the second run onwards (for n >= 256) some entries stayed at the input value 2. 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 complex ung*/unm* variants) passed nullptr for cuSOLVER's devInfo argument and skipped the lapack_info_check call that every other routine (getrf, potrf, …) performs.

Beyond losing all error reporting, this also skipped the implicit synchronization that lapack_info_check performs: it reads devInfo back through a blocking queue.wait(). Without that wait, the SYCL event returned from the native-command submission could signal before the cuSOLVER kernels had actually finished, so the subsequent memcpy read partially-computed data — producing the observed run-2+ corruption.

Fix

• Allocate a real devInfo and call lapack_info_check in 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.
• 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.

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=lapack using an open-source DPC++ toolchain (intel/llvm nightly, CUDA backend), and ran the #626 reproducer (QR of a diagonal matrix, checking Q diagonals across repeated runs):

Build	n=512, 6 runs
develop (unpatched)	FAIL — run 2+ have dozens of wrong diagonal entries
This PR	PASS — all runs, all diagonals == 1

Also verified n = 256, 512, 1024 all pass with the fix.

Attribution note for reviewers

I isolated the two changes on the device:

• devInfo / lapack_info_check change only (stream change reverted): PASS.
• get_stream change only (devInfo change reverted): FAIL — still races.

So the devInfo/lapack_info_check addition is what actually resolves #626 (via the synchronization it introduces and by matching the working routines). The get_stream change 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.

• End-to-end reproduce + fix (n=256/512/1024).
• devInfo/lapack_info_check counts balanced across buffer and USM paths.
• CI: cuSOLVER LAPACK functional tests.

Suggestion: add a functional test that runs geqrf+orgqr repeatedly on a known-diagonal matrix to guard against regressions.

[andrewtbarker]

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?

[sknepper]

Thanks for the fix and the new test! A couple of suggestions

[sknepper]

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.

[sknepper]

I believe orgqr should either wait for geqrf to finish, or pass in the output event from geqrf

[melonakos]

Thanks for chasing this one down, Zheming — #626 has been a bad bug and I think you've found the actual root cause. But I'd like to see this restructured before it goes in, and there's a defect in a helper it leans on. Details below.

The real fix is excellent, and it's one line

The substance of #626 is this, in cusolver_scope_handle.cpp:

-    return sycl::get_native<sycl::backend::ext_oneapi_cuda>(queue);
+    return ih.get_native_queue<sycl::backend::ext_oneapi_cuda>();

That's the bug. cuSOLVER work was going onto the queue's default stream while the SYCL runtime tracked completion via the native-command stream, so the returned event could signal before the computation finished — which is exactly why results looked correct once and wrong on repeated runs. Your comment explaining it is clear and I'd merge that change essentially as-is.

Please split the PR

Right now this is three unrelated changes in one 324-line diff:

1. the stream fix above (~1 line),
2. devInfo error-check plumbing threaded through ~14 routines,
3. a change to LAPACK test gating in tests/unit_tests/CMakeLists.txt.

Reviewer time is the scarce resource on this repo — that's the whole reason this queue is long. Item 1 plus your new geqrf_orgqr_diagonal.cpp test is a small, obviously-correct PR that fixes a real user-facing bug and could land quickly. Item 2 is a behavior change across a lot of surface area that needs more scrutiny (see below). Bundling them means the fix waits on the plumbing.

Could you pull 1 + the test into its own PR? I'll review that one promptly.

The devInfo plumbing sits on a broken helper

Before expanding lapack_info_check to ~14 more USM call sites, the USM overload it calls needs fixing. On develop:

inline void get_cusolver_devinfo(sycl::queue& queue, const int* devInfo,
                                 std::vector<int>& dev_info_) {
    queue.wait();
    queue.memcpy(dev_info_.data(), devInfo, sizeof(int));
}

Two problems:

• The memcpy is asynchronous and never waited on. The returned event is discarded, and lapack_info_check reads dev_info_ immediately afterward. That's a race — the value inspected may be whatever was in the vector before the copy landed. Needs queue.memcpy(...).wait().
• sizeof(int) ignores dev_info_size. For any call with dev_info_size > 1 only element 0 is copied and the rest stay zero-initialized, so errors on all but the first batch entry are silently dropped.

Both predate your PR, so this isn't your bug — but this PR takes a helper that doesn't reliably work and applies it in ~14 more places, which turns a latent problem into a real one. Worth fixing the helper first, in its own commit.

devInfo leaks on the error path in the USM routines

Every USM routine now has this shape:

int* devInfo = create_devinfo(queue, __func__);
auto done = queue.submit(...);
lapack_info_check(queue, devInfo, __func__, func_name);
free(devInfo, queue);
return done;

lapack_info_check throws computation_error when info > 0 — which is its entire purpose — and when it throws, free(devInfo, queue) never runs. So the device allocation leaks precisely when a decomposition fails, and a caller retrying in a loop leaks every iteration.

This is the same exception-safety shape you just fixed properly in #764: cleanup on the happy path only. A small RAII holder for the devInfo allocation would fix all ~14 sites at once, the same way the pointer-mode guard did.

The USM entry points become synchronous

get_cusolver_devinfo calls queue.wait(), so every USM routine now blocks until all queued work completes before returning its event. The USM API's contract is asynchronous — callers get an event specifically so they can pipeline. Making geqrf and friends synchronous to check info is a real semantic and performance change, not just an internal detail.

I don't think there's a clean way to have both eager error reporting and async USM here, which is part of why I'd rather see this discussed in its own PR. One option is checking info lazily or only in the buffer path; another is gating it behind a build option. Worth deciding deliberately.

The test CMake change needs a maintainer decision

Removing list(REMOVE_ITEM TEST_TARGET_DOMAINS "lapack") means LAPACK tests now build when LAPACKE is absent, rather than being skipped. I can see why you need it — your new test doesn't compare against a reference, so it should run without LAPACKE — and I think the direction is right. But it changes the build for every configuration without LAPACKE, so it wants explicit sign-off rather than riding along in a bug-fix PR. @sknepper, worth a look.

Two mechanical blockers

• This PR currently conflicts with develop (mergeable: CONFLICTING). That's why no CI has ever run on it — GitHub can't construct the merge commit, so the pull_request workflows never trigger. This is the answer to the CI question you raised back in August: not a broken pipeline, just a conflict. A rebase should light CI up.
• #758 overlaps this PR entirely. It contains these same three cuSOLVER files with identical line counts, plus the rocSOLVER side. Once this lands, please rebase #758 down to just the rocSOLVER delta. Also worth removing the committed qr_diag_repro binary and qr_diag_repro.cpp from that branch.

Happy to review the split-out stream fix as soon as it's up.