Skip to content

Conversation

zluda-violet
Copy link
Collaborator

No description provided.

Copy link

@Copilot Copilot AI left a 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-violet zluda-violet requested a review from vosen September 25, 2025 23:20
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) };
Copy link
Owner

@vosen vosen Sep 26, 2025

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

Copy link
Collaborator Author

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

@vosen
Copy link
Owner

vosen commented Sep 30, 2025

This is very involved, how about something like this (with an appropriate comment why we do these acrobatics instead of mem::transmute):

    let error_node: *mut hipGraphNode_t = FromCuda::from_cuda(std::ptr::from_mut(&mut result_info.errorNode))?;
    let error_from_node: *mut hipGraphNode_t = FromCuda::from_cuda(std::ptr::from_mut(&mut result_info.errorFromNode))?;
    *error_node = h_error_node;
    *error_from_node = h_error_node;

@zluda-violet zluda-violet requested a review from vosen September 30, 2025 05:02
@zluda-violet
Copy link
Collaborator Author

Sure, done.

) -> 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 =
Copy link
Owner

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 into h_error_node on stack
  • hipGraphExecUpdate writes into h_error_node on stack
  • We copy bits from result_info.errorNode (without writing into it) into result_info.errorFromNode

Copy link
Collaborator Author

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.

@zluda-violet zluda-violet requested a review from vosen September 30, 2025 18:36
@vosen vosen merged commit f68ea06 into vosen:master Sep 30, 2025
6 checks passed
@zluda-violet zluda-violet deleted the cuGraphExecUpdate_v2 branch October 1, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants