Repository navigation
Conversation
1 task
✅ 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. |
aledudek
force-pushed
the
users/aledudek/gfx950-conv-ingestor-dev
branch
from
September 30, 2026 14:18
8e7c0db to
9fd94d4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12780 +/- ##
========================================
Coverage 72.12% 72.12%
========================================
Files 2869 2869
Lines 473609 473609
Branches 69870 69870
========================================
+ Hits 341565 341570 +5
+ Misses 108304 108302 -2
+ Partials 23740 23737 -3
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
aledudek
force-pushed
the
users/aledudek/gfx950-conv-ingestor-dev
branch
from
October 1, 2026 15:41
9fd94d4 to
c21cf42
Compare
aledudek
force-pushed
the
users/aledudek/gfx950-conv-ingestor-dev
branch
2 times, most recently
from
October 2, 2026 13:47
d7f2147 to
3a9c3e3
Compare
aledudek
marked this pull request as ready for review
October 2, 2026 14:04
aledudek
force-pushed
the
users/aledudek/gfx950-conv-ingestor-dev
branch
from
October 6, 2026 12:23
3a9c3e3 to
54717ce
Compare
Contributor
|
With the AOT changes for the rocKE kernels so that the shape isn't hard-coded at compile time, the matcher in here needs to change to also be more relaxed and use the kernels for the range of what they actually can handle at runtime now. |
Contributor
Author
Good point, thanks. Updated the matcher, it no longer requires the exact build shape. It only checks what the binary still bakes in after AOT, so each kernel is used for every shape it can run. |
cgmillette
reviewed
Oct 7, 2026
cgmillette
left a comment
cgmillette
left a comment
Contributor
There was a problem hiding this comment.
PR #12780 — gfx950 rocKE conv fwd as an AOT hipDNN engine (aledudek, AICK-2048) — 🟠 REJECT: sound and well-tested, but needs a careful rebase onto #12659
| Prior item | Status |
|---|---|
| BradPepersAMD: matcher must serve every shape the AOT kernels can run, not just the build shape | 🟢 FIXED 055b1a09: matches on baked caps only (load/store width, grouped/pointwise; direct: filter/stride/pad, spatial G). Same rule as rocKE KernelCache.supports_problem |
| Bots: policy ✅ (AICK-2048), codecov, Math CI | 🟡 Math CI SKIPPED at 055b1a09; codecov tracked +3 lines. GPU/C++ results are author-reported only |
| # | Sev | file:line | Net-new finding |
|---|---|---|---|
| N1 | 🟠 | HkpPackaging.cmake:1273 |
dirty vs develop #12659 (rocKE optional, default OFF): 3 conflicts. The auto-merged hkp_gfx950_conv_fwd_available() still calls _hkp_root_declares_engine_for_arch, which #12659 deleted → "Unknown CMake command" on every gfx950 rocKE-ON configure. Port to develop's probe form (HKP_PACK_SHIPPED_ENGINES_product). |
| N2 | 🟠 | integration_tests/CMakeLists.txt:85, test_desk_check_invariants.py:426 |
Same rebase, 2 silent test breaks. (a) Conv suite compiled ungated (develop gates attention's with hkp_product_offers_engine); it skips only non-gfx950 (IntegrationGpuGfx950ConvFwd.cpp:395) and ASSERTs the engine is offered (:411) → red on gfx950 with rocKE OFF. (b) dtype-premise fix must move to tests/rocke/test_desk_check_rocke_corpus.py:297, which still asserts ("bf16","BF16"); conv bundle is fp16/fp16. |
| N3 | 🟡 | Gfx950ConvFwdNative.cpp:446-462 |
isDefaultDirectArm keys on the kernel's build G, not the graph's. Post-relaxation, on a non-catalog G=32 graph the std bw4 builds for G=64/96 tie the spatial default at 4.0, while the identical G=32 build ranks 3.0. Unbenchmarked order only; test :1204 uses same-shape kernels. Use matched.problem.groups. |
| N4 | 🟡 | gfx950_conv_fwd.kdp.json |
Relaxed matching makes the per-shape catalog mostly duplicates: 88 implicit-GEMM kernels = 24 capability classes; verified IR-identical modulo name for the 11-kernel class. Median catalog graph: 26 candidates / 4 distinct binaries (max 44/12); BenchmarkPlan times every one (1+7 runs). Follow-up: one kernel per class. |
| N5 | 🟡 | PR body; coverage.md:68 |
Stale after 055b1a09. Body: "exact geometry", "per-shape catalog", grouped pointwise "declined". coverage.md:68: depthwise 1×1 s1 p0 "declined by both families". Head serves grouped pointwise (Native.cpp:204-207, README.md:9-10). Fix before squash. |
Utility: 🟢 Positive and needed: first Python-free, runtime-dispatched rocKE conv engine in hipDNN with cross-family benchmarking. 958ea54d fixes a live develop bug (cshuffle store width 8 drops outputs when K/groups % 8 ≠ 0; config-20 IR v4i32→v2i32), so land it first. Matcher, ABI names and kernargs check out against rocKE on host (adapter 181/181). Merge after the N1/N2 rebase.
Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good
…s, grouped included, through hipDNN Add hipkernel:Gfx950ConvFwd, a kernel-ingestor engine that serves 2D forward cross-correlation on gfx950 from rocKE implicit-GEMM kernels compiled ahead of time into the provider's kpack. Each compiled variant is specialized for one convolution geometry; the native pack matches the hipDNN graph against the packaged metadata exactly and launches the code object without Python. Contract: dense channels-last FP16/BF16 storage, FP32 accumulation, symmetric padding, groups inferred as X channels / W dimension 1 (any divisor of C and K up to the grid-z limit, depthwise and channel-multiplier depthwise included). Grouped pointwise (1x1, stride 1, no padding) is declined in the native matcher, the rocKE adapter and the shape miner: rocKE's pointwise shortcut indexes the input and output without the group offset and writes wrong results. Launch geometry runs N-tiles over K/G with one grid z-slice per group, and the filter byte count uses C/G so the B buffer-resource bound matches the packed [K, Y, X, C/G] allocation. The shipped catalog holds synthetic verification shapes only, each with tile_k 64 and 128. Workload shapes are never committed: production catalogs are generated from a private request list and packed from a descriptor root outside the source tree via HIPKERNELPROVIDER_PRODUCTION_SOURCE_ROOT, as documented in docs/gfx950_conv_fwd/README.md. Integration follows the ingestor runbook and the gfx950_attention_dense reference: the bundle lives in the shipped descriptor root; tests register behind hkp_gfx950_conv_fwd_available() and route into the gfx950 census shard; a census suite, matcher/geometry unit tests, a GPU integration test, shared integration bundles and a provider-level external check target with its TOML are added. The native pack uses the KPACK-only kernel-code overload and exports its module cache. The generator config, request inventory and tooling profile (with the specialization block dispatch_parity needs) live under IngestorGenerator/configs; mine_conv_shapes.py and benchmark_conv_integration.py mine requests and probe the integrated path. The attention profile now names its bundle, since the gfx950 shard holds two. Verified on gfx950 with the default production root: provider unit tests, conv and attention census suites, descriptor validator (both engines), desk check and verify_variant_sets in full mode, the conv GPU integration test and external integration suites with grouped and depthwise cases executing, dispatch_parity, launch_surface, the runbook placeholder gate, and the IngestorGenerator and rocKE packaging pytest suites. Not run: the attention GPU suites (SDPA off in this build).
…irect kernel hipkernel:Gfx950ConvFwd served depthwise convolution only through the implicit-GEMM kernel. With one channel per group every lane of that kernel loads from its own cache line, so most of each memory request is discarded and no tile, warp or pipeline choice changes the lane-to-address mapping. rocKE's direct depthwise kernels map adjacent channels to adjacent lanes and read each input line once. They take the same six kernel arguments as the implicit-GEMM kernel and need no LDS or workspace, so they are packaged as a second kernel family in the same engine. Kernel families: - New metadata fields kernel_family (0 implicit GEMM, 1 direct depthwise), direct_variant (std or spatial), block_w and block_waves. kernel_family is an integer engine knob, so a family can be forced; benchmarking compares both families. Without benchmarking, direct candidates rank above implicit GEMM where they apply, the default direct arm first. - Direct kernels carry fixed placeholders for the implicit-GEMM fields. The native pack refuses unknown families and inconsistent field combinations, and launches std and spatial kernels with their own grid and block. - The direct kernel's own symbol omits filter size, padding and stride, so the adapter names every direct kernel from a hash of the full spec. Correctness guards: the direct kernels leave trailing output rows unwritten when padding exceeds (filter - 1) / 2, and on their runtime-loop path can write past the last output row into the next image. Both the rocKE adapter and the native kernel matcher accept a direct kernel only when the last output row covers the last input row (floor((in - 1) / stride) == out - 1, for H and W), with groups == C == K, symmetric stride and padding, no dilation, and the spatial variant only below 64 groups. Every other shape stays on implicit GEMM, which serves it correctly. The kernels themselves are not changed here. Existing kernels are unchanged: the generator gains an opt-in builder_default flag on KMD fields, so descriptors that leave the new fields at their defaults keep their metadata. All previously shipped descriptors, symbols and code objects are byte-identical, pinned by a test. The synthetic catalog adds direct arms for the existing depthwise shapes and new synthetic cases for stride 2 with odd and even heights, a partial channel tile, both runtime-loop paths, and depthwise graphs that must stay on implicit GEMM. benchmark_conv_integration.py discovers and forces either family. Verified on gfx950: provider unit tests, conv and attention census suites, descriptor validator, desk check and verify_variant_sets in full mode, the conv GPU integration test (every direct arm against the CPU reference, cross-family benchmarking and winner reuse, implicit-GEMM fallbacks), the shared integration bundles including the new Depthwise bundle, the runbook placeholder gate, launch_surface, dispatch_parity, and the IngestorGenerator, descriptor-packaging and rocKE packaging pytest suites. Not covered: benchmark_conv_integration.py still needs an implicit-GEMM arm for auto/reuse runs, and the attention GPU suites (SDPA off in this build).
…v pack The Linux superbuild runs clang-tidy with warnings as errors, and the gfx950 conv pack and its tests tripped six checks: a negated conjunction in the epilogue rule (readability-simplify-boolean-expr), a repeated return type (modernize-return-braced-init-list), a redundant member initializer, a push_back in a loop without reserve, an int multiplication widened to int64, and adjacent string literals that clang-format produced by splitting long kernel names in the census inventory (bugprone-suspicious-missing-comma). The census name list is now exempt from clang-format so each kernel name stays one literal. No behaviour change. Verified: clang-tidy with the hipDNN configuration reports nothing in the touched files; provider unit, census and the gfx950 conv GPU integration tests pass.
The AOT switch made the Python builder default the cshuffle store width to 8. When K/groups is not a multiple of 8, a vector store straddles the end of a group's output slab, fails its bounds check and is dropped, so those outputs are never written. This hits grouped, depthwise and dense convs. Restore the default_vector_sizes(cpg, kpg) width, which the C++ engine already uses, so the two engines agree again. Add a grouped config with K/groups = 12 to both conv parity emitters; the byte-identity gate had no case where the two defaults differ.
…OT conv ABI rocKE's conv kernels now take the problem as kernel arguments: implicit GEMM appends 39 i32s and the direct depthwise kernels 14 to the six leading arguments. The native pack computes them as ConvArgs does (extents, packed strides, magic-division pairs, tile counts) and checks a named signature, since every problem argument is a by_value i32 and only names catch a reordering. The adapter exposes launch_signature()/launch_values() over ConvArgs, which the benchmark tool now uses. Mirror rocKE's new direct-conv rule (odd filter, "same" padding) in the native guard, and refuse an implicit-GEMM reduction extent at or above 2^23 as ConvArgs does.
… run With the rocKE AOT change the problem shape is passed to the kernel at runtime, so a packaged kernel is no longer limited to the shape it was built for. Instead of requiring an exact shape match, the matcher now checks only what the binary still keeps from its build problem: - implicit GEMM: grouped vs ungrouped path, the 1x1 fast path, and the load/store vector widths, which must divide the graph's channels per group - direct depthwise: filter, stride, padding, and for the spatial variant the group count Grouped 1x1 graphs are now served by grouped kernels. Without benchmarking, the kernel built for the exact shape is still preferred. Tests now cover shapes no kernel was built for, running every kernel that claims them against the CPU reference. The benchmark tool and docs are updated to match.
aledudek
force-pushed
the
users/aledudek/gfx950-conv-ingestor-dev
branch
from
October 8, 2026 14:48
055b1a0 to
ca5c0b4
Compare
1 task done
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.
ISSUE ID: AICK-2048
Replaces: #12476
Motivation
Ships rocKE-authored gfx950 forward convolutions as a hipDNN engine,
hipkernel:Gfx950ConvFwd. The kernels are compiled ahead of time into the provider's kpack archive and dispatched at runtime without Python. Each problem has several candidate kernels; on first use hipDNN benchmarks them and records the fastest, and later processes reuse the recorded winner without searching again.rocKE's gfx950 forward dispatcher returns one fixed implicit-GEMM configuration for every problem and never selects rocKE's direct depthwise kernels. Packaging both kernel families behind hipDNN's benchmarking gives per-problem selection, including between families.
Rebased onto develop including #12783 (rocKE conv kernels switched to AOT). That change gave both conv kernel families a new kernel-argument ABI, which this PR now launches with. It also introduced a store-width regression for grouped and channel-padded convolutions, which the first commit fixes (see rocKE fix below).
Technical Details
rocKE fix (
fix(rocke): size the conv fwd cshuffle store to K/groups again, own commit, easy to split out): after feat(rocke): switch conv kernels to AOT #12783 the Python builder defaults the cshuffle store width to 8. When K/groups is not a multiple of 8, a vector store straddles the end of a group's output slab, fails its bounds check as a whole and is dropped, so those outputs are never written. Grouped, depthwise and dense convolutions are all affected; the dispatcher picks cshuffle for any even K/groups. The commit restores thedefault_vector_sizes(cpg, kpg)width, which the C++ engine already uses, so the two engines agree again. The byte-identity corpus had no config where the two defaults differ; a grouped config with K/groups = 12 is added to both conv parity emitters. It is red on develop and green with the fix.Native engine pack (
packs/Gfx950ConvFwdNative.cpp,packs/Gfx950ConvFwdGeometry.hpp): graph matcher, kernel matcher, scorer and dispatcher for 2-D forward cross-correlation, NHWC fp16/bf16 tensors, fp32 accumulation, symmetric padding.AOT launch ABI: both families open with
(A, B, D, A_bytes, B_bytes, D_bytes)and append the problem asi32kernel arguments, in the order defined byrocke/library/kernels/common/conv_abi.py:num_pid_m/num_pid_n;ConvArgs.to_launch_valuesdoes (implicitGemmKernargs,directKernargs,magicDivision); unit tests compare them withConvArgsoutput.hkp_packrecords from the code object. Every problem argument is a by_valuei32, so only the names catch a reordering; a mismatch fails atprepare().ConvArgsdoes, for the kernel's 24-bit address products.Two kernel families in one engine, selected per problem:
kernel_family = 0) serves every accepted graph;kernel_family = 1,stdandspatialvariants) serve pure depthwise graphs, with their own ABI block and launch grid.kernel_familyis an integer engine knob; benchmarking compares both families. Without benchmarking or a record, the default direct arm ranks first.forward_padding_reason, mirrored natively), and floor((in - 1) / stride) == out - 1 on H and W. The last check still matters for the width of a non-square filter. Other depthwise graphs stay on implicit GEMM.Builder adapter (
rocke/library/builders/common/convolution_forward.py): builds either family throughhkp_pack, with a build-time support predicate and hashed kernel symbols. Implicit-GEMM pipelines:mem,compv3,compv4,basic.launch_signature()/launch_values()expose the AOT launch throughConvArgs.Catalog (synthetic verification shapes only): 44 requests, 118 kernels (88 implicit GEMM with
tile_k64 and 128, 30 direct), generated fromIngestorGenerator/configs/gfx950_conv_fwd.yamlinto the provider's shipped descriptor rootsrc/engines/kernel_ingestor_engine/descriptors/rocKE/gfx950_conv_fwd/. Unchanged by the rebase: every spec still validates against the AOT builders and every direct entry already uses "same" padding. Workload catalogs are built outside the tree; the procedure is indocs/gfx950_conv_fwd/README.md("Private workload catalogs").Generator (
IngestorGenerator/codegen): opt-inbuilder_default: trueon KMD fields, so fields added with a default are omitted from existing entries and an extended bundle keeps its descriptors byte-identical. Feedback from the hipDNN tooling owners welcome.Test registration:
hkp_gfx950_conv_fwd_available()gates the census suite and the external integration target, which is routed into the gfx950 census shard, withconfig/hipkernel_Gfx950ConvFwd.tomland the categories YAML.Tooling:
mine_conv_shapes.pyderives conv requests from benchmark graphs (its output stays outside the tree);benchmark_conv_integration.pychecks correctness and compares against a direct rocKE launch, for either family, through the adapter's AOT launch helpers.Docs:
docs/gfx950_conv_fwd/(README, graph_contract, coverage, mining), including the AOT launch contract.Test Plan
On gfx950, from a clean build of this branch with every pack re-packed against the AOT builders, and an install of that build:
hip_kernel_provider_tests) and census (hip_kernel_provider_census_testsvia CTest from the install tree).hipdnn_validate_descriptors(both shipped engines),hkp_desk_check --mode full,verify_variant_sets --mode full.hip_kernel_provider_integration_tests): every catalog variant against the CPU reference, every direct arm, cross-family benchmarking with winner reuse, and depthwise graphs outside the direct guard.hipdnn_integration_tests --test-engine hipkernel:Gfx950ConvFwd --reference-executor gpu --fail-on-unsupported): Smoke, Spatial and Depthwise.Test Result
test_hkp_pack_hip.py/test_hkp_pack_layout.py(30 failed, 22 errors) fail on the test machine because its HIP compiles pick up the distro HIP headers instead of ROCm's. This PR changes neither those fixtures nor the HIP compile path.*Gfx950ConvFwd*: 54 passed. Census (gfx950, install tree): 21/21.*Gfx950ConvFwd*133/133; whole binary 162/162.hip_kernel_provider_integration_testsneeds no environment override: itsmain()adds the production descriptor tree throughHIPDNN_DESCRIPTOR_RUNTIME_DIRalongside the test set. The categorized external suites need the TOML next to their CTest file, so they run from an install tree.Known limitations: the shipped catalog is synthetic, so real workloads need a private catalog; depthwise 1x1 at stride 1 without padding is declined by both families; the unbenchmarked order ranks the direct family first, and benchmarking corrects it where implicit GEMM is faster.
Not addressed here (rocKE, pre-existing): for a one-element cshuffle store, the C++ engine's shared
CShuffleEpiloguereads two elements from LDS where the Python engine reads one. No current byte-identity config reaches that path, and the packaged kernels are built with the Python engine. Fixing it touches an epilogue shared by every kernel family, so it is left for a separate change.Submission Checklist