[AMDGPU] Surface QuadrantsAssertionError after in-kernel assert (fix barrier hang) - #870
[AMDGPU] Surface QuadrantsAssertionError after in-kernel assert (fix barrier hang)#870paveltc wants to merge 3 commits into
Conversation
Replace S_ENDPGM with __builtin_trap so peer wavefronts waiting on s_barrier do not hang the host, and publish assert state into pinned coherent host memory so the host can format QuadrantsAssertionError after hipErrorLaunchFailure (HIP context is dead afterward). Co-authored-by: Cursor <cursoragent@cursor.com>
Timeout is enforced by the subprocess.run(..., timeout=) path instead. Co-authored-by: Cursor <cursoragent@cursor.com>
Some ROCm/HSA configurations (notably inside Docker) turn the in-kernel __builtin_trap() into an uncatchable SIGABRT rather than returning a catchable hipErrorLaunchFailure, so the host never raises QuadrantsAssertionError. Treat a SIGABRT-killed child as a skip (an environment limitation) while still failing on the wall-clock timeout (barrier-hang regression) and on a wrong/absent exception. Upstream AMDGPU CI runs bare-metal, where the trap is catchable. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb26004957
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| amdgpu_system_mem_fence(); | ||
| __builtin_trap(); |
There was a problem hiding this comment.
Publish assert state before peer wavefronts can trap
When an assertion fails in multiple wavefronts, the winning wave sets runtime->error_code = 1 before copying and publishing the pinned state. Another wavefront can observe that value, skip the locked section, and reach this unconditional trap before st->error_code is stored; the host hook then sees code 0 and reports a generic HIP launch failure instead of QuadrantsAssertionError. Ensure only the publisher can trap, or make other failing wavefronts wait until the pinned error state has been published.
Useful? React with 👍 / 👎.
| // After the assertion has been surfaced, further HIP calls on the dead context also return | ||
| // launch failure; ignore them so Program teardown does not terminate() from a destructor. | ||
| if (amdgpu_device_assert_already_surfaced()) { | ||
| return; |
There was a problem hiding this comment.
Do not swallow post-assert failures during normal execution
After the first caught AMDGPU assertion marks this process-global flag, every later HIP call returning 719 silently succeeds through this return, not just calls made during teardown. For example, the existing test_assert_minimal flow catches func() and then expects func2() to raise, but the dead context's launch/synchronization failures are ignored, allowing subsequent kernels or copies to appear successful and expose stale results. Limit suppression to the destructor/finalization path and continue reporting the dead context during ordinary API calls.
Useful? React with 👍 / 👎.
|
Superseded by #871, which raises the same change from the AMD-Ecosystem organization fork per policy (topic branch on an AMD org fork → upstream). Closing this one to avoid duplicates. |
Summary
On AMDGPU, a failed in-kernel
assertpreviously emittedasm("S_ENDPGM"), which only terminates the faulting wavefront. Peer wavefronts still waiting ons_barrierthen deadlock, and the host hangs forever inhipStreamSynchronize. This PR replaces that with a dispatch-wide__builtin_trap()and translates the resulting fault back into a properQuadrantsAssertionErroron the host, preserving the debug-mode assertion contract without hanging.CUDA / CPU / Metal paths are unchanged. All new behavior is gated on
debug+Arch::amdgpu.Approach
__builtin_trap()faults the whole dispatch, so the host getshipErrorLaunchFailure(719) rather than a hang — but the context is then dead, so the usual device-side error-retrieval kernels can no longer run. To preserve the error message:materialize_runtime(debug + amdgpu only) wehipHostMalloc(...Coherent)anAmdgpuAssertErrorStateand publish its device-mapped address into the runtime. This mirrors the existingadstack_overflow_flag_dev_ptrprecedent and survives a device fault.quadrants_assert_format, the faulting wavefront (serialized under the existingerror_message_lock) copies the message template + arguments into the pinned buffer, issues a system-scope fence (amdgpu_system_mem_fence, patched to an LLVM seq_cst fence inllvm_context.cpp), storeserror_codelast, then__builtin_trap()s.AMDGPUFunction::operator()interceptshipErrorLaunchFailure, and a debug-only hook reads the pinned state and raisesQuadrantsAssertionError(a subclass ofAssertionError). Subsequent 719s on the now-dead context are ignored soProgramteardown does notterminate()from a destructor.Testing
Validated on an AMD Instinct MI308X (gfx942), ROCm 7.2.4, base
upstream/main:tests/python/test_assert.py::test_amdgpu_assert_raises— a failed assert raisesQuadrantsAssertionErrorwith the formatted message;isinstance(e, AssertionError)holds.tests/python/test_assert.py::test_amdgpu_assert_barrier_no_hang— one thread asserts while siblings hitblock.sync(); raises instead of hanging (the original bug).test_assert_*) unchanged.Both new tests run each case in an isolated child subprocess (the HIP context is dead after a trap; HIP is unsafe after
fork) with a wall-clock timeout that fails on the barrier-hang regression.CI notes
Upstream AMDGPU CI (
test_gpu.yml→test_linux_amdgpu,runs-on: amdgpu) runs bare-metal on the self-hosted runner — no container — which matches the environment where the trap returns a catchablehipErrorLaunchFailure. Some ROCm/HSA configs (notably inside Docker) instead escalate the trap to an uncatchableSIGABRT; the tests treat aSIGABRT-killed child aspytest.skip(environment limitation) while still failing on timeout or on a wrong/absent exception, so no runner goes spuriously red.Known limitations / possible follow-ups
AmdgpuAssertErrorStateHostView) inllvm_runtime_executor.cpp; astatic_asserton size/offsets would harden this.Program).Made with Cursor