Repository navigation
fix(rocSOLVER) Fix race condition in SYEVJ GPU kernel on gfx11 (LLVM Issue #207883) - #12266
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Restore reference-result validation and add regression coverage for the affected gfx11 sizes.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes a gfx11 SYEVJ GPU-kernel race by adding synchronization around LDS accesses. The change also removes validation checks from host LAPACK reference helpers.
Changes:
- Adds synchronization barriers for shared-memory reads and writes.
- Removes
info == 0assertions from reference routines. - Requires regression coverage for affected matrix sizes.
File summaries
| File | Summary |
|---|---|
projects/rocsolver/library/src/lapack/roclapack_syevj_heevj.hpp |
Adds barriers protecting SYEVJ/HEEVJ LDS accesses. |
projects/rocsolver/clients/common/misc/lapack_host_reference.cpp |
Removes host-reference LAPACK status checks. |
Review details
Suppressed comments (4)
projects/rocsolver/clients/common/misc/lapack_host_reference.cpp:8651
- Removing these assertions from the
sytf2/sytrfreference wrappers makes factorization failures (including singular pivots) invisible at the wrapper boundary. The associated tests can then compare results without an explicit reference-status check, weakening failure diagnostics; please preserve the check or return/handleinfoexplicitly.
ssytf2_(&uploC, &n, A, &lda, ipiv, info);
projects/rocsolver/clients/common/misc/lapack_host_reference.cpp:6331
- This removes the debug check that the
sytrs2reference solve succeeded. Without replacing it with explicitinfohandling, a failed CPU reference can be consumed as if it were valid and obscure the real test failure; please preserve the check or propagate the status to the caller.
ssytrs2_(&uploC, &n, &nrhs, A, &lda, ipiv, B, &ldb, &(work[0]), &info);
projects/rocsolver/clients/common/misc/lapack_host_reference.cpp:8702
- This removes the debug-time check for
sytrf's returnedinfowhile the wrapper still writes the factorization outputs. If the CPU factorization fails, the test can proceed with an invalid reference without any diagnostic; preserve validation or propagate and handle the status explicitly.
ssytrf_(&uploC, &n, A, &lda, ipiv, work, &lwork, info);
projects/rocsolver/library/src/lapack/roclapack_syevj_heevj.hpp:178
- The check-in SYEVJ/HEEVJ parameterization still samples n=40 and n=67, so it does not exercise the gfx11 small-kernel window around n=48–58 that this synchronization change is intended to fix. Please add regression cases for the affected sizes (or otherwise add a focused test) so this barrier ordering is covered in CI.
__syncthreads();
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
🟡 Changes recommended
The added size cases do not reliably reproduce the data-dependent, intermittent race.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
…Issue #207883) (#12266) ## Motivation <!-- Explain the purpose of this PR and the goals it aims to achieve. --> This PR fixes a race condition in SYEVJ GPU kernel. The problem showed up on gfx11 architecture. The solution is to add calls to __syncthreads() to avoid race condition hazards in accessing arrays cosine_res[] and sines_diag[] in LDS shared memory. This will address JIRA ROCM-31219 https://amd-hub.atlassian.net/browse/ROCM-31219 ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> The problem was initially identified by github user zpengmei in PR 9135 (now closed) #9135 The issue was initially thought to be related to "[AMDGPU] Miscompiled post-barrier LDS reload in a large-workgroup HIP kernel on gfx11 (silent wrong results) #207883) llvm/llvm-project#207883 Anshil Gandhi (angandhi@amd.com) has identified the source of the problem of race condition where synchronizations are required. This PR adds the calls to __synchthreads() in the GPU kernel for SYEVJ. ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> Pass google tests for all routines that use the Jacobi method on all architectures, including gfx11. ## Test Result <!-- Briefly summarize test outcomes. --> All tests passed. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. --------- Co-authored-by: Mark Gates <mark.gates@amd.com>
TheRock Submodule Bump ActivityNewest first
|
Motivation
This PR fixes a race condition in SYEVJ GPU kernel. The problem showed up on gfx11 architecture.
The solution is to add calls to __syncthreads() to avoid race condition hazards in accessing arrays cosine_res[] and sines_diag[] in LDS shared memory.
This will address
JIRA ROCM-31219
https://amd-hub.atlassian.net/browse/ROCM-31219
Technical Details
The problem was initially identified by github user zpengmei in PR 9135 (now closed)
#9135
The issue was initially thought to be related to "[AMDGPU] Miscompiled post-barrier LDS reload in a large-workgroup HIP kernel on gfx11 (silent wrong results) #207883)
llvm/llvm-project#207883
Anshil Gandhi (angandhi@amd.com) has identified the source of the problem of race condition where synchronizations are required.
This PR adds the calls to __synchthreads() in the GPU kernel for SYEVJ.
Test Plan
Pass google tests for all routines that use the Jacobi method on all architectures, including gfx11.
Test Result
All tests passed.
Submission Checklist