Repository navigation
feat(hipdnn): re-save a loaded execution plan from the bytes it was loaded from - #14517
Merged
GarbyGilbertAMD merged 5 commits intoOct 30, 2026
Merged
Conversation
…oaded from A plan loaded from saved bytes could not be saved again. The backend asked the plugin to serialize the restored execution context, and hip-kernel-provider refuses to capture a plan it restored from a payload, so to_binary() of a graph loaded with its plan and to_compiled_plan_binary() of a loaded plan failed. ExecutionPlanDescriptor::deserializeBackendPlan seeds the serialized plan cache with the exact bytes it loaded, after the plugin restores the context and before the descriptor is finalized. A later save returns those bytes and never calls the plugin. A re-save therefore describes the kernel the loaded plan runs, with its original provider version and format versions; a newer kernel in an upgraded provider never enters it. To use a newer kernel, build the plan again from its graph and save it. Keeping the loaded bytes in the backend was chosen over re-encoding the plan inside the provider. It works for every plugin and every payload shape, adds no contract for plugin authors, and makes a re-save byte-identical. It also keeps a version 1 envelope at version 1, where a rebuilt envelope would claim version 2 with no tensor alignments and fail to load. The cost is one copy of each loaded plan's bytes for the plan's lifetime, the same copy a saved built plan keeps. The provider still refuses a direct call of its save hook on a restored context, with a message that names the backend behaviour. The backend tests cover the byte-identical re-save, the version 1 envelope, a failed restore, and the first-save cache of a built plan, including concurrent saves and a retry after a failed fill. The GPU save/load integration tests save the loaded plan again, load the second blob in a fresh handle, and validate it for pointwise and convolution, as a graph with its plan and as a plan alone. RFC 0009, the plugin development guide and the provider README describe the behaviour. ALMIOPEN-2940
github-actions
Bot
added
documentation
project: hipdnn
project: hip-kernel-provider
labels
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #14517 +/- ##
===========================================
- Coverage 77.35% 77.35% -0.00%
===========================================
Files 2894 2894
Lines 487094 487094
Branches 73692 73692
===========================================
- Hits 376774 376771 -3
- Misses 85462 85465 +3
Partials 24858 24858
*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 28, 2026 16:13
BrianHarrisonAMD
approved these changes
Oct 28, 2026
BrianHarrisonAMD
left a comment
BrianHarrisonAMD
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
Some minor feedback to consider pre-merge:
Comments
- The graph-and-plan re-save check is too weak. In IntegrationGpuKernelIngestor.cpp, the GRAPH_AND_PLAN case only checks that the
second save contains a plan. It does not check that the plan inside is unchanged. That is the path where
hipdnnBackendCreateAndDeserializeExecutionPlan_ext cuts the plan out of the combined container. Suggest checking that
to_compiled_plan_binary() returns the same bytes for loaded and reloaded. - The docs' "original bytes" wording is too broad. If you pass a combined graph-and-plan container to from_compiled_plan_binary,
only the plan part is cached. to_compiled_plan_binary() then returns just the plan, not the container you passed in. That
behaviour is right, but the RFC and README say "the bytes it was loaded from", which reads like the whole input. Saying "the plan
bytes it was loaded from" fixes it. - The memory note in the PR description is wrong. It says this is "the same copy a saved built plan already keeps". A built plan
only keeps the copy once it has been saved. Every loaded plan now keeps one whether or not it is saved again, and
hip-kernel-provider plans contain the kernel code. - Optional: deserializeBackendPlan makes a temporary copy of the plugin payload that it doesn't need; it could pass
serializedPluginPayload->data() directly. That copy predates this PR, but loading now also allocates the cache, so peak memory at
load is higher. - Optional: the public header comment on Graph::serialize_compiled_plan still says "Requires build_plans() or build()". A plan
loaded from bytes can now be saved too, and the header is where users will look.
…bytes A loaded plan keeps the plan bytes it was loaded from and returns them on a later save. When the input to from_compiled_plan_binary() is a graph-and-plan container, only its plan part is kept, so to_compiled_plan_binary() returns the plan and not the container. The descriptor, the frontend header, RFC 0009 and the provider README now say plan bytes instead of the bytes it was loaded from. The documentation also states the memory cost exactly. Every loaded plan keeps one copy of its plan bytes for its lifetime, whether or not it is saved again, while a built plan keeps a copy only after its first save. For the kernel ingestor engines the plan bytes include the kernel code object. Graph::serialize_compiled_plan() states that a plan loaded from bytes can be saved as well as a built plan. The save and load integration tests of a graph with its plan now compare the plan bytes that the loaded graph and the reloaded graph save with the plan bytes of the original plan, which covers the path that cuts the plan out of the container. deserializeBackendPlan() hands the plugin payload to the plugin directly instead of copying it first, which lowers peak memory at load. ALMIOPEN-2940
CMiservaAMD
approved these changes
Oct 28, 2026
CMiservaAMD
left a comment
CMiservaAMD
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The item described in this comment isn't really in scope of this PR but it looks like something that would be good to clear-up as it's related to loading graph-only (trying to load graph-only from a file that doesn't have a graph isn't properly captured as an error): https://github.com/ROCm/rocm-libraries/pull/14517/changes#r4406618366
…h rejection A re-save of a loaded plan succeeds and returns the plan bytes it was loaded from, so it teaches a caller nothing about the kernel inside. A caller who upgrades the provider for a kernel fix and then loads and saves their plans gets the old kernel back. The frontend and backend headers now say so at each save and load entry point. Graph::serialize_compiled_plan() states that a loaded plan is not rebuilt, that its bytes still hold the original kernel and the provider and format versions, that loading a graph-and-plan container keeps only its plan part, and that a newer kernel needs a new plan built from the graph. Its error message for a graph without a finalized plan also names from_compiled_plan_binary(). serialize() and to_binary() describe when they embed the plan, to_compiled_plan_binary() follows serialize_compiled_plan(), and the graph-only loaders point out that loading the graph alone is how to replace a plan saved by an older provider. The load entry points, and hipdnnBackendCreateAndDeserializeExecutionPlan_ext() in the C header, state that a restored plan keeps a copy of its plan bytes, including any GPU code object, for its lifetime. hipdnnBackendGetSerializedExecutionPlan_ext() states that a deserialized plan returns its loaded bytes unchanged without calling the plugin. RFC 0009 limits the byte-identical guarantee to the plan; a re-save of the graph is not guaranteed to be byte-identical. A bare plan and a bare graph both carry no file identifier, so hipdnnBackendCreateAndDeserializeGraph_ext() parses a bare plan as a graph and rejects it only because the Graph verifier fails on the plan's fields. A new test saves a plan with the production writer and expects the graph loader to reject it with HIPDNN_STATUS_BAD_PARAM and no descriptor. ALMIOPEN-2940
Contributor
Author
Logged ALMIOPEN-3105 to capture this |
CMiservaAMD
approved these changes
Oct 29, 2026
Brings the branch up to date with develop. The save rules in RFC 0009 keep the branch's list, which moves the restored-plan refusal out of the numbered rules, and add develop's rule that a kernel with more than one launch cannot be saved, in the order captureIngestorPlan() checks. ALMIOPEN-2940
Brings in the documentation of plan device binding. The deserialize_compiled_plan() comment keeps both paragraphs: the restored plan keeps its plan bytes and returns them on a later save, and it is bound to the device of the load handle, so moving it to another device means deserializing the same bytes there. ALMIOPEN-2940
GarbyGilbertAMD
enabled auto-merge (squash)
October 30, 2026 16:15
GarbyGilbertAMD
deleted the
users/ggilbert/ALMIOPEN-2940/resave-loaded-plan
branch
October 30, 2026 21:08
Contributor
TheRock Submodule Bump ActivityNewest first
|
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 plan loaded from saved bytes could not be saved again: the backend asked the plugin to serialize the restored execution context, and hip-kernel-provider refuses to capture a restored plan, so
to_binary()of a graph loaded with its plan andto_compiled_plan_binary()of a loaded plan both failed. The backend now keeps the plan bytes a plan was loaded from and returns them on a later save, without calling the plugin. When the input is a graph-and-plan container, only its plan part is kept. The design decision is recorded in RFC 0009 ("Decision: Re-Save From the Loaded Bytes").JIRA ID : ALMIOPEN-2940
Follow-up to #13315, which deferred re-saving a loaded plan to this ticket.
Risk Assessment
2 —
ExecutionPlanDescriptor::deserializeBackendPlanseeds the existing serialized-plan cache with one added line, and passes the plugin payload to the plugin without a temporary copy; the save path for a built plan is unchanged. No schema, public API or plugin ABI change. Behaviour change:to_binary()of a graph loaded with its plan andto_compiled_plan_binary()of a loaded plan now succeed for every plugin that exports the three serialization hooks, and hipDNN no longer calls the plugin's save hook for a loaded plan. A re-save returns the plan bytes the plan was loaded from, so it keeps the original provider and format versions; a newer kernel in an upgraded provider never enters it (rebuild the plan from its graph to pick one up). Memory cost: every loaded plan keeps one copy of its plan bytes for its lifetime, whether or not it is saved again, while a built plan keeps a copy only after its first save. For hip-kernel-provider plans the bytes include the kernel code object. Load no longer copies the plugin payload before it hands it to the plugin, which lowers peak memory at load.ASIC Coverage
Architecture-independent host code in the backend descriptor; the provider change is a message and comments. PR CI is sufficient. Verified locally on gfx1152 (Windows), where all four GPU save/load cases ran; PR CI provides the Linux build and the other architectures.
Testing Summary
Testing Checklist
hipdnn_backend_tests(-DHIPDNN_ENABLE_KERNEL_INGESTOR=ON) - Status: Passedhip_kernel_provider_tests- Status: Passedhip_kernel_provider_integration_tests --gtest_filter=IntegrationGpuKernelIngestorSaveLoad*- ASIC: gfx1152 - Status: Passedctest -R test_name_validation(hipDNN and hip-kernel-provider) - Status: PassedTechnical Changes
ExecutionPlanDescriptor::deserializeBackendPlanassigns the plan bytes it was loaded from to_serializedPlanCacheafter the plugin restores the context and beforefinalize();serializeBackendPlanreturns the cache when it is populated.Graph::serialize_compiled_plandocuments that a plan loaded from bytes can be saved.PluginDevelopment.mdand the hip-kernel-provider README describe the behaviour and the memory cost.