hipGraph: fixes needed by Kokkos::Graph on chipStar - #1522
Open
pvelesko wants to merge 22 commits into
Open
Conversation
This was referenced Aug 29, 2026
Collaborator
Author
|
/run-aurora-ci |
1 similar comment
Collaborator
Author
|
/run-aurora-ci |
A kernel launched into a stream that is inside
hipStreamBeginCapture/hipStreamEndCapture executes immediately and the
returned graph is empty. Kokkos hip.graph_capture observes the captured
kernel's write before the graph is launched ("0 is not in data used in
the captured kernel, got 5").
…dges
hipGraphLaunch returns hipErrorTbd ("Failed to find" thrown by
CHIPGraphNode::removeDependency) when the graph contains redundant edges
that more than one path makes redundant. Kokkos hip_graph.when_all_cycle
builds this shape with when_all() over nodes that are already ordered by
a longer chain.
captureIntoGraph() started with an unconditional return false, so every kernel launch, memcpy, memset and event op issued to a stream between hipStreamBeginCapture and hipStreamEndCapture executed right away and the graph returned by hipStreamEndCapture was empty. Kokkos wraps stream-captured regions in child graph nodes and saw the captured kernel's result before the graph was launched. initCaptureGraph() also forgets the last node of the previous capture on the stream; otherwise the first node of a new capture is chained behind a node that belongs to another graph, which leaves the new graph without a root node.
pruneGraph_() enumerates every leaf-to-root path once and then walks all (path, sub-path) pairs, so an edge that several pairs single out as redundant is handed to removeDependency() again after the first pair removed it, and removeDependency() throws "Failed to find", turning hipGraphLaunch into hipErrorTbd. Any when_all() over nodes that a longer chain already orders produces such a shape.
hipGraphLaunch on a graph containing a hipGraphAddChildGraphNode node
returns hipErrorTbd ("Attemped to execute GraphNode"). Kokkos wraps
every stream-captured region in a child graph node, so
hip.graph_capture cannot submit its graph.
The N-th hipGraphLaunch of one hipGraphExec_t runs the graph's nodes N times (a single kernel node runs 6 times over 3 launches). Kokkos hip_graph.repeat_chain submits one graph ten times.
CHIPGraphNodeGraph::execute() threw "Attemped to execute GraphNode", so hipGraphLaunch failed with hipErrorTbd on any graph that held a hipGraphAddChildGraphNode node. The node now instantiates its child graph and runs it to completion at its position in the schedule, which already places it after its dependencies and before its dependants. ExtractSubGraphs_() was meant to inline the child graph but edited a local copy of the node list that compile() never looked at, wired the edges the wrong way round, and read one element past the end of the node list whenever the child graph node was the last node.
hipGraphGetNodes and hipGraphGetRootNodes dereference the output array even when the caller passes null to query the count, which segfaults. hipGraphGetRootNodes also returns the total node count and hipGraphGetNodes fills only the first array slot. Kokkos hip.graph_instantiate_and_debug_dot_print dies here before it reaches hipGraphDebugDotPrint.
hipGraphDebugDotPrint returns hipErrorNotSupported and writes no file. Kokkos hip.graph_instantiate_and_debug_dot_print expects a DOT file that names the kernel of each kernel node.
Both wrote the first node through the output array without checking it, so the documented count-only query with a null array segfaulted, and a non-null array only ever received its first slot. hipGraphGetRootNodes also reported the total node count rather than the root count. Both now return the count for a null array and otherwise fill up to the given capacity and return the number written.
Writes the graph as a DOT digraph: one box per node labelled with its type, kernel nodes with the kernel's host function name, launch dimensions and memset, host and event details under the matching hipGraphDebugDotFlags bits, child graphs as nested clusters, and one edge per dependency. The call used to return hipErrorNotSupported.
hipGraphNodeSetEnabled and hipGraphNodeGetEnabled return hipErrorNotSupported. Kokkos hip.interact_with_hip_node disables the kernel node of an instantiated graph, submits, expects the kernel not to have run, re-enables it, submits again and expects it to have run.
The enabled switch belongs to the executable graph, so it is stored on the node's copy in CHIPGraphExec's compiled graph, found through the clone map from the original node handle the API identifies it by. A disabled node is skipped at launch, which makes it behave like an empty node. Only kernel, memcpy and memset nodes take the switch; other types, a node the graph did not hold at instantiation, null handles and a null output pointer return hipErrorInvalidValue.
…ng graph hipGraphExecKernelNodeSetParams and hipGraphExecMemsetNodeSetParams look the node up in the clone map of the original graph, which is only populated when that graph itself came from hipGraphClone, so on a normally built graph they fail with hipErrorTbd or hipErrorInvalidValue. Where they do resolve a node they update the original graph's node instead of its copy in the executable graph. The test instantiates a one node graph, launches it, updates the executable graph's node, launches again and expects the new parameters, then checks the original graph still carries and reproduces the old ones.
setParams only stored the hipKernelNodeParams struct while execute() launches the exec item whose kernel, arguments and launch configuration were bound at construction, so hipGraphKernelNodeSetParams changed what hipGraphKernelNodeGetParams reported and nothing else. It also kept the caller's kernelParams pointer, which is only valid for the call. setParams now resolves the kernel, deep copies the argument bytes into the node's own buffers and rebuilds the exec item with a private kernel handle, which is what the two constructors did inline; both go through it now. The copy constructor does too instead of cloning the exec item: the clone shared the source node's argument buffer, so a copy of a kernel node was only usable while the original node was alive.
… graph
The hipGraphExec{Kernel,Memcpy,MemcpyToSymbol,MemcpyFromSymbol,Memset,
Host}NodeSetParams and hipGraphExec{EventRecord,EventWait}NodeSetEvent
bindings looked the node up in the clone map of the original graph,
which is only populated when that graph was itself produced by
hipGraphClone, so on a normally built graph they failed with hipErrorTbd
or hipErrorInvalidValue. Where they did resolve a node most of them then
updated the original graph's node rather than its copy in the executable
graph.
They now resolve the node through CHIPGraphExec::getExecNode(), the
executable graph's own clone map, and update that copy; a node the graph
did not hold at instantiation or a node of another type is
hipErrorInvalidValue. CHIPGraphExec::launch() runs the copy instead of
the original node, so the update is what the next hipGraphLaunch
executes while the original graph and later instantiations of it keep
their parameters, as hipamd does.
hipLaunchHostFunc never consulted the stream's capture status, so between hipStreamBeginCapture and hipStreamEndCapture the host function was enqueued as a stream callback and ran right away, while the kernels around it were recorded into the graph. A captured kernel/host/kernel chain thus ran its host steps during capture and the graph replayed only the kernels. The call is now recorded as a CHIPGraphNodeHost chained behind the last captured node, the same way kernel launches, memcpys and memsets are, so it runs in dependency order at hipGraphLaunch.
CHIPBackend.hh includes CHIPGraph.hh before chipstar::ExecItem is defined, so the inline destructor deleted ExecItem_ through an incomplete type (-Wdelete-incomplete) and skipped ExecItem's virtual destructor. Move the definition into CHIPGraph.cc.
Mirror the pNodeParams and pNodeParams->fn checks of hipGraphHostNodeSetParams so Unit_hipGraphExecHostNodeSetParams_Negative gets hipErrorInvalidValue instead of installing a null callback.
hipGraphAddChildGraphNode documents childGraph as the graph to clone into the node, but CHIPGraphNodeGraph aliased the caller's graph, so a graph added as its own child made CHIPGraphExec::launch recurse without bound (Unit_hipGraphAddChildGraphNode_OrgGraphAsChildGraph overflowed the stack) and destroying or editing the child graph after the add changed what the parent executed. Clone in the constructors and in setGraph, and have hipGraphExecChildGraphNodeSetParams compare node counts instead of graph identity, which can no longer match a clone.
hipGraphGetNodes now honours numNodes as the capacity of the nodes array on entry, so asking for the nodes with numNodes still zero fills nothing and reports zero. The test read the count back into a variable it had left at 0, which the old implementation ignored.
pvelesko
force-pushed
the
2026-08-29-hipgraph-kokkos-fixes
branch
from
August 31, 2026 12:32
bb4039a to
939d739
Compare
Collaborator
Author
|
/run-aurora-ci |
1 similar comment
Collaborator
Author
|
/run-aurora-ci |
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
1 participant
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.
Found running the Kokkos::HIP unit tests (Kokkos::Graph, stream capture) on Aurora PVC. Stream capture executed work eagerly,
hipGraphLaunchfailed on redundant edges and on child graph nodes, repeated launches re-ran the graph N times,hipGraphGetNodessegfaulted on a null array,hipGraphDebugDotPrintandhipGraphNodeSetEnabledwere unimplemented,hipGraphExec*NodeSetParamsupdated the wrong graph, andhipLaunchHostFuncunder capture ran immediately. Each fix has its reproducer test committed before it. Three follow-up commits from review of the series define the kernel node destructor wherechipstar::ExecItemis complete, makehipGraphExecHostNodeSetParamsreject null host params, and make child graph nodes own a clone of the child graph as the HIP header documents, which stops a graph added as its own child from recursing at launch.Fixes #1490
Fixes #1491
Fixes #1492
Fixes #1493
Fixes #1494
Fixes #1495
Fixes #1496
Fixes #1498
Fixes #1502