Repository navigation
fix(hip-kernel-provider): log why Gfx950AttentionDense declines a graph - #12927
Merged
Merged
Conversation
hipDNN reports a declined graph to its caller only as "No engine configurations available for the graph", and the ingestor SDK's INFO line names neither the engine nor the cause. A BHSD graph, the most common decline, gave the user nothing to act on. The graph match now returns a reason with each decline, and logs it at INFO as "hipkernel:Gfx950AttentionDense declined the graph: <reason>". Layout reasons name the operand, its uid, dims and strides, and the BSHD strides the kernel bakes. Every other decline names its field and value (data type, head size, GQA grouping, mask bounds, sinks, missing scale, and so on). Which graphs match is unchanged.
This was referenced Oct 2, 2026
✅ 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. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The diagnostic refactor preserves matcher outcomes and includes focused logging tests.
Review effort: Balanced
Findings: None
What changed in this PR
Adds actionable INFO logging when Gfx950AttentionDense declines an SDPA graph without changing matching behavior.
Changes:
- Returns detailed decline reasons from graph matching.
- Logs operand, layout, mask, dtype, shape, and unsupported-feature failures.
- Adds host tests for decline and success logging.
| File | Description |
|---|---|
Gfx950AttentionDenseNative.cpp |
Produces and logs detailed graph-decline reasons. |
TestGfx950AttentionDenseMatchers.cpp |
Verifies diagnostic logging and successful-match silence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ging graph_match returns std::optional<BoundTokens> again. Each decline site logs through HIPDNN_PLUGIN_LOG_INFO and the message, including the operand descriptions, is built inside the macro argument, so nothing is formatted when INFO is off. - Each line carries a stable cause key: "<engine> declined the graph [layout]: ...". The tests check the key and the operand named, not the wording. - Operand dims and strides print through vecToString and convertFlatBufferVectorToStdVector; null vectors print as []. - notBshdReason re-checks the rank before indexing dims. - Enums stream through FlatbufferTypeHelpers.hpp, which gains an operator<< for AttentionImplementation. - The engine name is GFX950_ATTENTION_DENSE_ENGINE_NAME in IngestorPacks.hpp, shared by the pack table, the pack's log lines and the descriptor census, which fails if the UED declares another name. - The local concat and formatAxes helpers are gone.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12927 +/- ##
===========================================
- Coverage 72.12% 70.68% -1.44%
===========================================
Files 2861 2940 +79
Lines 472095 484567 +12472
Branches 69805 69878 +73
===========================================
+ Hits 340452 342483 +2031
- Misses 107911 118191 +10280
- Partials 23732 23893 +161
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
BrianHarrisonAMD
approved these changes
Oct 4, 2026
BrianHarrisonAMD
left a comment
BrianHarrisonAMD
left a comment
Contributor
There was a problem hiding this comment.
LGTM, but you'll need to sequence with your #12926
graph_match runs for every graph the catalog misses on, so the "not a single SDPA-forward node" decline logged a line for convolution, normalization and every other non-attention graph. That decline now logs only when the graph has an SDPA-forward node somewhere (two SDPA nodes, or SDPA fused with other ops). Any other graph declines silently. The check runs only when INFO is enabled. New test: a single pointwise graph declines with no decline line.
Adds TestFlatbufferTypeHelpers to the flatbuffers SDK tests: each AttentionImplementation value streams its schema name, and an out-of-range value streams an empty string.
1 task done
yassinsolim
added a commit
that referenced
this pull request
Oct 5, 2026
Closes #12915 ## Motivation cuDNN applies no scale to an SDPA graph that sets neither `attn_scale` value nor tensor: cudnn-frontend adds the `attn_scale` multiply to the SDPA node only when one is set (`include/cudnn_frontend/node/scaled_dot_product_flash_attention.h`), and its docs give the default as 1.0 (`docs/operations/Attention.md`). hipDNN did not agree with itself on this. `hipkernel:Gfx950AttentionDense` declined such a graph, while the CPU and GPU references, the ASM SDPA engine and hip_flash2 applied 1/sqrt(head_size). Running the hipDNN kernel-integration runbook on gfx950 (MI355X), all 12 rocKE perf-oracle graphs in the dnn-benchmarking corpus omit the scale, so Gfx950AttentionDense served none of them. Per review, this PR makes every hipDNN site read an absent `attn_scale_value` (with no scale tensor) as 1.0, in one change, so no engine and reference disagree in between. Brian Harrison cc'd Daryl Hawkins (@DarylHawkinsAMD) on this scope change. RFC 0017 and its worked example described the old 1/sqrt default and are updated here. ## Technical Details Behavior changes, each now 1.0 when the graph sets no scale: - `hipkernel:Gfx950AttentionDense` (`Gfx950AttentionDenseNative.cpp`): binds `attn_scale_value().value_or(1.0F)` instead of declining. - test_sdk CPU reference (`CpuFpReferenceSdpa.hpp`, forward and backward). The `cpu_graph_executor` plans pass the missing scale through as nullopt, so they follow. The SDPA dynamic tolerance (`DynamicTolerancesSdpa.hpp`) models the same scale. - GPU reference (`GpuFpReferenceSdpa.hpp`, `fprop`). - ASM SDPA forward and backward plan builders: both now call `plan_utils::attnScaleOrDefault` (`SdpaPlanUtils.hpp`). - hip_flash2: `extractParams` stores the resolved scale (`attnScaleFor`), and `HipFlash2FwdPlan` launches with it as is. The old 0.0 sentinel, which also turned an explicit `attn_scale_value` of 0.0 into 1/sqrt(D), is gone. - `generate_sdpa_fwd_golden.py`: with no `--attn-scale` the scale is 1.0 (generator 1.3.0). Checked-in data keeps its meaning: - A scan of every checked-in JSON graph with an SDPA node (66 files, all under `integration-test-bundles`): 50 set `attn_scale_value`, 15 (`SdpaFwdRuntimeScale`) use a runtime scale tensor, and one set neither, `quick/SdpaBackward/Default/graph.template.json`. That template now states `attn_scale_value = 1/sqrt(128)`, the scale it was graded with before. All 16 of its sweep cases have D = 128. The scan finds no graph left with neither. #12933, which re-enables these backward bundles in CI, edits only their `sweep.json`, so the two PRs do not conflict. - `generate_golden_data.sh` relied on the generator default to reproduce the 35 golden bundles. It now passes `--attn-scale` with the same 1/sqrt(D_qk) value, so a regeneration reproduces the checked-in data. Every bundle's `.meta.json` scale matches its graph. Docs: the frontend (`SdpaAttributes.hpp`, `SdpaBackwardAttributes.hpp`, `Graph.hpp`, `SdpaBwdNode.hpp`), `samples/README.md` and RFC 0017 with its worked example now describe the scale as `attn_scale` (1.0 when unset), not a fixed 1/sqrt(d_k). Left unchanged on purpose: rocKE's Python attention dispatch (`rocke/library/dispatch/attention/bindings.py`) and `hipdnn_torch` default to 1/sqrt(d) because they follow PyTorch's `scaled_dot_product_attention`. `hipdnn_torch` always sets the scale on the hipDNN graph, so it does not depend on the hipDNN default. Tests and samples that set 1/sqrt(D) explicitly are also unchanged. What could break: a graph with no scale now computes softmax(QK^T) with no scaling on every engine. Code that relied on the old implicit 1/sqrt(D) has to set the scale. Overlap with #12927 (#12916, decline reasons): both edit the same `attn_scale_value` hunk in `Gfx950AttentionDenseNative.cpp` and `TestGfx950AttentionDenseMatchers.cpp`. Whichever lands second needs a small conflict resolution. Keep this PR's default and drop that PR's `attn_scale_value` decline and its test case. ## Test Plan Every changed site has a test that an absent scale behaves as 1.0. Each was also run against a mutant that restores the old default (1/sqrt(D), or the 0.0 sentinel for hip_flash2), with the tests unchanged: - `TestCpuFpReferenceSdpaFp64.DefaultScaleIsOne` (renamed from `DefaultScaleIs1OverSqrtD`), plus the hand-computed tests that relied on the old default (`SanityCheck`, `GqaDifferentKVHeads`, `CausalMask`, `LseOutputMatchesFormula`, `BackwardSanity`), now computed with 1.0. - `TestCalculateSdpaFwdTolerance.CustomScaleVsDefault` and the default-scale tolerance cases. - `TestGpuSdpaFwdDefaultScale.AbsentScaleIsOne` (GPU reference, hand-computed expectation). - `TestSdpaFwdPlanBuilder.AbsentAttnScaleValueIsOne`, `TestSdpaBwdPlanBuilder.AbsentAttnScaleValueIsOne`, `TestHipFlash2AttnScale.AbsentAttnScaleValueIsOne`, `TestGfx950AttentionDenseGraphMatch.AbsentAttentionScaleBindsOne`. - `reference-data-scripts/tests/test_generate_sdpa_fwd_golden.py`, registered as the `hipdnn_golden_generator_python_tests` CTest entry. It skips when PyTorch is not installed. - GPU knob suite: the two NoScale gfx950 dense cases removed in the previous revision are back. Engine and CPU reference now agree on 1.0. Builds: develop 0d45e83 plus this branch, preset `hip-kernel-provider` with the kernel ingestor and rocKE on, SDPA, ASM SDPA and hip_flash2 on, `GPU_TARGETS=gfx950`. - clang-tidy 20 ON (in-tree `.clang-tidy`, warnings as errors) on a CPU node, over every translation unit that reaches a changed C++ file. Files whose diff is only comments were left out of the selection, because including `Graph.hpp` would pull in most of the tree. - `ENABLE_CLANG_TIDY=OFF` full build, install and tests on gfx950 (MI355X). ## Test Result | | result | |---|---| | clang-tidy 20, 71 translation units (CPU node) | 0 warnings, 0 failed objects | | `hipdnn_test_sdk_tests` (gfx950) | 2251 passed, 3 skipped, 0 failed | | `hip_kernel_provider_tests` (gfx950) | 1089 passed, 29 skipped, 0 failed | | `hipdnn_integration_tests_unit_tests` (gfx950) | 887 passed, 2 skipped, 0 failed | | `hipdnn_gpu_ref_tests --gtest_filter=*Sdpa*` (gfx950) | 134 passed, including `AbsentScaleIsOne` | | gfx950 dense GPU knob suite, incl. both NoScale cases | 19/19 passed, knob-filter case 1/1 | | gfx950 dense gpu-ref bundle target (12 SdpaFwd bundles) | passed | | ASM SDPA forward GPU cases on gfx950 | 13 passed, 5 failed: the five `Dqk192_Dv128` cases. They fail the same way on unchanged develop 0d45e83 (`hipModuleLaunchKernel` failure, 512-wide blocks for a 256-wide kernel); #12931 fixes that. 32 gfx942-only cases skipped. | | `hipdnn_golden_generator_python_tests` | Skipped in the CI image, which has no PyTorch. Run locally with PyTorch 2.14 (CPU): 2/2 passed. | | Mutant (old defaults restored, tests unchanged) | the 4 test_sdk tests, the 4 engine tests and `AbsentScaleIsOne` all FAILED; the generator test failed with scale 0.25 (1/sqrt(16)) where 1.0 was expected | Not run: hip_flash2 and the ASM SDPA backward engine on a GPU. Both are gfx942-only and no gfx942 node was used, so their absent-scale paths are covered by the host tests only. clang-format 18 and black 25.12.0 are clean on the changed files. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
# Conflicts: # dnn-providers/hip-kernel-provider/src/engines/kernel_ingestor_engine/packs/Gfx950AttentionDenseNative.cpp
…merge Removing the no-scale decline case from LogsTheCauseOfEachOtherDecline left two blank lines before the loop. clang-format 18 rejects it.
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
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.
Closes #12916
Motivation
When
hipkernel:Gfx950AttentionDensedeclines a graph, the log says onlyingestor: engine declined graph_match for device N, and the frontend reports "No engine configurations available for the graph." Running the hipDNN kernel-integration runbook, a BHSD graph from the dnn-benchmarking corpus was declined this way, and finding the cause meant reading the matcher.GraphMatchFnhas no reason channel, and the pack returned a barenulloptat each decline site.Technical Details
Gfx950AttentionDenseNative.cpp:graph_matchstill returnsstd::optional<BoundTokens>. Each decline site logs its cause throughHIPDNN_PLUGIN_LOG_INFOand returnsnullopt. The whole message, including the operand descriptions fromdescribeOperand()andnotBshdReason(), is built inside the macro argument, and the macro checks the INFO level before evaluating it. With INFO off, a decline does no formatting work. Which graphs match does not change.hipkernel:Gfx950AttentionDense declined the graph [layout]: Q (uid 1): dims [2, 4, 256, 128], strides [131072, 32768, 128, 1] is not dense BSHD (token-major, head fastest); the kernel bakes strides [131072, 128, 512, 1] (unit-extent axes exempt) and takes no stride arguments. The keys arenode,operand,layout,data_type,shape,gqa,head_size,size_limit,mask, one per unsupported optional feature,mma_core_modeandimplementation.vecToString(convertFlatBufferVectorToStdVector(...)), so a null vector prints as[].notBshdReason()checks the rank itself before indexing dims.FlatbufferTypeHelpers.hpp. It gains anoperator<<forAttentionImplementation, next to the existingDataType,NodeAttributesandPointwiseModeones.IngestorPacks.hpp: newGFX950_ATTENTION_DENSE_ENGINE_NAME. The pack table, the pack's log lines and the descriptor census (TestGfx950AttentionDensePacks) use it. The census looks the loaded descriptor set up by that name, so it fails ifgfx950_attention_dense.ued.jsondeclares a different one.The user sees the cause with
HIPDNN_LOG_LEVEL=infoor a log callback at INFO. Returning it through the frontend error would need aGraphMatchFnand frontend API change inprojects/hipdnn, which this PR does not attempt.#12926 (#12915, default attention scale) landed first, and develop is merged in here. An absent
attn_scale_valuebinds 1.0, so this PR no longer declines a scale-less graph: theattn_scale_valuedecline and the "no scale" case inLogsTheCauseOfEachOtherDeclineare gone, and develop'sAbsentAttentionScaleBindsOnecovers that graph.Test Plan
Host tests in
TestGfx950AttentionDenseMatchers.cpp, capturing the log withSharedLogRecorderat INFO. They check the cause key and the operand named, not the wording:LogsTheOperandAndTheBakedStridesWhenQIsNotBshd: keylayout,Q (uid 1)and the expected strides.LogsTheOutputWhenOnlyOIsNotBshd: keylayout,O (uid 4).LogsTheCauseOfEachOtherDecline: two nodes, null strides, FP32, D256, a sliding window, sinks, a COMPOSITE implementation hint.LogsNoDeclineForAGraphItServes.LogsNoDeclineForAGraphWithNoSdpaForwardNode: a RELU-only graph, whichgraph_matchalso sees.TestFlatbufferTypeHelpers.cppinhipdnn_flatbuffers_sdk_testscovers the newoperator<<: eachAttentionImplementationvalue streams its schema name, and an out-of-range value streams an empty string.Linux CPU node, this branch merged with develop bca4446 (which carries #12926), quick-start configure flags (preset
hip-kernel-provider, kernel ingestor and rocKE on, SDPA, ASM SDPA and hip_flash2 on,GPU_TARGETS=gfx950):.clang-tidy(warnings as errors) overhip_kernel_provider_impl,hip_kernel_provider_testsandhipdnn_flatbuffers_sdk_tests, which covers every changed file.hip_kernel_provider_testsandhipdnn_flatbuffers_sdk_testsbuilt and run with no GPU on the node.No GPU is needed: the change is in graph matching and logging.
Test Result
Merged tree, 32-core CPU node, no GPU:
TestGfx950AttentionDense*hip_kernel_provider_tests, fullTestFlatbufferTypeHelpers*hipdnn_flatbuffers_sdk_tests, fullclang-format 18 is clean on all seven changed files.
From the pre-merge revision, not re-run after the merge: the hip-kernel-provider census CTest entries (gfx950), including
TestGfx950AttentionDensePacks, 10/10 passed, and the formatting counter (OFF: 0, WARN: 0, INFO: 18 for 11 decline lines).Not run: a GPU run of this branch.
Submission Checklist