Repository navigation
feat(hipdnn): report kernel ingestor runtime impact - #14227
Merged
GarbyGilbertAMD merged 16 commits intoNov 6, 2026
Merged
GarbyGilbertAMD merged 16 commits into
GarbyGilbertAMD merged 16 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #14227 +/- ##
===========================================
- Coverage 77.66% 77.66% -0.00%
===========================================
Files 2903 2903
Lines 490379 490379
Branches 74220 74220
===========================================
- Hits 380847 380846 -1
+ Misses 84579 84578 -1
- Partials 24953 24955 +2
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
GarbyGilbertAMD
marked this pull request as ready for review
October 23, 2026 19:53
Base automatically changed from
users/bharriso/hipdnn-deep-review-skill
to
develop
October 27, 2026 16:16
✅ 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. |
A change to the kernel ingestor can build cleanly and still break at run time. The ingestor C++ compiles only with the ingestor flag ON, and the contracts between a rocKE kernel and the C++ that launches it are kept by hand: the kernel's argument list is restated in native packs, test pins and descriptor signatures, and launch geometry, applicability rules and the shipped variant set have Python and C++ halves. A copy that falls out of step is caught on a device, in a flags-ON lane, or not at all, and two pull requests that each pass alone can break the target branch once both merge. The hipdnn-deep-review skill gains an IMPACT lane for these contracts. impact.py reads git objects only and reports, per contract, what changed on which side between the merge base and the head, and also in the head merged into the target branch's tip, so a copy that landed on the target after the merge base is compared too. It judges content rather than paths: comment, whitespace and type-preserving data edits are inert, removed or re-signed Python symbols are traced to their readers through imports and base classes, and a surface file it cannot understand is reported as changed, never as safe. impact_emit.py builds every shipped rocKE variant at both revisions on the CPU, regenerating DVC-held catalogs with the generator's registry enumerator, and compares each variant's kernel arguments and emitted LLVM IR, which separates a kernel edit that changes shipped code from one that does not. Each run ends in one line: BREAKS, NEEDS-DEVICE with the architectures, NONE or N/A. The lane brief maps these facts to findings, names the device runs that would settle what static analysis cannot, and hands CI coverage, build wiring and public API questions to the lanes that own them. The partition script scopes the lane's packet to the ingestor paths, the leak sweep accepts its finding IDs, and the skill's documentation describes the lane and the standalone command. ALMIOPEN-2834
impact.py could report a change as safe when it was not. A removed Python function lost every reader that still imported it as soon as any other file defined a function of the same name, which per-architecture builders do by design. A code edit to the generic-target header produced no row and rolled up as no surface file. A builder whose argument changed type kept its declared C++ list reported as agreeing, because the pair was compared by name only. And an emitter output file was folded into any report, so a stale file from an earlier head, one family for an unrelated source, or a family that built no variant could turn shipped-module rows inert. A removed symbol now keeps every reader bound to its old module. The generic-target header is a surface file in impact.py and a trigger in partition.py. Declared pairs compare each argument's size and kind as well as its name, resolving types held in module or function variables, and report when types could not be compared. impact_emit.py records the tree ids it archived, impact.py refuses an emit file made for other trees or with a malformed shape, and an all-equal emit folds only rows on modules a built family imports. A builder whose parameters change only in which are conditional, or a declared C++ list that shares too few names to compare, now gets a row instead of silence. A method removed from a base class and read through a subclass or an untyped receiver is reported as changed when the reading file's imports reach the class. The emitter's child runs with an allow-listed environment that passes no credentials, in its own process group so a timeout ends the whole tree, and with the newest LLVM flavor both revisions know. Two variants with one name but different specs are recorded as a failure rather than collapsed. C++ classification follows comment line splices and raw strings with encoding prefixes. A skipped merged view stays changed whenever the change touches the surface, and CMake and requirements files are labelled as left to the BUILD and DEPS lanes in the rows and the rollup instead of reading as no impact. The lane brief documents the emitter failure rule and the remaining blind spots of the symbol check, and the runbook counts the emitter's two workers against the core budget. ALMIOPEN-2834
The emitter compared shipped variants by their LLVM IR alone, so a change that reaches the code object without touching the IR folded every shipped-module row to inert. Dropping the COMGR optimisation level from -O3 to -O2 for every packed kernel read as no runtime impact. impact_emit.py now records each variant's COMGR inputs as well (the ISA name, the codegen options per kernel and the fresh-process compile flag), writes them as schema 3, and fails every variant when the packer pins a lowering backend other than Python. impact.py refuses older emit files, keeps a family with changed COMGR inputs from folding, and never folds rows on code that runs after lowering, rocke/helpers/compile.py and rocke/runtime/, because an equal IR digest says nothing about it. A changed DVC pointer no longer folds either. The emitter regenerates only the variant list of a DVC-held catalog, which cannot prove the file's other fields equal. Module reachability missed two paths. An import of a name from its package (from rocke.helpers import f) stopped at the package __init__, so a body edit of the module that defines f, which a shipped builder runs, read inert. The import now also reaches the defining module through the package's re-exports. And a module the packer reaches got only the architectures of the builders that happen to import it too, although the packer compiles every packed variant through it, so the device run it named missed the others. Such a module now ships on every architecture. Three paths were reported as runtime impact that are not. A rocKE C++ engine source is inert while no provider build file outside rocKE names the engine's targets and the packer pins the Python backend, since no shipped kernel is then built by the engine. CI test-category lists are left to the BUILD lane. And a merge conflict with the target tip no longer discards the whole merged view: the conflicted tree is analysed, rows that read a conflicted file are dropped, and one row stands in for the conflicted files, so a contract race in files that merged cleanly is still found. The lane brief documents each rule and the remaining gaps. ALMIOPEN-2834
The question the deep review's IMPACT lane answers, whether a change breaks the kernel ingestor at run time while the default build stays green, is also asked outside a review: on a single commit, a branch before it is opened as a pull request, uncommitted work, or before the ingestor flags are turned ON by default. Running a multi-agent review for that is out of proportion, and the scripts that collect the facts were reachable only from inside the deep-review skill. The new hipdnn-ingestor-impact skill owns impact.py and impact_emit.py and answers the question on its own. It pins the revisions for each input form, runs the static report and, when rocKE or hkp_pack Python changed in code, the emitter, judges each row, and reports the rollup with the device runs that would settle what static analysis cannot. Its reference holds what both uses share: the contracts, the row statuses and rollup, the emitter's rules, how each row is judged, and the known gaps. The deep review consumes the skill the same way it consumes the superbuild skills. The IMPACT lane calls the scripts from the sibling skill directory and keeps only what a review adds: where the evidence goes, when the lane stops, how a judged row becomes a finding, and its handoffs. impact.py no longer imports the review's helper library or reads its PIN.md; the lane passes the clone, base, head and target tip explicitly. The tests move with the scripts, the skill validator checks the new skill's scripts and command fields, and the skill catalogs list it. ALMIOPEN-2834
GarbyGilbertAMD
force-pushed
the
users/ggilbert/ALMIOPEN-2834/ingestor-impact-analysis-skill
branch
from
October 27, 2026 17:25
86c2f6a to
f019d5c
Compare
GarbyGilbertAMD
changed the title
GarbyGilbertAMD
marked this pull request as draft
October 27, 2026 21:18
Contributor
Author
|
Temporarily back to draft so it can absorb some updates (eg the multi-launch). Seems to break against integrations using the latest and greatest. |
… surfaces New ingestor engines read as BREAKS when nothing was broken. A launch_surface entry whose python_source names a method, such as a launch_block defined on a tuning spec class, was reported as pointing at a missing function, because impact.py looked the symbol up at module level only while launch_surface.py --check accepts any function of that name. A declared kernargs pair whose function is a SignatureBuilder factory, which has no b.param list, was reported as a missing builder. Every static list of a multi-launch pack was compared with the one declared builder, so the reduce kernel's argument list disagreed with the segment builder. A copy of an argument list that lacks a parameter the builder newly adds only conditionally was counted as stale, although every variant that leaves the condition off still passes that list. And the emitter lowered a builder returning a KernelModule as a single kernel, so every variant of such a family failed and its rows could not be settled. impact.py now resolves launch_surface symbols by name anywhere in the module and reports a function missing only when no function of that name exists. A declared kernargs pair compares each static C++ list with the b.param builder its names match, searched in the surface's module and the manifest's own source module with the declared function first, and names that builder in the row; a list that matches no builder is still compared with the closest one and reported as a mismatch, and a mirror that holds no KernelArgument list says so. A copy that still matches the new list without its conditional parameters is changed rather than mismatched. impact_emit.py lowers a KernelModule kernel by kernel, as the packer does, and records each companion launch's argument list; its output is schema 4. impact.py folds it by comparing each static C++ list with the launch it mirrors, so a companion's changed arguments are checked like the primary's. The reference documents the pairing rules and the remaining gaps. ALMIOPEN-2834
impact.py resolved a launch_surface python_source symbol through the def and class bodies of the module only, so a function defined under a module-level if, try or with passed launch_surface.py --check but read as missing, or went unseen when it changed. It now matches every function of that name anywhere in the module, as the generator's own check does, and compares all of them. The reference now states when a C++ argument list that matches no builder is compared at all (it must share at least two names, and half of its own, with one), and quotes the rows a list built by helper calls produces, including that a dynamic list gives no pair row when only the builder changes. New test cases cover a second function of the same name changing, an edit to the manifest's source builder alone (a body edit reads changed, an argument type change reads mismatch), and a pack that only the manifest's source builder can pair with an emitted companion launch, since the pack-name fallback cannot find it. ALMIOPEN-2834
…IOPEN-2834/ingestor-impact-analysis-skill # Conflicts: # projects/hipdnn/tools/ai/tests/test_deep_review_partition.py
GarbyGilbertAMD
marked this pull request as ready for review
October 28, 2026 21:44
…n impact The ingestor impact skill compared kernel argument lists and launch-surface declarations between rocKE and their C++ copies, but never what either side accepts. A support predicate could admit a spec its builder asserts on, a module's companion predicate could accept a head count its primary refuses, and a hand-restated C++ function could return a wrapped value for an input the Python refuses. While it builds each shipped variant, the emitter now records the specs the build passes to the module's other build functions. After the builds it puts each builder's support predicate, as the packer resolves it, to a table of one-field variations of the shipped specs. A spec the predicate accepts and the builder raises on is a builder finding. A value outside the sign range of every shipped spec that one predicate of a module accepts and another refuses is a sign finding. A finding the base revision's table also has is marked as pre-existing, and the emit schema becomes 5. impact.py lists the tables in their own section and prints a Pack-time line above the runtime verdict. The line counts the findings a change introduces and the older ones it ships for the first time, and stays apart from the runtime verdict: the tables never change a row or the rollup. The skill and the deep-review IMPACT lane gain a judge-written C++ differential table for header-only restatements, a C++ tables summary line beside the two verdicts, and a severity scale for every judged finding. ALMIOPEN-2834
The ingestor impact report could call a change NONE or N/A when it was not. Multi-launch UKDs, which list their launches under `sources`, were neither rooted in the shipped closure nor built by the emitter, so an edit to such a kernel read as reached by no shipped variant. A launch-surface grid or block helper was compared as its named def alone, so a change in the code it hands off to folded to inert on equal emitted IR. A flatc header counted as regenerated beside any change under the generator or the flatbuffers SDK, tests included, and plugin SDK headers that the descriptor loader compiles in were outside the surface. The closure now roots on every launch, and the emitter groups a multi-launch UKD's launches by source, builder, spec and target as the packer does, listing any it cannot read as not inspected; the emit schema becomes 6. A host-side surface function is compared together with every def, class and assignment it reaches by name. A flatc header is regenerated only beside its own schema or flag file, and SDK headers in the include closure of the ingestor SDK headers, with the generic-target table's CMake template, are on the surface. Device arch lists come from the UKD and KDP arch arrays with generic targets expanded, and a file outside the shipped closure widens the arch its path names to every arch a KDP named for it ships. With a target tip, the emit fold also applies to merged-view rows whose module content is the head's. A member read through a same-named class imported from another module is no longer a reader of the removed one. A global_buffer argument declared with a non-pointer size disagrees with a pointer parameter, a path-only python_source locator is accepted, a static list related to the declared builder is compared with it before a smaller builder it happens to fit, and flatc_flags.txt is read as build input. The registry enumerator stays in the emitter child's process group, so a timeout ends it too. ALMIOPEN-2834
A support-predicate probe calls the real builder on specs the predicate accepts, and a builder that never returns on such a spec held the emitter child until its timeout, losing the argument and IR comparison of every shipped variant. The probes now run in a worker thread that a family's budget bounds: a family past it is abandoned, its tables and those after it read as timed out, and the document is still written. The emitter builds its output document in one function that the report's loader is tested against, so the two schema numbers cannot drift apart. A b.param call under a with block, a try body or an if True branch always runs, so it is no longer marked conditional, and a copy missing it is a mismatch again. A static KernelArgument list with a comment inside a literal's braces is read with the comment blanked rather than taken for an argument. A launch-surface module that stops parsing is reported as such instead of as a missing function, and an emit family without both base and head params or companions is refused rather than crashing the fold. The deep-review partition triggers the IMPACT lane on every plugin SDK header and on the generic-target CMake template, which the report now treats as surface. The lane's emitter block is self-contained, the rollup format and the severity of a descriptor value the loader rejects agree with the reference, and the reference names the probe cut, the probe timeout, the emitter's environment variables and the git version the merged view needs. ALMIOPEN-2834
Brings in develop up to eca8c4d. The one conflict is the deep-review skill's front matter: this branch rewrote its description to name the IMPACT lane and develop added the prior argument to its argument hint, so the merge keeps both. ALMIOPEN-2834
…r impact The rocKE LLVM flavor ladder moved from lower_llvm.py into rocke/core/llvm_profile.py, which lower_llvm.py now imports it from, so the emitter could no longer find a default flavor on any tree that has the new module. It now reads the ladder from llvm_profile.py when the tree has it and from lower_llvm.py otherwise. The packer accepts hip and hsaco entries in a multi-launch UKD's sources map. Such a UKD has nothing for the emitter to build, so it is skipped like a single hip launch instead of being listed as not inspected, which would have kept every emit file from folding. FlyDSL keeps one DVC-held catalog per architecture in its own producer tree; only a catalog under the rocKE tree now counts as shipping the rocKE kernels of its architecture. ALMIOPEN-2834
…IOPEN-2834/ingestor-impact-analysis-skill
The emitter proves that the base and the base plus the change emit the same code. A merged-view row folded whenever its own file in the merge equalled the head's, which says nothing about a target that changed a module the change imports. Such a row now folds only when the target changed no descriptor and no module of the shipped closure since the base. A class a reader imports through its package's __init__ re-export now resolves to the module that defines it, so a member removed from that class while the reader still uses it is a mismatch again rather than a read of some other class. A launch-surface helper's reach also follows module aliases, so code it calls as m.f through import a.b as m or from a import b as m is compared with it. The plugin SDK include closure follows quoted includes beside the including header, and an emitter row for a generic-target family names the member architectures it was built for. ALMIOPEN-2834
A reader's class is resolved through the repository first, but when it does not resolve to a repository class the old rules still decide: a name imported from another module (an external package included), or a def or class of that name defined in the file, is not the defining module's class. Dropping those rules read a member call on such a name as a removed member still in use. The emitter lists a descriptor whose kernel source is not an object, or whose kind, source, builder or target is not a string, under not_inspected instead of failing its child process on an unhashable family key. New test cases cover the merged-view fold guard for a target that edits only a descriptor, the generic-target member fallback on every emit row kind, and a quoted SDK include resolved from the include root. The deep-review runbook, the skill's trigger text and the emitter docstring now match the code. ALMIOPEN-2834
When a reader's class does not resolve through the repository, only an import from outside the repository now proves it is another class. An import from a repository package whose export the resolver cannot follow (an alias such as `Space = waves.Space`, or a lazy export) may still be the defining module's class, so its read of a removed member stays a break instead of being dropped. The emitter lists a descriptor whose `name` is not a string under not_inspected, where it used to fail its child process on an unhashable key. A single-launch descriptor without a `name` is named by its `id`, as a multi-launch one already was. The launch string check lives in one helper, the docstring lists every non-rocKE packer kind, and new tests cover a missing or null `kernel_source` and the kpack and embedded source kinds. ALMIOPEN-2834
GarbyGilbertAMD
deleted the
users/ggilbert/ALMIOPEN-2834/ingestor-impact-analysis-skill
branch
November 6, 2026 21:04
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.
Summary
A kernel ingestor change can build cleanly and still break: the ingestor C++ compiles only with its flag ON, and a rocKE kernel's argument list, launch geometry and applicability rules are restated by hand in C++, test pins and descriptors. This adds the
hipdnn-ingestor-impactskill, which reports those contracts and the kernel support predicates for any PR, branch, commit range or uncommitted change, and anIMPACTlane inhipdnn-deep-reviewthat runs the same scripts and turns their results into findings.JIRA ID : ALMIOPEN-2834
Risk Assessment
Risk 2 of 5. Review tooling only; no product code, build or CI file changes.
impact_emit.pyruns rocKE code from the analysed revisions, the same exposure as building them, in a child process with an allow-listed environment that drops credential variables (it keepsHOMEand network access) and a timeout that ends its whole process tree.ASIC Coverage
No ASIC impact: the scripts read git objects and run rocKE on the CPU only. Passing PR CI is sufficient.
Testing Summary
Unit tests on synthetic git repositories (no network, no rocKE) cover argument lists against every copy, multi-launch packs and multi-launch (
sources) UKDs, launch-surface symbol lookup (tied tolaunch_surface.py) and the code a host-side surface function reaches, arch lists from the shipped descriptors, SDK headers the loader includes, generated headers and their schemas, the merged-tree view and conflicts, removed and moved symbols, generic targets, emit-file guards, the emitter's environment, timeout and hanging probes, the predicate tables and thePack-timeline.Acceptance on develop changes with known outcomes and on two review revisions of feat(hip-kernel-provider): add gfx950 unified attention 2D/3D ingestor engines #14242, at this head:
BREAKS:build_attention_densegained three arguments its literal test pin did notBREAKSin the merged view onlyNEEDS-DEVICE all-ingestor-archs, gfx950NONE: arguments and IR equal for all 880 shipped variants;Pack-time: CLEANNONENONEN/ANEEDS-DEVICE all-ingestor-archs, gfx950, nomismatch;Pack-time: FIRST-SHIPPED 1: an older 2D predicate accepts a tile size its builder raises onPack-time: NEW 1, FIRST-SHIPPED 1: the reduce predicate accepts a non-positivenum_query_heads, as review foundNo CI job runs
projects/hipdnn/tools/ai/testsorvalidate-skills.py; both were run locally on Windows. The emitter's process-group tests, which skip on Windows, were run on Linux (WSL2) and pass.Testing Checklist
python -m pytest projects/hipdnn/tools/ai/tests- Status: Passed (551 passed, 10 skipped, one commit before this head; at this headtest_ingestor_impact_emit.pypassed in full and the changedtest_ingestor_impact.pycases passed)python projects/hipdnn/tools/ai/validate-skills.py- Status: Passed (17 skills)impact_emit.py runandimpact.py report --repo . --base <rev> --head <rev> [--target-tip <rev>] [--emit <json>]on the ranges above - Status: PassedTechnical Changes
hipdnn-ingestor-impact/scripts/impact.py: reads git objects only and compares the merge base, the head and the head merged into the target tip. Each row ismismatch,changed,inertoroutside, and the report ends in aPack-time:line andRuntime impact: BREAKS | NEEDS-DEVICE <archs> | NONE | N/A. Launch-surface symbols resolve aslaunch_surface.py --checkresolves them, a host-side surface function is compared with the code it reaches, and each static C++ argument list is compared with the builder it matches, the declared one first. Device arch lists come from the shipped descriptors'archarrays, and plugin SDK headers in the loader's include closure are on the surface.hipdnn-ingestor-impact/scripts/impact_emit.py: builds every shipped rocKE variant at both revisions on the CPU, multi-launch UKDs included, and compares arguments, IR digests and COMGR inputs, kernel by kernel for aKernelModule. It also puts each builder's support predicate, as the packer resolves it, to one-field variations of the shipped specs: an accepted spec the builder raises on, or a non-positive value one predicate of a module accepts and another refuses, is a pack-time finding, marked when the base already had it. A probe that never returns costs only its tables.hipdnn-ingestor-impact/SKILL.md,reference.md: the workflow, report format and severities; the contracts, row meanings, emitter and predicate-table rules, known gaps, and a judge-written C++ differential table for header-only restatements.hipdnn-deep-review: theIMPACTlane maps judged rows, predicate-table findings and C++ table disagreements to findings and names the device runs that would settlechangedrows; the runbook re-runs it when the target tip moves,partition.pytriggers it on the ingestor paths and plugin SDK headers, and the leak sweep carries its IDs.docs/ai-rules.mdand the rootAGENTS.mdregister the new skill.