-
Notifications
You must be signed in to change notification settings - Fork 845
Implement cuGraphExecUpdate_v2 #528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the cuGraphExecUpdate_v2
CUDA function by adding it to the CUDA function declarations and providing a HIP backend implementation. This function updates an executable graph with a new graph and provides detailed information about any update failures.
- Adds
cuGraphExecUpdate_v2
to the CUDA function declarations - Implements the HIP backend wrapper that converts HIP graph update results to CUDA equivalents
- Adds necessary type mappings for
CUgraphExecUpdateResultInfo
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
zluda_common/src/lib.rs | Adds CUgraphExecUpdateResultInfo type mapping for CUDA to HIP conversion |
zluda/src/lib.rs | Declares the cuGraphExecUpdate_v2 function in the CUDA function list |
zluda/src/impl/graph.rs | Implements the HIP backend for cuGraphExecUpdate_v2 with result conversion logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
zluda/src/impl/graph.rs
Outdated
let mut update_result: hipGraphExecUpdateResult = unsafe { std::mem::zeroed() }; | ||
unsafe { hipGraphExecUpdate(h_graph_exec, h_graph, &mut h_error_node, &mut update_result) }?; | ||
|
||
result_info.errorNode = unsafe { std::mem::transmute(h_error_node) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to use FromCuda
instead of transmute here and below?
In case we change the definition of CUgraphExec to be more complicated than transmute of hipGraphExec_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's the new code look? It still uses transmute
but in a wrapper struct returned from FromCuda
This is very involved, how about something like this (with an appropriate comment why we do these acrobatics instead of
|
This reverts commit f770063.
Sure, done. |
zluda/src/impl/graph.rs
Outdated
) -> CUresult { | ||
// We use FromCuda here instead of transmute in case our hipGraphNode_t representation changes | ||
// in the future. | ||
let mut h_error_node: hipGraphNode_t = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong to me, my reading is that
- We load bits from
result_info.errorNode
intoh_error_node
on stack hipGraphExecUpdate
writes intoh_error_node
on stack- We copy bits from
result_info.errorNode
(without writing into it) intoresult_info.errorFromNode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're correct. The code you wrote works I think, I just misread it before.
No description provided.