Repository navigation
fix(hip-kernel-provider): ASM SDPA backward rejects the a16 accumulator - #14687
Open
brockhargreaves-amd wants to merge 1 commit into
Open
brockhargreaves-amd wants to merge 1 commit into
brockhargreaves-amd wants to merge 1 commit into
Conversation
SdpaBwdPlanBuilder advertised the sdpa.bwd.accumulator_type knob with valid values a32 and a16, and initializeExecutionSettings accepted a16. isApplicable checks only the A32 kernels. With an A16 setting, buildPlan asks for a DQDKDV row with atomic32=0, pssk=1, pddv=1, and the gfx942 registry has none: its 24 a16 rows all have pssk=0, hd64 or hd128, and mask none or top-left causal. So buildPlan threw INTERNAL_ERROR "failed to resolve dqdkdv kernel ... (isApplicable should have rejected)" for every graph isApplicable accepts. On gfx942 (MI300X), develop's builder built all 42 accepted graphs of a sweep with a32 and failed all 42 with a16: bf16 and fp16; hd128 with no mask, top-left and bottom-right causal and sliding window; hd192 with no mask and both causal masks. hipDNN users cannot reach this today. AsmSdpaEngine publishes no builder knobs and never calls initializeExecutionSettings, so it always builds A32 plans, and the frontend refuses or drops knobs that an engine does not advertise. This change leaves the engine alone: with only a32 valid there is nothing to forward, and forwarding the knobs before this fix would have exposed the failure. Selecting the a16 rows that do exist would not fix it either. The gfx942 a16 kernels add dQ atomically, in the 16-bit output type, into the output dQ, address dQ and dO with Q's strides, and never load seqlen_k, the window bounds, or any V, dO, dK or dV stride other than dK's seq stride. In a bf16 diagnostic run with those rows patched in, cases inside that envelope matched a32 to 1 bf16 ulp. A dQ holding stale values, a dQ or dO layout that differs from Q's, Sq != Skv, and S not a multiple of 64 each gave wrong gradients, and a padded Q/dO row pitch and S=300 also wrote past the end of dQ. Now: - getCustomKnobs lists only a32. - initializeExecutionSettings rejects a16 with INVALID_VALUE and a message of its own. Other unknown strings still get INVALID_VALUE and non-string values BAD_PARAM. - buildPlan refuses an A16 setting with an INTERNAL_ERROR that says so, before it queries the device or resolves a kernel. It is the only gate: getMaxWorkspaceSize still sizes an A16 setting without dq_acc, but no A16 plan is ever built to use that size, and the A16 code in buildPlan and SdpaBwdPlan is kept for a future enablement. - Comments in the builder, the plan, and the params, kernel-args and workspace-size headers that said A16 can be selected, or that its kernels write dQ directly in BF16 and need no dQ zeroing, now say that the a16 kernels add dQ atomically into the output dQ, which must be zeroed first, and that A16 is refused. The comment on the rejection lists the graphs that getCustomKnobs and initializeExecutionSettings would have to limit A16 to before it could be enabled. isApplicable cannot do that: it sees no knob, and it also validates A32, which runs graphs A16 cannot. An unset knob and a32 behave as before, and isApplicable and the A32 plans are unchanged. Tests in TestSdpaBwdPlanBuilder.cpp: - GetCustomKnobsReturnsAccumulatorKnob now expects only a32 (gfx942 only, like the other isApplicable tests). - InitializeExecutionSettingsAcceptsA32 covers a32, and an unset knob from an invalid config, a valid config with no knobs, and a valid config with only another knob. - InitializeExecutionSettingsRejectsA16 - InitializeExecutionSettingsRejectsUnknownAccumulator - BuildPlanRejectsA16Setting, which needs no GPU. - Gfx942HasNoA16DqdkdvForDispatchedLayout pins the registry fact that the rejection comment cites, with the pssk=0 a16 rows as positive controls. Checked on gfx942 (MI300X). All 228 asm_sdpa unit tests pass except IsApplicableRejectsGfx950, which skips on gfx942 as it does on develop (223 tests there). The host-only tests also pass on a machine without a gfx942 device. Each of 9 mutants makes one of the tests above fail: mapping a16 back to A16, dropping the a16 arm, letting a32 fall through, listing a16 again, dropping the buildPlan check (buildPlan then fails to resolve a DQDKDV kernel, as on develop), accepting unknown strings, dropping the has-knob check (a valid config without the knob then throws std::out_of_range), pointing the registry check at gfx950, which has such rows, and making every lookup in it miss. develop's quick and standard SdpaBackward bundles (8 cases) pass against the CPU reference with this plugin, as with develop's, and still build A32 plans. clang-format 18 and clang-tidy 20 are clean on the changed files. Rebased onto develop 98b504d, after #14075, whose range gate now runs first in isApplicable. The GPU results above are from develop 61c2675. On the new base, the four ALMIOPEN-3035 ASM SDPA fix branches (backward layouts, attention scale, a16 accumulator, forward layouts) were built and run together: - hip_kernel_provider_tests: 912 pass and 7 skip on gfx942 (MI325X), 886 pass and 33 skip on gfx950 (MI355X); none fail. - hip_kernel_provider_integration_tests: none fail on either. - develop's SDPA bundles on ASM_SDPA_ENGINE, run like the CI lane (claims enforced): 62 pass and 56 skip on gfx942, 15 pass and 103 skip on gfx950, and no support claim fails. A host-side run of the real isApplicable over these bundles finds no outcome change from develop. - No #14622 SdpaBackward case changes outcome: no bundle sets a knob (host-checked for this branch alone). - clang-tidy 20 and clang-format 18 are clean on the changed files. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 task done
⏳ Waiting on Required Checks⏳ Required checks have not all concluded yet. This bot run is exiting without failing so those checks can report independently.
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
⏳ Waiting for required checks to start or finish:
|
This was referenced Oct 29, 2026
brockhargreaves-amd
marked this pull request as ready for review
October 30, 2026 19:26
15 of 16 tasks
brockhargreaves-amd
added a commit
that referenced
this pull request
Nov 5, 2026
…rated bundles (#14622) ## Summary This PR adds the SDPA backward test plan required by RFC 0022 (Per-Op Test Plans, merged in #13937). It also adds a generator that writes the SdpaBackward bundles from the plan, and the bundles it writes. The 16 inherited SdpaBackward cases (all B = H = 1, packed BHSD, fixed scale) are replaced by 503 generated cases in four levels. Each case carries a purpose and traces to the plan items it covers. > Written with AI assistance (Claude Code). The plan follows the merged RFC 0022; it records one remaining deviation (DV-4) for reviewers to approve. JIRA ID : ALMIOPEN-2894 ## Risk Assessment Risk 2 of 5. The PR changes test data, test tooling, test configuration and documentation only; no library or provider code changes. The changes that reach beyond the SdpaBackward bundles: - `support_matrix_units.collect_sweep_units()` leaves out cases whose `metadata.purpose` is `probe`, as RFC 0022 §9 requires. Today only SdpaBackward cases carry a `purpose`, so other ops' matrices are unchanged. The committed `SUPPORT_MATRIX_OVERVIEW.md` is re-rendered. - `ASM_SDPA_ENGINE.toml`: two dtype-only SdpaBackward tolerance rows are replaced by 12 generated rows (seven tolerance classes), plus temporary skips for the open engine defects, each naming its ticket. - `ASM_SDPA_ENGINE_test_categories_integration.yaml`: six rejection probes that ASM_SDPA_ENGINE wrongly accepts are quarantined with their tickets (RFC 0022 §6). Both files affect the ASM lane only. - #14358's eight PyTorch golden cases move, unchanged, from `quick/SdpaBackward/{Default,AdditiveMask}/` to `quick/SdpaBackward/Golden/{Default,AdditiveMask}/`, so the generator owns the `Default` sweeps. They keep their fp16 tolerance row. `bhsd/` and the `*_fwd_pair` bundles are untouched. The main risk is CI noise from new cases. Every level has been run on gfx942 and gfx950 with no failure (see Testing Checklist). ## ASIC Coverage The bundles run in every provider lane whose tier YAML matches `*Sdpa*`. Only `ASM_SDPA_ENGINE` accepts SdpaBackward graphs today, and only on gfx942: its backward plan builder declines every other device ("Device string does not match gfx942"). Claims are harvested for gfx942 only. PR CI tests gfx942. On gfx950 every generated backward case is declined, so a gfx950 run only shows that nothing fails; one is recorded below. PR CI on gfx942 is sufficient. ## Testing Summary - **Generator tests** (`hipdnn_sdpa_bwd_bundle_generator_python_tests`, label `quick`, 19 tests). They fail when the checked-in sweeps or the TOML block drift from the generator. They also check legality, all-pairs coverage up to nightly, the per-case ceilings and cumulative budgets of each level, two-way traceability between plan items and cases, that every TOML skip still matches a written case, and that the tier-YAML quarantine holds only rejection probes. - **`generate_sdpa_bwd_bundles.py --dry-run --report`.** Reports counts, budgets, the all-pairs proof, legality, and each declared-but-gated case with its gate. - **Claims run** (2026-10-26, MI300X, gfx942). All generated bundles ran on `ASM_SDPA_ENGINE`, configured like CI. `harvest_support_claims.py` turned the passing cases into the 272 claims. Every claimed case passes its tolerance class with a margin of at least 2.4 (median 4.2). - **Measured run, every level on GPU** (2026-11-04, plan §4.5). `hipdnn_integration_tests` ran on `ASM_SDPA_ENGINE` configured like the CI lane: `--test-engine` (claims enforced), the PR's `ASM_SDPA_ENGINE.toml`, the tier YAML's quarantine as a GTest filter, device input fill and the GPU reference as oracle. It ran all four levels, on a fresh MI300X (gfx942) and an MI355X (gfx950) node, built from this branch. The bundles, TOML and YAML are identical to `6e014ad50e0`. - **PR CI** runs the quick and standard levels on gfx942. ## Testing Checklist - [x] Generator tests - CTest `hipdnn_sdpa_bwd_bundle_generator_python_tests` - 19 of 19 - Status: Passed - [x] Plan report - `python3 reference-data-scripts/generate_sdpa_bwd_bundles.py --dry-run --report` - 503 written, 77 gated, 0 legality violations, all-pairs 669 of 669 - Status: Passed - [x] Bundle and support-matrix checks - `verify_support_claims.py`, `render_support_matrix.py --output -`, `feature_matrix.py --check`; the bundle, claim, render and support-feature pytest suites - Status: Passed (`verify_golden_bundles.py` reports no SdpaBackward error; its 322 errors in other ops' bundles are on develop too) - [x] pre-commit on the changed files - Status: Passed - [x] Claims run - `hipdnn_integration_tests`, `ASM_SDPA_ENGINE`, MI300X (gfx942), 2026-10-26 - Status: Passed - [x] Measured run, gfx942 - `hipdnn_integration_tests --test-engine ASM_SDPA_ENGINE --test-config ASM_SDPA_ENGINE.toml '--gtest_filter=*SdpaBackward*-<the tier YAML quarantine>'` - MI300X, Slurm job 68275259 - Status: Passed. 553 tests in 43.3 s, 0 failed, no claim broken: - 289 executed and passed against the GPU reference, and 15 expected rejections rejected; - 226 declined by ASM, and 23 hit the DEF skips; - per level: quick 2.4 s, PR 2.8 s, nightly 16.5 s, full 21.4 s. - [x] Quarantine check, gfx942 - the six quarantined cases run alone - Status: all six fail as expected ("Engine ASM_SDPA_ENGINE accepted expected-rejection bundle"), which shows the quarantine is needed. - [x] Every level, gfx950 - same command - MI355X, Slurm job 68275260 - Status: Passed. 0 failed: ASM declines every backward graph on gfx950, and the rejection probes, quarantined ones included, are correctly rejected. - [x] The other SDPA bundles, same lane - `--gtest_filter=*Sdpa*:-*SdpaBackward*` - Status: Passed. gfx942: 47 passed, 84 skipped. gfx950: 16 passed, 115 skipped. 0 failed on either. - [ ] PR CI on gfx942 - TheRock multi-arch CI - Status: Pending on `6e014ad50e0`. The 2026-11-02 run failed one case, `prbr01`, which is now gated. Later runs failed only on the hipCCL configure break from #14976, which develop reverted in #15089 (merged into this branch). ## Technical Changes ### What this adds - **The plan:** `dnn-providers/integration-tests/docs/test-plans/sdpa-bwd.md`. It follows the RFC 0022 template: the operation, the feature inventory, the shape catalog, building the sweep (levels, budgets, probes, regressions, trade-off register), the golden reference, pass criteria, and exhaustive mode. - **The generator:** `reference-data-scripts/sdpa_bwd_plan.py` holds the declarations: axes, legality, the shape catalog, level rules and the cost model, one record per case. `generate_sdpa_bwd_bundles.py` writes the sweeps, the support-claim files and the engine-TOML tolerance block from them. Nobody edits the bundles by hand. RFC 0022 open question 1 cites this module as the per-op Python option. - **The bundles:** 503 written cases, spread across the levels: | Level | Directory | Cases | |---|---|---| | quick | `quick/` | 56 | | PR | `standard/` | 152 | | nightly | `comprehensive/` | 220 | | full | `full/` | 75 | - Each case carries `purpose` (`claim`, `probe` or `regression`) and its plan items. - 77 more cases are declared but not written yet, each waiting on a named gate: an oracle extension, a harness feature or golden data. - One of the 77 is PRB-R01 (`dropout_probability = 0`). Since #14358 the references decline any graph that sets `dropout_probability`, so it waits on ALMIOPEN-3149 or ALMIOPEN-3150. - **Levels:** the cost model assigns each case to the lowest level whose per-case limits it fits: | Level | Per-case time | Per-case bytes | Cumulative budget | |---|---|---|---| | quick | 1.5 s | 4 MiB | 20 s | | PR | 5 s | 16 MiB | 120 s | | nightly | 75 s | 64 MiB | 480 s | | full | 300 s | no limit | 1,800 s | Each cumulative budget covers that level and every level below it, and keeps 5 % in reserve. The budgets come from a CPU-reference cost model; with the GPU reference, the measured run above is far below them. The ASM lane YAML picks the cases up by GTest prefix (`quick_*Sdpa*`, `standard_*Sdpa*` and so on), and each level also runs every level below it. - **Rejection probes** (21) carry `metadata.expected_rejection` (#14346): the harness never executes them, and an engine that accepts one fails the lane. PRB-M02 and PRB-M02f (`right_bound = -2`) are not written, because the frontend rejects them since #14358; `TestSdpaBwdNode.PreValidateChecksDiagonalBandBounds` covers that rule. - **dBias:** the five dBias probes and the live GL-14 case are written, now that #14358's references model dBias. To keep PR and nightly inside their reserve, the plan's trim rule moved M07.pr to nightly and M26.n to full. - **Tolerance classes:** seven classes, written as 12 `[[tolerance_overrides]]` rows between generated BEGIN and END markers in `ASM_SDPA_ENGINE.toml`. - **Support claims:** 272 claims for `ASM_SDPA_ENGINE` on gfx942. - **Probes left out of the support matrices** (RFC 0022 §9). This is the `support_matrix_units` change above. - **Generator file I/O is UTF-8**, so its tests pass on Windows. ### Temporary ASM_SDPA_ENGINE exclusions These stay until each fix lands; the PR with the fix deletes its entries. **Open engine defects** (`[[test_skips]]` in `ASM_SDPA_ENGINE.toml`). These are valid graphs that ASM accepts and computes wrong. Other engines still run them. | Cases | What goes wrong | Ticket | Fix | |---|---|---|---| | DEF-01 | head-dim stride other than 1 | ALMIOPEN-3036 | #14684 | | DEF-02 | non-packed Stats | ALMIOPEN-3037 | #14684 | | DEF-04 | Hk = Hq > Hv | ALMIOPEN-3039 | #14684 | | DEF-05 and H61 | odd element strides | ALMIOPEN-3038 | #14684 | | DEF-06 | scale of 0 or below | ALMIOPEN-3043 | #14688 | | DEF-03 | fp16 dV precision in the AITER kernel | ALMIOPEN-3046 | none in hipDNN (kernel) | **Quarantined rejection probes** (`_common_excludes` in `ASM_SDPA_ENGINE_test_categories_integration.yaml`, as RFC 0022 §6 asks). These are undefined graphs ASM should decline but accepts. | Cases | What ASM accepts | Ticket | Fix | |---|---|---|---| | prbu01, prbu03 | O or dQ dims that differ from Q's | ALMIOPEN-3039 | #14684 | | prbu13 | a dQ sequence stride below D (overlapping rows) | ALMIOPEN-3038 | #14684 | | prbt04, prbt04n | an O dtype that differs from Q's | ALMIOPEN-3050 | none yet | | prbr05 | `dropout_scale_inv` without a dropout mask | ALMIOPEN-3051 | none yet | ### Dependencies (all merged) - #13937, RFC 0022 (2026-10-30). Its first-phase documents, the methodology guide and the `docs/test-plans/` index, are not on develop yet, so the plan links the RFC's sections directly. - #14312: O and Stats derived from the forward of the same inputs. - #14483: `left_bound` uses cuDNN's meaning. The bundles write `left_bound = W`. - #14358 (2026-11-02): SDPA references cover the full graph API, and the GPU reference is the first oracle in `auto` mode. It also adds frontend rule R14 (`right_bound >= -1`). - #14346 (2026-11-03): the `expected_rejection` and `no_*_reference` bundle flags. - #14709 (2026-11-02): zero strides in the data SDK `Tensor`. ### Interaction with other open PRs - **#14684, #14686, #14687, #14688** (ASM declines, ALMIOPEN-3035). Each deletes its own entries: TOML skips for the DEF cases, and tier-YAML lines for the quarantined probes it fixes (#14684: prbu01, prbu03, prbu13). Whichever of these and this PR lands second removes the entries. - **#14999** (SDPA forward plan bundles, ALMIOPEN-2893). Both PRs edit two files the same way: - `ASM_SDPA_ENGINE.toml`: keep this PR's generated block and put #14999's `SdpaFwdPlan` skips after the `# END SdpaBackward tolerance classes` line. The generator test only checks the BEGIN to END span. The filters do not overlap (`SdpaBackward` vs `SdpaFwdPlan`). - `SUPPORT_MATRIX_OVERVIEW.md`: re-render it; do not hand-merge. The two PRs also add lines near each other in `integration-tests/CMakeLists.txt` and `test_categories_external.yaml`; these auto-merge. ### Before review - [x] `prbr01` (`dropout_probability = 0`) gated; ALMIOPEN-3149 and ALMIOPEN-3150 track the decision. - [x] Rejection probes carry `expected_rejection`; the six ASM accepts are quarantined in the ASM tier YAML with their tickets. - [x] `prbm02` and `prbm02f` removed; the frontend unit test of R14 covers them. - [x] The five dBias probes and GL-14 written; REG-08 (rank-3 Stats) still waits for the derivation extension. - [x] The plan refreshed against the merged RFC: DV-1 and DV-2 now agree with it, DV-3 is an RFC §11 gap, and only DV-4 remains a deviation. - [x] The measured run (plan §4.5) recorded above. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
SdpaBwdPlanBuilderadvertises ansdpa.bwd.accumulator_typeknob with values a32 and a16. With a16,buildPlanthrowsINTERNAL_ERRORfor every graphisApplicableaccepts, because the gfx942 registry has no a16 DQDKDV kernel for the dispatched layout. Ticket ALMIOPEN-3045, epic ALMIOPEN-3035.hipDNN users cannot reach this today:
AsmSdpaEnginepublishes no builder knobs and always builds A32 plans. The fix makes the builder truthful before anyone forwards its knobs.Technical Details
getCustomKnobslists only a32.initializeExecutionSettingsrejects a16 withINVALID_VALUE.buildPlanrefuses an A16 setting before it resolves any kernel.useA32, the two-kernelSdpaBwdPlanconstructor and the A16 branches inexecuteandbuildDqdkdvArgsare now unreachable. They are kept for a future A16 enablement, tracked in ALMIOPEN-3119, which names the known gaps: dQ zeroing,max_seqlen_dqand the gfx950 dq_shuffle stage.ASIC Coverage
The change is in the backward builder, whose
isApplicableaccepts only gfx942, plus host-side settings checks that do not depend on the architecture. TheRock PR CI covers it on gfx94X-dcgpu (unit tests, provider integration tests and the ASM SDPA bundle lane), and passing PR CI is sufficient. The gfx950 column below is supplementary.Test Plan
Unit tests and provider integration tests on gfx942 and gfx950; develop's and #14622's SDPA bundles on
ASM_SDPA_ENGINE.Test Result
This branch at
4bdd8c5123a, in TheRock Multi-Arch CI run 40941612305 (PR CI, which tests the branch merged with develop) on gfx94X-dcgpu. Counts are summed over the two test shards:hip_kernel_provider_tests_standard_suitehip_kernel_provider_integration_tests_standard_suiteIntegrationGpuSdpaBwdbf16 and fp16 cases includedhip-kernel-provider-asm-sdpa-external-integration_standard_suiteSdpaBackwardbundle cases includedCombined build on develop
98b504d92c5(after #14075; supplementary). The four ALMIOPEN-3035 ASM fix PRs (#14684, #14688, this PR, #14686) were built together for gfx942 and gfx950 and run on one MI325X (gfx942) and one MI355X (gfx950). Each branch was also checked on its own with a host-side runner of the realisApplicableand unit tests.hip_kernel_provider_testship_kernel_provider_integration_testsASM_SDPA_ENGINElane config, claims enforced138218c0755), its temporary skips removedclang-tidy 20 (
--warnings-as-errors='*') and clang-format 18 are clean on the changed files.InitializeExecutionSettingsRejectsA16,BuildPlanRejectsA16SettingandGfx942HasNoA16DqdkdvForDispatchedLayoutpass on gfx942 and gfx950.Submission Checklist
🤖 Generated with Claude Code