[AMDGPU] Non-terminating in-kernel assert + OOB index clamp (arch-independent) - #898
[AMDGPU] Non-terminating in-kernel assert + OOB index clamp (arch-independent)#898paveltc wants to merge 3 commits into
Conversation
The AMDGPU in-kernel assert used `S_ENDPGM`, which kills only the faulting wavefront and deadlocks peers parked on `s_barrier` (hanging the host on sync). Switching to `__builtin_trap()` fixes the hang on CDNA but escalates to an uncatchable SIGABRT on some arches (e.g. RDNA/gfx1011), taking down the process. Instead, make the assert non-terminating: the error is already recorded, so the faulting wave now runs to the kernel's natural end, reaching every barrier with its peers. The host raises QuadrantsAssertionError via the normal post-sync error check and the HIP context stays alive. To keep the continued execution safe after a failed bounds check, the AMDGPU-gated check_out_of_bound pass clamps the offending index into [0, size-1] for ndarray/field/matrix accesses so no real out-of-bounds access (and thus no HSA fault) occurs. The assert predicate and message still use the original index; CPU/CUDA paths are unchanged. API impact: none (pure runtime/codegen fix; no new Python API, kwargs, or env vars).
![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: 48df19aa53
ℹ️ 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".
| auto one = new_stmts.push_back<ConstStmt>(TypedConstant(1)); | ||
| auto size_minus_one = new_stmts.push_back<BinaryOpStmt>(BinaryOpType::sub, upper_bound, one); | ||
| auto clamped = clamp_index_to_bounds(new_stmts, stmt->indices[i], zero, size_minus_one); |
There was a problem hiding this comment.
When an AMDGPU kernel accesses an external array whose dimension is zero, upper_bound - 1 becomes -1, so this clamp still redirects the access to an invalid index rather than preventing it. Zero-sized NumPy arrays are supported by the existing API (tests/python/test_numpy.py:198-205); a debug kernel that accesses one will therefore record the assertion and then perform an access at -1, potentially causing the same GPU fault or HSA process abort this change is intended to prevent. The zero-extent case needs to suppress or redirect the access to storage that is actually valid.
Useful? React with 👍 / 👎.
| // Do NOT terminate the wavefront here. Killing only the faulting wave (the old `S_ENDPGM`) deadlocks | ||
| // any peer waves parked on `s_barrier`, hanging the host; killing the whole dispatch (`__builtin_trap`) | ||
| // escalates to an uncatchable HSA exception (SIGABRT) on some AMD architectures (e.g. RDNA / gfx1011), | ||
| // taking down the process instead of raising. Instead, the error is already recorded above; let this | ||
| // wave run to the kernel's natural end so it still reaches every barrier alongside its peers. The host |
There was a problem hiding this comment.
This changes the user-visible semantics of kernel assert on AMDGPU: after a failed assertion, the faulting wave continues executing subsequent statements, so assertions can no longer safely serve as guards and later side effects may occur before Python receives QuadrantsAssertionError. The existing assertion documentation in docs/source/user_guide/debug.md does not mention this backend-specific behavior, and this public usage change needs a corresponding user-facing caveat.
AGENTS.md reference: AGENTS.md:L15-L22
Useful? React with 👍 / 👎.
…sert Address review feedback on the non-terminating in-kernel assert: - P1 (zero-extent ndarray): clamping an out-of-bounds index to 0 cannot make an access to a zero-extent external array safe, because such an array was staged with no device buffer (the launcher skipped it), leaving the kernel a host/null pointer that faults (hipErrorIllegalAddress, escalating to an uncatchable HSA abort on RDNA). The AMDGPU launcher now stages a minimal always-valid device buffer for zero-extent host-side external arrays, so the clamped access lands in valid memory. Apply the clamp lower bound last so a zero-extent dimension clamps to 0 rather than -1. - P2 (docs): document that a failed assert surfaces at the next sync point and, on AMDGPU, the faulting thread runs to the kernel's end, so assert is not a hard mid-kernel guard. Add test_amdgpu_assert_out_of_bound_zero_extent_no_fault covering OOB store and read of an empty ndarray plus context survival. No user-facing API change.
The is_host_external declaration fits within the 120-col limit, so clang-format requires it on a single line. Unwrap it to satisfy the Linters pre-commit check. Whitespace-only; no behavior change.
|
Hi @hughperkins — I pushed one more commit (e22df01) that's a one-line clang-format fix — unwraps a line that fit within the 120-col limit, whitespace-only, no behavior change. It fixes the Linters failure from the previous commit. Since it's a fork PR, the Actions for that commit are sitting in "action_required" — could you hit "Approve and run workflows" when you have a moment? That'll turn Linters green and let the Test Linux AMD GPU job pick it up off a warm runner. |
|
triggered. (not that linters are not blocking for starting the amd runner, AFAIK. The dependency is:
|
2 participants
API impact: none
Pure runtime/codegen fix. No new Python API, kwargs, config toggles, or env vars. The out-of-bounds index clamp lives entirely inside the already debug-gated
check_out_of_boundpass and is scoped to AMDGPU, so non-debug runs and CPU/CUDA codegen are unchanged.Problem
The AMDGPU in-kernel assert used
asm("S_ENDPGM"), which terminates only the faulting wavefront. Peers parked ons_barrierthen deadlock, hanging the host onhipStreamSynchronize. Replacing it with__builtin_trap()fixes the CDNA hang but escalates to an uncatchable SIGABRT on some AMD architectures (e.g. RDNA1 / gfx1011), taking the process down instead of raising a catchable error — so that approach can't be made uniform across AMD arches.Approach (architecture-independent, cooperative)
runtime.cpp): the error is already recorded before the arch-specific tail, so the AMDGPU branch now simply returns. The faulting wave runs to the kernel's natural end and reaches every barrier alongside its peers — no deadlock, no trap. The host raisesQuadrantsAssertionErrorthrough the normal post-sync error check, and the HIP context stays alive (no more one-assert-per-process / dead-context limitation).check_out_of_bound.cpp, AMDGPU-gated): because a failed bounds check now falls through to the access, the offending index is clamped into[0, size-1](min(max(idx,0), size-1)) forExternalPtrStmt(ndarray),GlobalPtrStmt(field), andMatrixPtrStmt. This keeps the continued execution in bounds so there's no real out-of-bounds access (and thus no GPU memory fault / HSA exception). The assert predicate and message still use the original index; only the pointer's index operand is redirected to the clamped value.No trap semantics, pinned host buffers, launch-failure hooks, or fences are needed, so there's nothing that diverges by architecture.
Tests
tests/python/test_assert.pygains three inline AMDGPU tests (no subprocess/SIGABRT/timeout harness needed anymore):test_amdgpu_assert_barrier_no_hang— thread 0 asserts while peers hitblock.sync; must raise, not hang.test_amdgpu_assert_context_survives— after catching an assert, a follow-up kernel runs to completion with correct results.test_amdgpu_assert_out_of_bound_no_fault— OOB field + ndarray accesses raise the correct bounds error with no fault; context stays alive.Local validation (MI308X, gfx942 / CDNA3)
test_assert.py: 22 passed on cpu+amdgpu, no hang.RDNA1 (gfx1011) validation needs the secret-gated AMD runner in upstream CI (see note below).
Note on RDNA1 CI
The gfx1011/V520 job (
test_gpu.yml) requires repo secrets that aren't exposed to fork PRs, so this PR alone won't exercise it. A companion branch on the upstream repo (as was done for the trap approach in #895) is needed to run the RDNA1 validation. This PR intentionally leaves #871 untouched.Made with Cursor