[AMDGPU] Surface QuadrantsAssertionError after in-kernel assert (fix barrier hang) - #871
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".
| // 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.
Stop suppressing all post-assert launch failures
If an application catches the new QuadrantsAssertionError and performs another AMDGPU operation in the same process, the surfaced flag remains true and this global wrapper silently treats every subsequent hipErrorLaunchFailure as success. Because the HIP context is dead after the trap, later launches, copies, and synchronizations can therefore appear successful while returning stale or uninitialized results; restrict suppression to the known teardown path, or explicitly reject further executor use instead of changing every AMDGPU call globally.
AGENTS.md reference: AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
…inding Previously, once an in-kernel assert surfaced, AMDGPUFunction::operator() swallowed every subsequent hipErrorLaunchFailure (719) as success until the next materialize_runtime(). That masks dead-context errors if user code catches the QuadrantsAssertionError and keeps issuing GPU work (Codex Genesis-Embodied-AI#871 P1). Now 719 is suppressed only where throwing would std::terminate(): during teardown (g_amdgpu_device_in_teardown, opened in LlvmProgramImpl::pre_finalize() before the finalize() syncs, cleared on the next materialize) or while unwinding (std::uncaught_exceptions() > 0). Any other post-assert GPU call now raises a clear hard error instead of returning stale/uninitialized results. Adds test_amdgpu_assert_dead_context_reuse_raises to lock in the behavior. All three amdgpu assert tests pass on the MI308X. Co-authored-by: Cursor <cursoragent@cursor.com>
Addressed the P1 (post-assert launch-failure swallowing) in 376f1ffThanks @codex — good catch. The prior code set a "surfaced" flag on the first in-kernel assert and then swallowed every subsequent Fix: 719 is now suppressed only in the two situations where throwing would
Any other post-assert GPU call now raises a clear hard error instead of returning stale/uninitialized results: if (amdgpu_device_assert_already_surfaced()) {
if (amdgpu_device_in_teardown() || std::uncaught_exceptions() > 0) {
return; // swallow: throwing here would std::terminate()
}
QD_ERROR(
"AMDGPU device context is unusable after an in-kernel assertion failure; "
"re-initialize Quadrants in a fresh process before issuing further GPU work "
"(while calling {} ({}))",
name_, symbol_name_);
}Regression test: Validated on an MI308X (gfx942, ROCm 7.2.4): all three assert tests pass. |
|
To use Codex here, create a Codex account and connect to github. |
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
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