feat(hipdnn): load UEDs from disk into the real descriptor types by SamuelReeder · Pull Request #10679 · ROCm/rocm-libraries

SamuelReeder · 2026-08-12T14:47:25Z

Summary

hipdnn_plugin_sdk::ingestor already models RFC 0017's descriptor set as parsed in-memory data, but nothing produced those descriptors from disk — every pack was hand-built in C++ and registered as a compile-time engine-table entry. This adds a header-only descriptor loader to plugin_sdk that reads per-descriptor JSON from an arch_content tree, resolves it by GUID into per-engine DescriptorSets, and builds GenericEngine instances from that data.

It then fills the seam the base branch left for it: discoverDescriptorSets()'s body becomes the directory scan, and both of the provider's engines are defined entirely by installed files. PointwiseAddDescriptors.cpp and PointwiseSubDescriptors.cpp are deleted and the pack table loses its descriptor column, so adding an engine no longer edits any C++ unless it needs a kernel. A file's type is its filename suffix (pointwise_add.kdp.json), so the loader dispatches through a six-entry table instead of a six-branch if/else and never opens a .json that names no type; there is no in-file schema member: the name already states the type, and a second spelling of it is one more thing to disagree with. A file still carrying the key is rejected as an unknown key. The two engines are also re-cut along a real boundary: hipkernel:PointwiseSub folds into hipkernel:Pointwise as a third pack, and a new hipkernel:ConvFwd takes its place, so the split is by graph node type rather than by operation. With HIPDNN_ENABLE_KERNEL_INGESTOR=ON the engines in hipdnn_list_engines come from files, and deleting a descriptor file deletes what it described. The UED also follows RFC 0020.

Descriptors are read from the provider's installed tree and then, when HIPDNN_DESCRIPTOR_RUNTIME_DIR names a directory, from a runtime drop-in tree after it. Both feed one catalog, so the rules that resolve two files claiming one id are the ones already written and tested rather than a new merge policy: identical content collapses, content that disagrees drops both. A drop-in can add an engine beside the shipped ones; it cannot quietly redefine one. HIPDNN_DESCRIPTOR_DIR keeps its existing meaning and replaces the installed tree, which is what tests and build-directory runs want.

JIRA ID : ALMIOPEN-2401

Risk Assessment

Medium risk. Everything new sits behind the default-OFF HIPDNN_ENABLE_KERNEL_INGESTOR flag and no shipping default changes, but with the flag on this moves an engine's provenance from the compile-time table to disk, so provider construction now depends on install layout and on file content that is not compiler-checked. The two failure modes that would matter — an engine advertised by copyEngineIds that the constructor then fails to build, and a descriptor set drifting from the hand-written pack — are both covered by direct regression tests, and load failures are logged and dropped rather than propagated. The RFC 0020 conformance pass renames the descriptor-backed engine and changes the on-disk key names, and this change makes version required on all six types and moves the KDP to the RFC key spellings (matchers, engine, dispatch, kernelDescriptors, kernel_source), so any descriptor authored against an earlier revision of this branch must be renamed to <name>.<type>.json, carry a version, and use those spellings. That is a clean cutover with no grace period: all nineteen shipped descriptors are migrated here, and nothing outside this branch authors them yet.

ASIC Coverage

Standard PR CI is sufficient. The change is host-side descriptor plumbing: it alters where an existing engine's descriptor set comes from, not what the engine supports or how kernels are selected, and the shipped descriptors carry no architecture-scoped content (KernelDescriptorPack's arch field is parsed, validated and enforced, but every shipped pack declares none, and the kernels are runtime-compiled HIP source that was already dispatched this way). The end-to-end path was exercised on gfx90a; no arch-specific code path is added or modified, so no multi-arch sweep is required.

Testing Summary

Loader unit tests covering parse, GUID resolution, duplicate collapsing, symbol validation, the state-manager probe, undeclared knobs, engine-id collision, sdk_version, an engine that ships no UHD, and reload idempotency.
Provider unit tests asserting copyEngineIds equals what EngineManager constructs, and pack-shape tests that read the loaded sets rather than a hand-written twin, so they fail if the descriptors stop being installed.
GPU integration covering both engines and all three packs of the multi-pack engine, checked against the CPU reference executor so a mul served by the add kernel would fail numerically. The conv case is held to an 18-term (C*R*S) accumulation tolerance rather than the bit-exactness a one-element pointwise op meets, since GPU and CPU sum in different orders.
Manual descriptor-directory checks: malformed files, duplicate subtrees, and deleting a UED or a pack file to confirm the engine or pack disappears.
Full build and unit run with the feature flag off, confirming nothing is compiled or staged.

Testing Checklist

Technical Changes

Adds hipdnn_plugin_sdk/ingestor/DescriptorLoader.hpp: a header-only loader that walks a descriptor tree, parses each file into the existing Descriptors.hpp types by its filename suffix, resolves GUID references into DescriptorSets, and exposes loadValidatedDescriptorSets<THandle>() ; engine construction uses the base branch's makeEngine().
Makes loading total: a malformed, unresolvable or symbol-missing descriptor is logged and costs only its own pack or engine. Filesystem, JSON and construction failures are all contained, so a bad file on disk cannot fail plugin creation.
Validates before advertising. A set is returned only if every native symbol it names is registered, its name claims an engine id no already-registered engine holds, a probe KernelIngestorStateManager constructs, and no knob names a field the metadata schema does not declare — the last two are what Container::copyEngineIds would otherwise advertise and then fail to construct. The knob rule is now one shared findUndeclaredKnob() used by both the loader and GenericEngine's constructor.
Cuts hip-kernel-provider over at the seam the base left: discoverDescriptorSets()'s body is now the scan of descriptorSearchDirectory(), which tries HIPDNN_DESCRIPTOR_DIR, then the directory this plugin was actually loaded from, then the configure-time install prefix — so a DESTDIR-staged or relocated install finds its own descriptors instead of advertising zero engines, and the test targets pass the staged build copy through ctest, and Container's definitions loop is unchanged. Both *Descriptors.cpp files are deleted, IngestorPack drops buildDescriptorSet, and failedPackLabels() goes with it — a pack whose symbols failed to register is already excluded by the loader's pre-flight check, so tracking labels was a second answer to one question.
Registers native symbols before the scan, inverting the old ordering: validation asks the registry whether each descriptor's symbol exists, which is what lets copyEngineIds advertise only ids whose engines can actually be built.
Returns the memoized set vector by reference instead of copying two DescriptorSets per call. A test helper holding a reference into the returned temporary is what surfaced it.
Ships both engines as nineteen JSON files staged into the build tree and installed under arch_content/hip-kernel-provider/descriptors, grouped one folder per engine (pointwise/, conv_fwd/). Folders are organizational only; the loader walks the tree and a file's directory means nothing to it. No architecture level in the shipped set: every pack here applies on every device, so a per-arch shard would only be a second identical copy. KernelDescriptorPack's arch field is parsed and carried, so a pack can name its GFX targets and KernelIngestorStateManager prunes it on a device those exclude; the shipped packs simply declare none. Entries are validated as bare gfx ids, non-empty and duplicate-free: arch is the one field whose typo is otherwise invisible, since "GFX942" would parse cleanly and then decline on every device, logging exactly what a healthy cross-arch install logs.
Points the pack tests at the loaded sets through a loadedSet(engineName) helper rather than keeping a hand-written twin to compare against. Once the JSON is the only source of truth there is nothing to drift from, and a fixture would just be a second place to edit; a test that reads what the provider reads is what fails if the descriptors stop being installed.
Restricts the authored format to what Descriptors.hpp models. RFC 0017 fields with no parsed representation are rejected as unknown keys rather than silently ignored, so adding a field stays a change in the structs and the loader and nowhere else.
Restores NativeRegistry::isRegistered(), removed on the base branch as unused: the loader's pre-flight symbol check is the use, and asking resolve() and catching would make an expected authoring mistake cost an exception.
Conforms the UED to RFC 0020, which is the source of truth for that descriptor: the UED's keys are heuristic and metadata; numerical_notes is parsed and carried as authored, since no hipDNN numerical-note vocabulary exists to map onto; and name must be a scoped namespace:local identifier, because it is hashed into the global engine-id space.
Gates every descriptor on its declared version (RFC 0017 §4, RFC 0020 §11.1): same major, no newer minor, checked in one place ahead of the catalog insert — which is what puts it ahead of duplicate detection, as §10.2.1 requires. Required on all six types, with no absence-safe default: a type carrying no version cannot be gated by the §11.1 accept rule at all, so a KMD that gained a field at 1.1 would be read by a 1.0 runtime with no way to refuse it. Descriptors also parse as JSONC (§4.3).
EngineManager::addEngine now logs a duplicate engine id instead of letting the map discard it silently. This is a backstop for collisions the descriptor rule cannot see — two hand-written engines, or a descriptor engine against a built-in — not a competing policy: §10.2.1's drop-all runs in the loader, before construction.
Parses the UED's sdk_version onto EngineDescriptor, added on the base as the graph-schema gate. Absent leaves the baseline so a UED predating the field loads unchanged; a malformed one drops the engine rather than silently reading as baseline. The loader only carries it — gating is match-time, since the floor it compares against is a property of each graph.
Drops every UED in an engine-name collision rather than keeping the first (§10.2.1) — load order is filesystem order, so keep-the-first left the surviving definition up to the directory walk — and adds HIPDNN_DISABLE_ENGINES (§12), which skips an engine before it claims its name, matching by name, engine-id hash or UED UUID. That is the recovery lever the drop-all rule needs.
Folds the base branch's separate hipkernel:PointwiseSub engine into hipkernel:Pointwise and adds a mul pack beside it: three packs under one engine id, sharing descriptors by reference. Sub is a PointwiseAttributes node like add and mul, so a separate engine split on the operation — which is what packs are for. PointwiseSubNative.cpp (476 lines, a near-duplicate of PointwiseNative.cpp) and five sub-only descriptors are deleted; the sub kernel and its pack survive.
Splits the graph matcher for it, rather than parameterizing one matcher per operation. Everything that makes a graph servable at all — one node, pointwise, binary, one element, real buffers, uniform dtype — is a single matcher both packs list, so KernelIngestorStateManager evaluates it once per graph and memoizes the verdict across packs; what differs is one enum compare each pack lists as a second graph-scoped matcher. Three of the four matchers and the whole dispatch are one descriptor referenced three times. Nothing exercised that memo, or the loader's matcher dedup, before this: two engines each get their own state manager.
Adds an operation field to that engine's KMD, because the catalog key is per engine and two packs both claiming (block_size=64, dtype=FLOAT) is a duplicate metadata tuple the state manager rejects at construction.
Adds a second engine, hipkernel:ConvFwd, from a naive direct-convolution reference kernel, so the provider's two engines are separated by graph node type (ConvolutionFwdAttributes vs PointwiseAttributes) rather than by operation. That is a real engine boundary; the operation split the base had was not. Deliberately narrow — stride 1, dilation 1, no padding, cross-correlation, packed NCHW/KCRS, FLOAT or HALF — and the matcher refuses everything outside it rather than computing a wrong answer.

Deliberate deviations and deferrals

There is no schema member, where RFC 0020 §4.2 requires one with the exact value hipdnn.ued/v1. The filename suffix carries that information, so the member would be a second source of truth for a fact the name already states — and a file whose name and body disagree has no correct reading. Typing the file from outside is also what lets a .json naming no descriptor type be skipped before it is opened, which an in-file member cannot do. A file still carrying the key is rejected rather than ignored.
version is required on all six types, where §4.2 mandates it for the UED and no merged RFC yet covers the other five. Stricter than the letter of the spec, and deliberate: RFC 0017 §4 versions every file type independently, and a type carrying no version cannot be gated by the §11.1 accept rule at all. The supported version is per type, one pair per row of the file-type table, so the first type to reach 1.1 cannot widen the gate for the other five.
sdk_version sits on the UED, where RFC 0017 §4 puts the graph-schema version on the UMD. It is modelled on EngineDescriptor in the base branch and read per-engine by GenericPlanBuilder, so moving it would desync the loader from the structs it fills. Flagged upstream rather than papered over here.
A KMD field accepts only name, type and default_value, so RFC 0017 §4's own example field ({"name": "tile_m", "type": "int", "optional": true, "default": 1}) is rejected on two keys. MetadataField has no members for them; accepting the keys without the storage would be a parser that silently discards what it read.
Not implemented: the JSON-Schema file artifact and build-time minimum-version lint (RFC 0020 §11.3, §11.4), which need a Draft 7 validator dependency and a CI parity check; and the lazy half of RFC 0017 §8.1's two-phase load ("the heuristic is named but not loaded"), since eager resolution is a superset of the required ordering — it is what lets the static copyEngineIds advertise only ids that will actually construct — and RFC 0020 §8 puts the loading mechanism out of scope.
Not a deviation, for the record: content-identical duplicates under one id collapse rather than dropping all, which is exactly what RFC 0020 §10.2.1's content-identical exception requires. The comparison is of parsed documents, not bytes, so comments, whitespace and key order do not turn two copies of one descriptor into a collision. Differing content under one id still drops both, as specified.

therock-pr-bot · 2026-08-12T20:41:55Z

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🔎 pre-commit	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—
🤖 therock-pr-bot	✅ Pass	—

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

therock-pr-bot · 2026-08-13T01:44:21Z

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

python -m pip install pre-commit
pre-commit install
pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

…criptor Removes the `schema` member from the six descriptor types and from all nineteen shipped files. A file's type is its filename suffix and nothing else. An earlier round added `schema` back for RFC 0020 4.2 conformance and cross-checked it against the suffix. That is two spellings of one fact, and a file whose name and body disagree has no correct reading -- the cross-check only decides which of the two to believe. Naming the type outside the file is also what lets a `.json` naming no descriptor type be skipped before it is opened, which the in-file member cannot do. Now a deliberate, documented divergence from 4.2 rather than a conformance fix: the header's deviation list says so, and a file still carrying the key is rejected as an unknown key rather than quietly ignored. Also folds in the rest of the review round: - descriptorSearchDirectory() takes the env override only when it names a real directory. The install-tree CTestTestfile.cmake is generated from the same ENVIRONMENT list as the build-tree one, so it ships this build's absolute staging path; on a test machine that path is absent, the override won unconditionally, and the installed suite loaded zero descriptors. - The module-relative branch keys on the address of a function in this module rather than on a symbol name, via a new getLoadedLibraryDirectoryForAddress on both platforms. dlsym(RTLD_DEFAULT) resolves from the caller's scope on glibc, so the name form did work, but an address cannot be ambiguous under any dlopen flag and needs nothing exported -- and it drops the Linux-only guard. - isPlausibleArchBaseId admits what archMatches admits: a candidate carrying its own feature suffix (`gfx942:sramecc+`) and LLVM generic targets (`gfx9-4-generic`) were unauthorable while the matcher handled them. - The version gate runs before the schema-shape checks, so a file from a newer toolchain reports as version skew rather than blaming another field. - The walk's increment retry is bounded; recovery assumed the increment still advanced, and an implementation that errors without advancing would spin. - Both test suites now depend on the descriptor staging target, which was reachable only through ALL, so building a suite by name on a fresh tree ran against a directory that did not exist yet. - Descriptors move under engines/kernel_ingestor_engine/, beside the engine that owns them; the staged and installed layout is unchanged. Tests: kernel-source kinds with no adapter, the dispatch and score pre-flights and the kernel-scope matcher arm (asserted by diagnostic, since the probe drops the engine either way and a survive/drop assertion cannot see the difference), conflicted non-UED descriptors, both remaining coerceKernelMetadata arms, the orphan-pack and duplicate-id diagnostics, conv dispatch/workspace/dtype, behavior notes, per-pack operation metadata, and descriptorSearchDirectory's three branches. SDK 321, provider 523 + 1 pre-existing skip, GPU 14/14 on gfx90a.

dladdr and GetModuleFileNameW report the name a module was *loaded with*, not where it is. getLoadedLibraryDirectoryForAddress hands that name out as a base for callers to resolve sibling paths against, and two spellings break that: dlopen("./sub/lib.so") -> dli_fname "./sub/lib.so" dlopen through a symlinked .so -> dli_fname is the link, not the file The first makes every derived path follow the process's current directory rather than the module -- and hip-kernel-provider's descriptorSearchDirectory() runs lazily on first use, so a chdir between load and that call silently changes the answer. The second measures from the link's directory, while the files a caller is looking for sit beside the real one. Both end the same way for the descriptor tree: is_directory() fails, the module-relative branch falls through to the configure-time prefix, and the provider advertises zero descriptor-backed engines with one INFO line. weakly_canonical, not canonical: the weak form tolerates a non-existent tail, so a path quirk cannot turn into a failure. error_code overload, falling back to the raw path, because this lookup is best-effort and has a fallback behind it. Verified against a real install reached through a cross-directory symlink, with no descriptors beside the link: resolves to the real tree and loads both engines. Same-directory version symlinks (libfoo.so -> libfoo.so.1) were already correct and are unaffected. SDK 321, provider 523 + 1 pre-existing skip, GPU 14/14 on gfx90a.

Many rounds of design review left multi-paragraph justifications on nearly every non-obvious line. Collapsed each to its single load-bearing point: why a decision was made, not every angle it was considered from. No code, signature, or logic changed -- comment text only. Notable consolidations: - DescriptorLoader.hpp's @file doc: 78 lines -> 42, cutting the exhaustive recitation of every optional field's default (already visible in Descriptors.hpp) while keeping the format table and both RFC deviations. Relocated the KMD `fields`-shape deviation note to parseMetadataSchema(), where the enforcing code actually lives, instead of the top-of-file doc. - Its name-collision-check comment: 23 lines / 4 paragraphs -> 13 lines / 2, keeping all four facts (drop-and-stand semantics, why it's rarely reachable, the cross-plugin blind spot, what catches it in-process). - Removed two dangling "D1"/"D3" deviation-number references left behind after the doc block that defined them was rewritten. - PointwiseNative.cpp/ConvNative.cpp @file docs and dispatch-handler class docs: 2-3 paragraphs each -> one. - EngineManager.hpp: deleted five Doxygen blocks that only restated their method's name/signature. - PlatformUtils.{linux,windows}.hpp: tightened the address-vs-symbol-form and weakly_canonical rationale added for the module-resolution fix, without dropping either fact. Left alone: comments already at one crisp non-obvious fact per line (the kernel .cpp headers, NativeRegistry.hpp, most refusal-case test comments). No behavior change: SDK 321, provider 523 + 1 pre-existing skip, GPU 14/14, identical to before the pass.

…iles A KDP's `kernelDescriptors` entry may now be a bare UUID string naming a standalone `.ukd.json`, alongside the inline object form. Both are supported and one pack may mix them; RFC 0017 §13.1 describes shipping a kernel separately from the pack that binds it, which an inline-only format cannot express. Loader: - `.ukd.json` is a seventh file type parsed into the existing KernelDescriptor struct and catalogued in DescriptorCatalog::kernels. The kernel parse splits into a shared body plus two allow-lists, because the forms differ by exactly one key: a file carries `version`, an inline entry has no file to version. - References resolve in resolveDescriptorSets, above the KMD coercion and the "declares no kernels" check, so a referenced kernel is validated exactly like an inline one and an all-references pack is not read as empty. - Resolved kernels are appended rather than interleaved. rank() orders by (score, priority, id), so vector position never decides selection. - A dangling reference drops the pack, matching the matcher and dispatch arms. - One id appearing twice in a pack (referenced twice, or referenced and also inline) is rejected: it would otherwise reach the state manager as two identical kernels, collide on the completed metadata key, and throw, costing the whole engine instead of the one malformed pack. - A `.ukd.json` no pack references warns -- reachable only through a reference list, so it would otherwise load and do nothing silently. - Factored the matchers UUID-parse into requireUuidEntry(), now shared with the bare-id kernel form. Shipped descriptors: conv_fwd's f16 kernel moves to conv_fwd_f16_block64.ukd.json and is referenced by id, leaving two inline. The pack demonstrates both forms and the GPU conv suite exercises the reference path for real. Verified: SDK 332 (+11), provider 523 + 1 pre-existing skip, GPU 14/14. hipdnn_list_engines reports 4 engines with the referenced kernel loaded; breaking the reference drops it to 3 with the dangling-reference error, the engine's "no loadable kernel pack" error, and the orphan warning. Mutating the splice to discard resolved kernels fails 4 of the new tests, so they are not vacuous.

The base now lets an engine ship no UHD and rank by declared order, so the loader stops requiring `heuristic` on a UED: absent means declared order, while naming one no file defines is still a broken install and drops the engine. Both halves are tested, and the ABI check confirms the engine count holds at four with the key removed. Review fixes: - Drops makeStateManager() and makeDescriptorEngine(), which duplicated MakeEngine.hpp. The base gave its copy a defaulted second parameter, so the two were ambiguous, not merely redundant, for anyone including both installed headers. makeDescriptorEngine() never had a caller. - Replaces mkdtemp with std::filesystem in the loader tests. MSVC ships no <unistd.h>, so the whole suite failed to compile on Windows. ScopedDirectory already creates atomically and throws on a taken name, so the helper only owes it a distinct one. - IgnoresAFileWhoseWholeNameIsASuffix now asserts the skip WARN. Widening the stem guard left it passing, because the sibling engine loads either way. - PluginDevelopment.md said a pack is one .cpp file fifty lines before saying one file can serve several. The shipped code matches the latter.

…once on no device Two review points from Brian. Runtime loading. The loader now walks several roots into one catalog, and the provider passes its own tree followed by HIPDNN_DESCRIPTOR_RUNTIME_DIR when that names a directory. HIPDNN_DESCRIPTOR_DIR keeps its meaning and replaces the provider's tree, which is what tests and build-directory runs want; the new variable is additive, which is what a drop-in wants. One catalog rather than a merge of several, so the rules that resolve two files claiming one id are the ones already written and tested: identical content collapses, content that disagrees drops both. A drop-in can therefore add an engine beside the shipped ones but cannot quietly redefine one. Verified through hipdnn_list_engines: a tree holding only pointwise reports 3 engines, 4 once a drop-in supplies conv_fwd, and 2 when the drop-in tries to claim a shipped id. No device, one bail. Both graph matchers opened by returning false when the context named no device, and any pack added later would have had to remember to. KernelIngestorStateManager::catalogFor answers it once instead, before the cache lookup and before any matcher runs: nothing below that point can be decided without a device, since pack pruning reads its arch, matchers read its properties, and a kernel that somehow matched could not be launched. An empty catalog is what those matchers were producing anyway. Each change is pinned by tests that fail when it is reverted: walking only the first root fails three loader tests, dropping the device bail fails the state manager test, and dropping the append fails the provider test.

…the doc addition Two self-review comments. The comment on the name-collision check claimed EnginePluginManager::validateBeforeAdding does no id dedup across plugins. Wrong: it keeps a set of every id already loaded and rejects a later plugin outright on overlap (EnginePluginManager.hpp:81-90), which loadPluginFromFile's tryCatch turns into a load failure for that plugin. The corrected comment says what actually happens: this loader's local registry can't see another plugin's names because each is a private, hidden-visibility copy, but the backend does catch the collision -- more harshly, since the whole plugin fails rather than just the one colliding engine. Dropped the "Adding an Engine with the Kernel Ingestor" section from PluginDevelopment.md. Purely additive against develop (140 insertions, 0 deletions), so removing it loses nothing that predates this PR; author's call to write that documentation separately.

…n on unknown keys A per-arch descriptor shard ships one KDP id per arch, each stamped with that shard's arch and built against that shard's kernels. Keyed by id alone the loader saw "same id, different contents", marked the entry conflicted, and dropped both, so the layout could not load at all. Packs and standalone kernels are now keyed by (id, sorted arch); the other five types stay keyed by id and still collapse byte-identical copies. A pack resolves a referenced kernel within its own arch, falling back to an arch-independent one, and a kernel defined only under some other arch now says so rather than claiming nothing defines it. State-manager tuple uniqueness narrows from engine-wide to per overlapping-arch group, since two shards' kernels complete to the same tuple and no single device sees both; overlap is a symmetric prefix test, so gfx942 and gfx942:sramecc+ still collide. An unknown JSON key is now a WARN naming the file rather than a rejection, so a descriptor may carry provenance or tracking fields. A missing required key stays fatal, and every type, pattern, enum, and value check on a known key is unchanged. Diagnostics carry the full path instead of the bare suffix, which a sharded tree makes ambiguous.

An unknown JSON key is a hard error again, except one that announces itself as extension data: a `x-`/`_` prefix, plus `provenance`, the block the descriptor packager stamps. Warning about the rest was silent damage -- a UED spelling `heuristik` has no heuristic key, absence is legal, so the engine loaded and ranked by the fallback forever, and the default log level is off. Nested unknown-key and field diagnostics now carry the file too; a bare "a 'kernelDescriptors' entry" names nothing in a tree of shards. A runtime drop-in root can no longer delete an installed engine. Roots are read one at a time and each root's entries are settled before the next, so a later root that redefines a settled id is refused and the installed definition stands. Within one root the RFC 0020 §10.2.1 drop-all rule is unchanged: two files there have no defensible order, two roots do. Also: the unreferenced-kernel scan drops a sort keyed on an id that now repeats across shards, DeclaredOrderKernelHeuristic becomes UnrankedKernelHeuristic because the tie-break it falls through to is priority then UUID bytes and not authoring order, descriptorSearchDirectory() documents the module-relative step it actually has, and buildPlan() declares its ordering dependency on initializeExecutionSettings(). Tests: drop-in precedence both ways and through the validated multi-root overload; extension keys versus a misspelled optional key; archOverlaps' empty arm; exact-arch versus arch-independent kernel precedence; and the two arch tests that asserted only "did not throw" or "loaded nothing" now assert which kernel a device sees and which key collided.

The build-time packager emits `"kind": "kpack"` with the archive coordinates beside it (library/toc_key/symbol/sha256) and a `provenance` block on the kernel entry. The loader spelled the kind `kpack_symbol`, and the four coordinate keys were unknown -- and since the key check runs before the kind switch, a packaged descriptor failed with "unknown key 'library'", sending a reader to fix the packager's vocabulary instead of waiting for the adapter that is actually missing. Adopt the producer's spelling and name the four keys without modelling them: KernelSource grows no fields until something can dispatch a kpack kernel, so the descriptor still fails, now naming the missing adapter. `provenance` already loaded as an extension key; the new test pins it at the kernel entry, which is where the packager puts it, rather than only on the UED.

The field comment read "an inline kernel takes its pack's arch", which sounds like inheritance the parser performs. Nothing assigns it: only parseStandaloneKernelDescriptor sets arch, an inline kernel's stays empty, and KernelDefinition has no arch at all. State the split the field actually encodes -- identity is per file, applicability is per pack.

…h shards Every per-arch loader test stops at resolveDescriptorSets, and every tuple uniqueness test builds its packs by hand, so the two halves of the change were never exercised in composition -- which is the path the provider takes. Two shards of one pack complete to identical tuples, so scoping uniqueness by arch overlap is what keeps them; without it the probe in loadValidatedDescriptorSets throws and the catch drops the whole engine. Verified by reverting the scoping: this is the only one of the thirteen arch tests that fails.

Identity is the id; arch is in the catalog key only because per-arch shards ship one id more than once. Resolution was reading it as identity anyway -- an exact-key lookup on (id, pack arch) with an empty-arch fallback -- so a kernel narrower than its pack missed both keys and dropped the pack, and the diagnostic blamed a missing shard. findKernelForPack now reads the id's whole range and decides by arch. One definition is the ordinary case and reduces to a check: within the pack's arch resolves, outside it is an authoring error named as its own failure, disjoint keeps the missing-shard wording, and two definitions both in reach are ambiguous rather than silently ranked. Inline kernels take `arch` too, checked while parsing since their pack is already in hand. Narrowing has to reach dispatch or the key would validate and not filter, so KernelDefinition carries the resolved list, buildCatalog gates each kernel on it, and tuple uniqueness claims by kernel rather than pack -- which is what lets one pack hold an implementation per capability, an MFMA build beside a portable one, on a tuple they share. Verified on gfx90a: kernels narrowed to gfx942 inside a pack claiming both load with no complaint and decline at match time; narrowed to gfx90a they execute. Mutation-checked, including the coverage direction, which bare gfx ids cannot distinguish -- only a feature suffix can.

…erently loadValidatedDescriptorSets<LoaderHandle>({installed, dropIn}) is ambiguous under clang-cl / MSVC STL: its std::filesystem::path ships an unconstrained InputIt-pair constructor, so {installed, dropIn} is a viable exact-type match for path's own two-iterator overload as well as vector<path>'s initializer_list one, and nothing ranks between them. libstdc++/libc++ SFINAE that constructor away for non-iterator types and never saw the conflict. Building the vector explicitly removes the choice instead of relying on the brace-list to resolve it.

…ffix isPlausibleArchBaseId parsed ':feature' groups and accepted them, but archMatches compares target-id text: a candidate must be a literal prefix of the device string, terminated by ':' or end. So "gfx942:xnack-" -- the spelling ROCm's own supported-target lists use, and a legal partial target ID to the toolchain -- matched no device at all, because a device reports "gfx942:sramecc+:xnack-" and names sramecc first. Validation accepted a value matching could never honor, and the pack vanished with nothing louder than an INFO decline. Reject the suffix at parse instead, and say what to write. The base id is all the loader has a vocabulary for; feature-level gating belongs in a matcher expression, which can read the device properties and say what it means. Generic targets (gfx9-4-generic) stay legal: '-' is part of the base id. Two tests changed because their premise is now unauthorable. Both used a suffix only to build arch lists that overlap without being equal -- the shape plain string equality would miss -- so they move to [gfx942] against [gfx942, gfx950], which is reachable and pins the same thing. That reordering also closes review item I7: the narrower list now comes first, so mutating archOverlaps to archCovers fails RejectsTwoPacksSharingATupleUnderOverlapping- Arch instead of surviving every tuple test. Also corrects KernelDescriptor::arch's contract comment, false in four clauses since per-kernel arch became a device filter (review I5): the field is populated inline as well as standalone, it is applicability and not only identity, and KernelDefinition does carry arch. Records the base-id rule beside the other format divergences (review I8). Mutation-checked: allowing ':' and '+' back into the accepted set fails exactly the two new tests.

BrianHarrisonAMD

LGTM!

Lets fix the cmake issues before merging, but the rest is looking good to me.

…hared lists Five review comments on the provider's CMake, four of them real. The installed CTest file carried this machine's build directory. Not the plain install path -- install_provider_ctest_files() writes no environment -- but the YAML-categorized one, where a single --environment list feeds the build-tree and install-tree files alike, so there is no seam to give them different values. On any other host that path names nothing and the suite loads no descriptors; on this one the installed copy is never the copy read. Rather than a second, install-shaped path, neither suite is told anything. The integration binary loads the real plugin, and the engine already resolves its tree from the loaded module -- the staged copy in a build tree, the installed copy in an install tree. That was always sufficient there and the environment entry only overrode it; the suite passes 14/14 with the variable unset. The unit binary links the engine statically, so there is no module to measure from, and it now compiles the staged path in and applies it from main() when nothing else set one and the directory exists. That makes the binary runnable on its own -- it previously failed 19 tests outside ctest, which read as a regression twice during development -- and keeps every generated CTest file free of machine-specific paths. Descriptor staging was a custom target with no outputs, so every build wiped the tree with rm -rf and re-copied it, and a concurrent reader could see it empty. It is now one explicit file list feeding staging, install, and the dependency edge, with per-file copy_if_different. A no-op build is now a no-op. The list is wiped once per configure rather than once per build, because copy_if_different cannot retract a descriptor deleted from the sources: Ninja orphans an output dropped from the graph instead of removing it, and the loader would keep reading a descriptor that no longer exists. Editing the list is what re-runs CMake, so deletion still propagates -- verified by dropping an entry and watching the staged tree fall to 19. Install now reads that same list instead of a directory pattern. The test binary's copy of the pack kernels duplicated the engine's list of the same four files. Both now include one IngestorKernels.cmake, since a pack whose kernel reaches only one of them fails at plan build, from a set of files CMake had in hand. The .jsonc comment was misread in review as "both extensions load". It never loads: all seven suffixes end in .json, so a .jsonc matches none of them and is diagnosed rather than read. Reworded to say that. Left alone: the arch_content/<arch> layout question, which needs the packager's tree to move in the same window.

…escriptors/ child The extra level said nothing the parent did not. Per-arch shards land as arch_content/hip-kernel-provider/<arch>/ when the packager emits them, and the walk is recursive with folder names carrying no meaning, so the layout needs no further change to accept them. One line, because the spelling has exactly one home: the staging rule, the install DESTINATION, the path compiled into the provider and the offset the resolution tests assert all read HIPDNN_DESCRIPTOR_SUBDIR. Nothing else in the tree names arch_content. An existing build tree migrates on reconfigure without stale copies, since the new root is the old one's parent and is cleared there. Verified on a relocated install rather than by inspection: installing to a prefix that did not exist at configure time and listing engines from it reports 4, the same as the build tree, and 2 once the installed descriptor tree is moved aside. The installed copy is now the copy the installed plugin reads, with no environment variable in play.

## Summary An inline `kernelDescriptors` entry is a UKD — the same schema a `.ukd.json` holds, differing only in where it is written — but it could not state which UKD schema version it targets, and was implicitly governed by its pack's version instead. This PR requires `version` on both spellings and routes them through one `versionIsSupported()`, so every descriptor in the format is gated by the same rule against its own type's accepted pair. JIRA ID : ALMIOPEN-2401 Follows up on [PR #10679](#10679) and implements the per-descriptor versioning rule in [RFC 0017 §4](projects/hipdnn/docs/rfcs/0017_UniversalKernelDescriptor.md). ## Field Handling How the loader treats each kind of key, for reviewers reading the parse changes. Only the `version` row moves in this PR — on an inline kernel it was previously an unknown key, and is now required. | Case | Result | |---|---| | Required key missing, or present with the wrong type | ERROR naming the file; the file is skipped | | Optional key absent | Documented default — an empty `arch` means every arch, absent `metadata` takes the KMD's defaults | | Unknown key | ERROR naming the file; the file is skipped | | Extension key: `x-` or `_` prefix, or `provenance` | WARN and ignored | | `version` this build cannot read | WARN; the descriptor is skipped, not an error | Unknown keys error rather than warn because absence is legal for most optional fields, so a typo like `heuristik` would otherwise load an engine that ranks by the fallback heuristic forever — and the default log level is `off`, so a warning reaches nobody. Note the differing scope of the last two rows: a parse error inside an inline kernel fails its whole `.kdp.json`, while a version skew on that same kernel drops only that kernel and leaves the pack serving the rest. ## Risk Assessment Low risk. The change is confined to descriptor parsing and is covered end to end: the new requirement is a format break, but the format is pre-release behind `HIPDNN_ENABLE_KERNEL_INGESTOR`, and the only content affected is in-repo — 11 shipped inline kernels, all stamped here and verified by the on-device run. The one new runtime behavior, refusing a kernel that declares a version this build cannot read, is unreachable until a UKD version bump. Both arms of the gate are mutation-checked, and unifying the rule means the pre-existing file-level version tests now defend the inline path too. ## Testing Summary - hipDNN plugin SDK unit tests covering the contract on both spellings: an inline kernel without `version` is refused exactly as a `.ukd.json` without one is, a newer version skips that kernel while its pack keeps serving, and a pack left with no readable kernel drops through the existing no-kernels path. - Provider unit and on-device integration tests confirming the 11 newly stamped shipped inline kernels load and dispatch unchanged. - Mutation checks on the unified gate, confirming the tests fail when either comparison arm is weakened. - A `.ukd.json` skew test closing a pre-existing gap: the violation cases only ever varied a UED's or KMD's version, leaving the UKD row uncovered. - Ingestor-disabled build, confirming the change stays behind `HIPDNN_ENABLE_KERNEL_INGESTOR`. ## Testing Checklist - [x] hipDNN plugin SDK unit tests - `./build-ued/bin/hipdnn_plugin_sdk_tests` - Status: Passed (376 tests, 4 new) - [x] hip-kernel-provider unit tests - `./build-ued/bin/hip_kernel_provider_tests` - Status: Passed (529 tests) - [x] Kernel ingestor integration tests - `./build-ued/bin/hip_kernel_provider_integration_tests --gtest_filter='*KernelIngestor*'` - ASICs: gfx90a - Status: Passed (14 tests) - [x] Commit hooks - `pre-commit run` - Status: Passed - [ ] PR CI - GitHub PR checks - Status: Pending ## Technical Changes - Reworks `versionIsSupported()` to take the accepted `major`/`minor` and a caller-supplied locator instead of a `FileType` row, so it serves any descriptor rather than only a file. A file names itself by path; an inline entry names itself by its position in a pack. - Requires `version` on inline `kernelDescriptors` entries and gates it against the UKD row rather than the KDP's, which versions the pack's own shape. This removes the last descriptor that was neither required to declare a version nor checked against one. - Collapses the kernel parsers into one. `parseKernelDescriptor()` reads a whole kernel given a locator and is what the UKD row calls directly; `parseInlineKernelDescriptor()` adds the locator, object check, and version gate a file gets from the walk, then delegates. The separate body helper, shared key-list helper, and standalone wrapper existed only to let two parsers agree, and are gone. A standalone document now drops into a `kernelDescriptors` array verbatim. - Deletes the separate inline predicate and the unstamped-version default. RFC 0017 §4 refuses a descriptor rather than reinterpreting it, and reading an unstamped entry as `1.0` was a reinterpretation. - Removes a redundant presence check in `versionIsSupported()` whose diagnostic `requireKey()` already produces verbatim one line later; the four missing-version tests are unchanged and still pass. - Stamps the 11 shipped inline kernels. The twelfth `kernelDescriptors` entry is a bare id reference whose `.ukd.json` carries its own version as a file. - Replaces `RejectsAnInlineKernelCarryingAVersion`, which pinned the previous contract, with `RejectsAnInlineKernelWithNoVersion` plus tests for the partial skip and the fully skewed pack. - Adds a `.ukd.json` minor-version-skew test. The UKD row already had major-mismatch coverage (`SkipsAStandaloneKernelOnAnUnsupportedMajor`, pre-existing); the minor arm was untested, and the new test also pins the outcome difference between the two spellings -- a skewed file drops its pack as a dangling reference, while a skewed inline entry leaves the pack its readable kernels. - The rest of a kernel descriptor is only validated once its version is accepted -- a version-skewed entry is skipped before its keys or arch are checked, so an unknown key or an out-of-pack arch on that same entry is never separately reported. Pinned by `SkewOnAnInlineKernelSupersedesItsOtherViolations`.

github-actions Bot added github actions project: hipdnn project: hip-kernel-provider labels Aug 12, 2026

SamuelReeder self-assigned this Aug 12, 2026

assistant-librarian Bot added the organization: ROCm label Aug 12, 2026

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch 3 times, most recently from 1ad41cc to 6219120 Compare August 12, 2026 17:00

SamuelReeder commented Aug 12, 2026

View reviewed changes

Comment thread projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/DescriptorLoader.hpp Resolved

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch from a326256 to ff9cf7c Compare August 12, 2026 20:35

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch from e534255 to 5a31713 Compare August 13, 2026 01:39

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch from 5a31713 to b4cee72 Compare August 13, 2026 05:30

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch 2 times, most recently from 0b68369 to 8f9a16a Compare August 13, 2026 16:22

github-actions Bot added the documentation label Aug 13, 2026

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch 3 times, most recently from 257cfcd to debf972 Compare August 13, 2026 19:57

SamuelReeder commented Aug 13, 2026

View reviewed changes

Comment thread projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/DescriptorLoader.hpp Resolved

SamuelReeder commented Aug 13, 2026

View reviewed changes

Comment thread projects/hipdnn/plugin_sdk/include/hipdnn_plugin_sdk/ingestor/DescriptorLoader.hpp Resolved

SamuelReeder marked this pull request as ready for review August 14, 2026 13:37

SamuelReeder requested review from a team as code owners August 14, 2026 13:37

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch from c66d2c3 to b820628 Compare August 14, 2026 16:02

SamuelReeder added 14 commits August 18, 2026 11:39

SamuelReeder force-pushed the users/sareeder/ued-runtime-loading branch from 82a6f1e to b53b68d Compare August 18, 2026 17:40