[DONOTMERGE] [AMDGPU] check_out_of_bound assert-state fix (parallel to #871) - #895
[DONOTMERGE] [AMDGPU] check_out_of_bound assert-state fix (parallel to #871)#895hughperkins wants to merge 9 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>
…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 #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>
… ABI guards Codex + Opus review fixes for the in-kernel assert trap path: - Fix multi-wave publish/trap race (Codex P1): set the device-side runtime->error_code gate LAST, after the pinned assert state is fully published and fenced. Previously the gate was set first, so a peer wave could observe error_code==1, skip the locked block, and trap the whole dispatch while this wave was still copying -- leaving the host to read an unpublished pinned buffer (error_code==0) and surface a generic launch failure instead of QuadrantsAssertionError. - Keep the offline cache safe (Codex P1): move assert_error_state_dev_ptr to the end of LLVMRuntime. Inserting it mid-struct shifted every later field; the default-on offline cache keys on the numeric version only, so an old cached kernel would misread the shifted fields. Appending at the tail preserves existing offsets. - Guard ABI drift (Opus): add static_asserts pinning both AmdgpuAssertErrorState and its hand-mirrored host view (AmdgpuAssertErrorStateHostView) to the same canonical layout via the shared constants. - Harden the fence patch (Opus): warn if amdgpu_system_mem_fence is not found during runtime-module patching instead of silently leaving the no-op host stub, which would break publish-before-trap ordering. - Replace non-ASCII em dashes in added lines (Opus) so the Check non-ASCII characters CI job passes. Co-authored-by: Cursor <cursoragent@cursor.com>
Codex #871 review: the in-kernel assert -> __builtin_trap() path is emitted whenever check_out_of_bound is enabled, but the pinned assert state and host launch-failure hook were installed only when config_.debug. Since debug implies check_out_of_bound but not the reverse, a check_out_of_bound=True, debug=False program would trap on an out-of-bounds access with no state to translate the resulting hipErrorLaunchFailure: the host surfaced a generic launch failure on a now-dead context instead of the bounds error. Widen the install gate in materialize_runtime to (debug || check_out_of_bound), so bounds-check-only mode surfaces QuadrantsAssertionError("Out of bound access ...") like debug mode does. - test: add test_amdgpu_out_of_bound_check_only_raises, running an out-of-bounds ndarray write under check_out_of_bound=True, debug=False in the isolated subprocess harness and asserting the bounds error is raised (with an explicit qd.sync(), since non-debug launches do not auto-synchronize). - docs: document the AMDGPU dead-context / one-assert-per-process limitation in user_guide/debug.md.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3508299206
ℹ️ 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.
When an internal AMDGPU runtime assertion fires—for example, allocate_from_reserved_memory() calls quadrants_assert_runtime() after exhausting preallocated memory—this unconditional trap now kills the HIP context even in the default debug=False, check_out_of_bound=False configuration. That configuration never allocates the pinned assert state or installs the launch-failure hook, so stream_synchronize() raises a generic launch failure before AmdgpuDevice::allocate_llvm_runtime_memory_jit() can report its intended out-of-memory error. Restrict the context-killing path to assertions with a host reporting channel, or install that channel for internal runtime assertions as well.
AGENTS.md reference: AGENTS.md:L7-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agent says:
Fixed in 768a76a. You're right -- quadrants_assert_format -> __builtin_trap() is reached by internal runtime assertions (allocate_from_reserved_memory's "Out of pre-allocated memory", ListManager checks) that fire regardless of debug/check_out_of_bound, so gating the reporting channel on debug || check_out_of_bound left the default config trapping into an untranslatable generic launch failure on a dead context.
The pinned assert state + launch-failure hook are now installed unconditionally on AMDGPU in materialize_runtime, so any assert -- internal, bounds-check, or user -- surfaces its real message instead of a bare hipErrorLaunchFailure. (This supersedes the earlier debug || check_out_of_bound gate.)
I did not add a dedicated regression test for the pure internal-runtime-assert path because those asserts (OOM of the preallocated chunk, list-manager exhaustion) cannot be triggered deterministically/safely from a kernel; the existing test_amdgpu_out_of_bound_check_only_raises exercises the non-debug reporting path structurally.
| // swallowed during teardown; materialize_runtime() clears it again on the next init. | ||
| if (config->arch == Arch::amdgpu) { | ||
| amdgpu_set_device_in_teardown(true); |
There was a problem hiding this comment.
After a caller catches an AMDGPU assertion and runs qd.reset(), this process-global flag remains true while the surfaced-assert flag also remains true. A subsequent qd.init(arch=qd.amdgpu) performs driver calls such as device_get_attribute() before materialize_runtime() eventually clears the flag; launch failures from the dead context are therefore silently treated as success, leaving default or invalid outputs and allowing initialization to proceed into allocations on an unusable context. Scope this suppression to the old program's teardown and restore it before returning from finalization.
AGENTS.md reference: AGENTS.md:L7-L13
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agent says:
Fixed in 768a76a. The process-global in-teardown flag opened in pre_finalize() / finalize() was previously cleared only by the next materialize_runtime(), so it lingered across into a subsequent qd.init() in the same process and could silently swallow dead-context launch failures during re-init driver calls.
It is now cleared at the end of LlvmRuntimeExecutor::finalize(), after all teardown synchronize()/free calls have run under the open window. So teardown itself is still covered (destructors do not std::terminate), but from the moment finalization returns, a dead-context launch failure surfaces as a hard error again instead of being treated as success.
💡 Codex Reviewquadrants/quadrants/rhi/amdgpu/amdgpu_driver.h Lines 117 to 118 in b2fb694 Check teardown before invoking the assertion hook
When ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
…ression window Codex #895 review, two P1s on the AMDGPU in-kernel assert / trap path: - "Keep runtime assertions off the unreported trap path": quadrants_assert_format -> __builtin_trap() is also reached by internal runtime assertions (e.g. "Out of pre-allocated memory" in allocate_from_reserved_memory, ListManager bounds checks) that fire even in the default debug=False, check_out_of_bound=False config. The pinned assert state + host launch-failure hook were installed only for debug || check_out_of_bound, so those internal asserts trapped into an untranslatable generic launch failure on a dead context. Install the reporting channel unconditionally on AMDGPU so any assert surfaces its real message. Supersedes the earlier debug || check_out_of_bound gate. - "Close the teardown suppression window after finalization": the process-global in-teardown flag opened in pre_finalize() / finalize() was cleared only by the next materialize_runtime(), so it lingered into a subsequent qd.init() in the same process and could silently swallow dead-context launch failures during re-init driver calls. Clear it at the end of LlvmRuntimeExecutor::finalize(), after all teardown synchronize()/free calls, so post-teardown failures surface as hard errors again. - docs: broaden the debug.md AMDGPU caveat to cover all assertion kinds (user assert, bounds check, internal runtime assert), not just debug / bounds-checking runs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 768a76a7eb
ℹ️ 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.
When offline_cache=True (the Python default) and the same kernel was cached by an earlier build, KernelCompilationManager reuses its serialized CompiledKernelData because get_hashed_offline_cache_key() contains no runtime revision and this commit does not change the numeric cache version. QuadrantsLLVMContext::link_compiled_tasks() embeds the runtime helper in that cached module, so such a kernel retains the old S_ENDPGM implementation rather than reaching this new trap/pinned-state path; an assertion with peers at a barrier can therefore still hang. Add a runtime ABI/codegen revision to the cache key or otherwise invalidate pre-change AMDGPU entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agent says:
Confirmed and fixed in 0ad744c. I verified the mechanism end to end: link_compiled_tasks() clones the runtime module (runtime.cpp, including quadrants_assert_format) into each kernel module, and CompiledKernelData serializes that whole linked module as LLVM IR into the .qdc cache; on a cache hit it is parsed back and JIT'd with no re-link (llvm_context.cpp link_compiled_tasks; compiled_kernel_data.cpp dump/load; amdgpu kernel_launcher.cpp register_llvm_kernel). Since get_hashed_offline_cache_key hashes only kernel IR + config + caps and the cache-manager gate compares only QD_VERSION, a pre-#871 AMDGPU kernel would indeed keep S_ENDPGM and still deadlock peers at s_barrier.
Fix: added an AMDGPU-scoped runtime-codegen revision tag ("amdgpu-runtime-rev:trap-assert-1") into get_hashed_offline_cache_key, which invalidates pre-change AMDGPU cache entries. Scoped to arch == amdgpu so CUDA/CPU/Metal caches are not needlessly invalidated (their baked runtime code is unaffected here: the reorder of runtime->error_code is benign for those backends and the LLVMRuntime layout change was made offset-preserving). The tag should be bumped on future AMDGPU runtime ABI/codegen changes.
|
|
||
| #### AMDGPU: assertions end the GPU context | ||
|
|
||
| On the AMDGPU backend a failed in-kernel assertion is implemented by trapping the entire GPU dispatch, which is what lets the failure raise `QuadrantsAssertionError` instead of deadlocking sibling threads waiting on a barrier. This covers every kind of assertion on this backend: a user `assert` (debug mode), a `check_out_of_bound` bounds-check failure, and internal runtime assertions such as the out-of-preallocated-memory check. A side effect is that the HIP context is left permanently unusable afterward: the assertion is delivered exactly once, and the common "catch the `QuadrantsAssertionError` and keep going" pattern does not work here. Any further GPU work in the same process - including re-initializing Quadrants - raises a hard error rather than silently returning stale results. To recover, start a fresh process. This limitation is specific to the AMDGPU backend; CUDA, CPU, and Metal are unaffected. |
There was a problem hiding this comment.
In containerized ROCm/HSA configurations where __builtin_trap() escalates to SIGABRT, the process terminates and no QuadrantsAssertionError is delivered, as _run_amdgpu_assert_child() explicitly acknowledges and skips at tests/python/test_assert.py:229-240. This paragraph currently promises a catchable exception and exactly-once delivery for the AMDGPU backend without that qualification, so users in those supported-looking environments may rely on recovery code that never runs; document this environment-dependent process-abort behavior.
AGENTS.md reference: AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agent says:
Good catch -- fixed in 0ad744c. debug.md now qualifies that whether the failure is delivered as a catchable QuadrantsAssertionError at all depends on the ROCm/HSA configuration: on bare metal the trap surfaces as a catchable hipErrorLaunchFailure, but in some containerized ROCm/HSA setups it escalates to an uncatchable SIGABRT that aborts the whole process, so no exception is delivered and catch-based recovery does not run there (the same environment _run_amdgpu_assert_child() detects and skips).
…me; doc SIGABRT caveat Codex #895 round 2: - P1 "Invalidate cached kernels before relying on the trap path": the LLVM runtime module (runtime.cpp, incl. quadrants_assert_format) is linked into and serialized with every cached kernel (link_compiled_tasks -> CompiledKernelData IR), while the offline cache key hashes only kernel IR + config + caps and the cache gate compares just QD_VERSION. A kernel cached by an earlier build at the same version therefore keeps the old S_ENDPGM assert path and can still deadlock peers at s_barrier, defeating this PR's fix. Add an AMDGPU runtime-codegen revision tag to get_hashed_offline_cache_key so pre-change AMDGPU entries are invalidated. Scoped to AMDGPU so CUDA/CPU/Metal caches (whose baked runtime code is unaffected here) are not needlessly invalidated. Bump the tag on future AMDGPU runtime ABI/codegen changes. - P2 doc: qualify the debug.md AMDGPU caveat - in some containerized ROCm/HSA configs the trap escalates to an uncatchable SIGABRT (whole-process abort, no QuadrantsAssertionError), so catch-based recovery does not run there.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
things to do:
|
hughperkins
added
awaiting review pass
|
|
||
| #### AMDGPU: assertions end the GPU context | ||
|
|
||
| On the AMDGPU backend a failed in-kernel assertion is implemented by trapping the entire GPU dispatch, which is what lets the failure raise `QuadrantsAssertionError` instead of deadlocking sibling threads waiting on a barrier. This covers every kind of assertion on this backend: a user `assert` (debug mode), a `check_out_of_bound` bounds-check failure, and internal runtime assertions such as the out-of-preallocated-memory check. A side effect is that the HIP context is left permanently unusable afterward: the assertion is delivered exactly once, and the common "catch the `QuadrantsAssertionError` and keep going" pattern does not work here. Any further GPU work in the same process - including re-initializing Quadrants - raises a hard error rather than silently returning stale results. To recover, start a fresh process. This limitation is specific to the AMDGPU backend; CUDA, CPU, and Metal are unaffected. One further environment caveat: whether the failure is even delivered as a catchable `QuadrantsAssertionError` depends on the ROCm/HSA configuration - on bare metal the trap surfaces as a catchable `hipErrorLaunchFailure`, but in some containerized ROCm/HSA setups it escalates to an uncatchable `SIGABRT` that aborts the whole process, so no exception is delivered and catch-based recovery does not run there either. |
There was a problem hiding this comment.
this seems way too detailed for user-facing doc. Could potentially be moved to advanced section, but lets just simplify to be the minimum that the user needs to know, no implementation details.
|
|
(agent currnetly fixing CI job failures) |
2 participants
This is a parallel PR to push fixes to #871 -- please DO NOT MERGE.
PR #871 is from the
AMD-Ecosystemorg-owned fork, and GitHub disables "allow edits by maintainers" for org-owned forks, so these fixes can't be pushed directly onto #871's branch. This branch (hp/amdgpu-assert-cob-fix) is based on #871's head and carries all of its commits plus one additional fix commit, opened as a same-repo PR so CI runs automatically and the diff is reviewable. The extra commit should be folded into #871 (cherry-pick or cross-fork merge) rather than merging this PR.Added fix commit (3508299)
Addresses the Codex review P1 on #871, "Install assert state for bounds-check-only mode":
__builtin_trap()path is emitted whenevercheck_out_of_boundis enabled, but the pinned assert state + host launch-failure hook were installed only whenconfig_.debug. Sincedebugimpliescheck_out_of_boundbut not the reverse, acheck_out_of_bound=True, debug=Falseprogram would trap on an out-of-bounds access with no state to translate the resultinghipErrorLaunchFailure, surfacing a generic launch failure on a now-dead context instead of the bounds error.materialize_runtimeto(debug || check_out_of_bound).test_amdgpu_out_of_bound_check_only_raises-- an out-of-bounds ndarray write undercheck_out_of_bound=True, debug=Falsein the isolated subprocess harness, asserting the bounds error is raised (with an explicitqd.sync(), since non-debug launches do not auto-synchronize).user_guide/debug.md.Note
The bulk of this diff is #871 itself (this branch is based on #871's head). Only commit
350829920is new here.Made with Cursor