Repository navigation
hip-kernel-provider: enable minimal single-token attention decode - #13108
tenpercent wants to merge 3 commits into
Conversation
Reuse the gfx942 and gfx950 tiled unified 2D kernels for BF16 B1/Sq1/Hq8/Hkv2/D128 with contiguous KV storage and a device valid length. Add native dispatch, packaged descriptors, matcher/census tests, and exact-engine token-by-token hipDNN validation. Validated on gfx942 and gfx950: four cases, zero skips or failures, 524288 output comparisons. Host matcher/census and focused clang-tidy checks pass.
github-actions
Bot
added
documentation
project: hipdnn
project: hip-kernel-provider
labels
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
|
Local correctness and architecture review of Recommendation: changes needed before merge — one confirmed correctness defect. The integration architecture is appropriate for the deliberately narrow first milestone. P1 / high, confirmed: unused V-cache NaNs contaminate valid decode outputThe new dispatch path launches full-tile kernels on the caller's cache. The documented contract requires a valid length and ordered appends, but does not require initializing unused capacity. The kernels load whole V tiles. Masking the attention scores zeroes probabilities beyond the valid length, but the PV matrix multiply still consumes those V entries: Reproduction: change only the unused V-cache fill in IntegrationGpuAttentionDecode.cpp:160: std::vector<uint16_t> hv(32768, uint16_t{0x7fc0}); // BF16 quiet NaNQ, K, appended V tokens, scale, length, reference math, provider and kernel archives remain unchanged. With capacity 128 and valid length 1, output should equal the finite first V token. Both supported masks instead fail
These were actual Slurm GPU runs using ROCm 10.0.0: four failures, zero skips, with the expected engine/kernel selected and the production provider unchanged. The NaNs are outside the logical valid sequence. The existing finite Suggested fix: make invalid V operands safe before PV and add a partial-tile regression. For the smallest integration-only milestone, explicitly require zero-initializing cache capacity before first use and when needed on reuse, document that valid length alone does not protect against arbitrary nonfinite tail contents, and test that initialization workflow. If untouched unused storage is intended to work, the kernel must mask/zero invalid V operands. Architecture assessment
Validation and limitsAll three matcher tests and ten descriptor-census CTest entries passed again, including negative controls. Diff whitespace checks passed. Existing finite-tail GPU validation remains valid for its initialized inputs: four cases across gfx942/gfx950, 128 appends per case, 524,288 comparisons, maximum absolute error 0.00297844. The independent NaN-tail probe establishes a limit of that coverage. Capacity endpoints 64/65536, other finite scales, simultaneous executions and the full production descriptor catalog were not numerically revalidated in this review. No graph-capture or performance guarantee was established. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #13108 +/- ##
===========================================
- Coverage 72.12% 71.02% -1.10%
===========================================
Files 2861 2944 +83
Lines 472089 485461 +13372
Branches 69803 70085 +282
===========================================
+ Hits 340449 344768 +4319
- Misses 107910 116765 +8855
- Partials 23730 23928 +198
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The new native GPU ABI and dispatch path requires the pending architecture review and broader provider regression validation.
Review effort: Balanced
Findings: None
What changed in this PR
Adds minimal BF16 single-token attention decode through the generic kernel ingestor for gfx942/gfx950.
Changes:
- Adds native graph matching, dispatch, workspace adaptation, and registration.
- Packages architecture-specific rocKE descriptors.
- Adds matcher, census, GPU integration tests, and interface documentation.
| File | Description |
|---|---|
projects/hipdnn/tools/IngestorGenerator/configs/attention_decode.yaml |
Defines the decode pack and kernels. |
dnn-providers/hip-kernel-provider/src/tests/engines/kernel_ingestor_engine/packs/TestAttentionDecodePacks.cpp |
Verifies packaged inventory. |
dnn-providers/hip-kernel-provider/src/tests/engines/kernel_ingestor_engine/packs/TestAttentionDecodeMatchers.cpp |
Tests graph admission constraints. |
dnn-providers/hip-kernel-provider/src/tests/engines/kernel_ingestor_engine/CMakeLists.txt |
Registers matcher and census sources. |
dnn-providers/hip-kernel-provider/src/tests/CMakeLists.txt |
Registers architecture-specific census tests. |
dnn-providers/hip-kernel-provider/src/integration_tests/kernel_ingestor_engine/IntegrationGpuAttentionDecode.cpp |
Exercises incremental GPU decode. |
dnn-providers/hip-kernel-provider/src/integration_tests/CMakeLists.txt |
Adds the integration test target. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/packs/AttentionDecodeNative.cpp |
Implements matching and dispatch. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/IngestorPacks.hpp |
Declares pack registration APIs. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/IngestorPacks.cpp |
Registers the decode engine. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/descriptors/rocKE/attention_decode/kernel_dtype_matches_graph.umd.json |
Defines kernel matching metadata. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/descriptors/rocKE/attention_decode/attention_decode.ued.json |
Defines the engine descriptor. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/descriptors/rocKE/attention_decode/attention_decode.udd.json |
Defines dispatch binding. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/descriptors/rocKE/attention_decode/attention_decode.kmd.json |
Defines variant metadata. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/descriptors/rocKE/attention_decode/attention_decode.kdp.json |
Packages architecture kernels. |
dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/CMakeLists.txt |
Builds the native implementation. |
dnn-providers/hip-kernel-provider/docs/attention_decode.md |
Documents the supported interface. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
This pull request has been automatically closed due to inactivity (30 days with no updates). If you'd like to continue working on this, feel free to reopen the PR or create a new one. |
ISSUE ID : AICK-2504
Single-token decode (
Sq=1) cannot use the existing tiled dense-attention ingestor pack. This addshipkernel:AttentionDecodeto the generic ingestor, reusing the gfx942/gfx950 tiled unified 2D kernels.The initial slice is BF16, B=1, Hq=8, Hkv=2, D=128, with contiguous token-major KV storage and a device INT32 valid-length scalar. The application zero-initializes the full K/V cache before each new sequence, appends K/V, and executes the same hipDNN plan once per token. The adapter supplies query offsets and an identity page table in caller workspace without copying KV. It supports unmasked and bottom-right causal attention and produces O only.
flowchart TD subgraph Caller["Application"] Init["Zero full K/V cache before each new sequence"] Append["Append K/V token; update Q and device valid length"] Init --> Append end subgraph Plan["Reusable hipDNN execution plan"] Match["AttentionDecode matcher validates supported graph"] Pack["Packaged gfx942/gfx950 kernel descriptor"] Prepare["Prepare kernel and immutable adapter table"] Match --> Pack --> Prepare end Append --> Execute["Execute on the hipDNN stream"] Prepare --> Execute Execute --> Workspace["Copy query offsets and identity page table into caller workspace"] Workspace --> Kernel["Tiled unified attention kernel"] Cache["Caller-owned Q, K/V cache and valid length"] --> Kernel Kernel --> Output["Write O"] Output -. next token .-> AppendCache initialization is required: valid length masks attention scores, but the kernels load full tiles and nonfinite unused V entries can contaminate PV. Arbitrary nonfinite or uninitialized tails remain unsupported. The integration regression starts each of two sequences with NaN-poisoned K/V storage, applies the required zero-initialization, and reuses the same plan, buffers and workspace across 128 appends per sequence.
The change includes native matching and dispatch, packaged descriptors, registration, interface documentation, matcher/census tests, and a frontend-to-provider integration test. Split-KV optimization, external paged inputs, broader shapes/dtypes, LSE, speculative decode, graph capture, and kernel masking of arbitrary nonfinite tails remain outside this initial scope.
Validation at
76dcb44220ae8ae8a0a7681b619464a0d2b1e50b:0.00297844with per-element limit0.015 + 0.015*abs(reference).68038701(gfx942,quanta-ccs-aus-f01-40) and68038702(gfx950,smci355-ccs-aus-m13-25) completed with exit0:0. An earlier gfx950 node returnedhipErrorNoDevicebefore execution; the identical artifact passed on the replacement node.Device validation used a focused production descriptor root containing the new pack; this is not a full-provider regression or performance claim. Capacity endpoints and other finite scales have admission coverage only. The PR remains a draft for review.
Tracked by AICK-2504, a child of AICK-2500.