Repository navigation
SWDEV-556588 - Use linked list for packetBatches_ as graphNode enable… #1128
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
Changes from 3 commits
9dfcd21
d77e075
c281a22
6b15202
bb57b4a
b8d73f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -401,8 +401,9 @@ hipError_t GraphExec::CaptureAndFormPacketsForGraph() { | |
| nodeCaptureStatus_.clear(); | ||
| nodeCaptureStatus_.resize(topoOrder_.size(), false); | ||
|
|
||
| // Clear previous batches | ||
| // Clear previous batches and node mapping | ||
| packetBatches_.clear(); | ||
| nodeToPacketsMap_.clear(); | ||
|
|
||
| // Process nodes and create batches of consecutive captured nodes | ||
| for (size_t i = 0; i < topoOrder_.size(); ++i) { | ||
|
|
@@ -418,32 +419,49 @@ hipError_t GraphExec::CaptureAndFormPacketsForGraph() { | |
| } | ||
|
|
||
| if (node->GraphCaptureEnabled()) { | ||
| // Start of a potential batch - try to capture packets for this node | ||
| std::vector<uint8_t*> currentBatch; | ||
| std::vector<std::string> currentKernelNames; | ||
| // Start of a new batch | ||
| PacketBatch newBatch; | ||
| PacketNode* lastNode = nullptr; | ||
|
|
||
| // Collect packets from consecutive captured nodes | ||
| size_t j = i; | ||
| size_t capturedNodeCount = 0; | ||
| while (j < topoOrder_.size() && topoOrder_[j]->GraphCaptureEnabled()) { | ||
| auto& currentNode = topoOrder_[j]; | ||
| status = currentNode->CaptureAndFormPacket(GetKernelArgManager(), ¤tBatch, | ||
| ¤tKernelNames); | ||
|
|
||
| if (status != hipSuccess || currentBatch.empty()) { | ||
| // Capture packets for this node | ||
| std::vector<uint8_t*> nodePackets; | ||
| std::vector<std::string> nodeKernelNames; | ||
| status = currentNode->CaptureAndFormPacket(GetKernelArgManager(), &nodePackets, | ||
| &nodeKernelNames); | ||
|
|
||
| if (status != hipSuccess || nodePackets.empty()) { | ||
| LogError("Packet capture failed"); | ||
| return status; | ||
| } | ||
|
|
||
| // Create PacketNodes for this node's packets | ||
| std::vector<PacketNode*> nodePacketNodes; | ||
| for (size_t k = 0; k < nodePackets.size(); ++k) { | ||
| PacketNode* packetNode = new PacketNode(nodePackets[k], nodeKernelNames[k], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also the created nodes, where are they released. You are putting this on the heap.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, it is part of packetBatch_ so when it gets released, dctor for PacketBatch release PacketNodes. |
||
| currentNode); | ||
| nodePacketNodes.push_back(packetNode); | ||
|
|
||
| // Insert into batch LinkedList | ||
| newBatch.insertAfter(lastNode, packetNode); | ||
| lastNode = packetNode; | ||
| } | ||
|
|
||
| // Store mapping from node to its PacketNodes | ||
| nodeToPacketsMap_[currentNode] = nodePacketNodes; | ||
|
|
||
| // Mark this node as successfully captured | ||
| nodeCaptureStatus_[j] = true; | ||
| ++j; | ||
| ++capturedNodeCount; | ||
| } | ||
|
|
||
| // Add the batch if it has packets | ||
| if (!currentBatch.empty()) { | ||
| packetBatches_.emplace_back(std::move(currentBatch), std::move(currentKernelNames), | ||
| capturedNodeCount); | ||
| if (newBatch.head != nullptr) { | ||
| packetBatches_.push_back(std::move(newBatch)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use emplace_back
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
| } | ||
|
|
||
| // Skip the nodes we just processed, the index will be incremented by the loop | ||
|
|
@@ -453,7 +471,7 @@ hipError_t GraphExec::CaptureAndFormPacketsForGraph() { | |
| if (childNode->GetChildGraph()->max_streams_ == 1) { | ||
| childNode->SetGraphCaptureStatus(true); | ||
| status = childNode->CaptureAndFormPacketsForGraph(); | ||
| nodeCaptureStatus_[i] = (status == hipSuccess); | ||
| //nodeCaptureStatus_[i] = (status == hipSuccess); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is nodeCaptureStatus ignored it,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Originally, I have this change as fix to child graph test as it is observed that if child graph is captured in batch, It is not processed in run. Child graph has its own graphExec. However it looks with recent changes that I have made, this can ve reverted to original. |
||
| if (status != hipSuccess) { | ||
| status = hipSuccess; // Continue with other nodes | ||
| } | ||
|
|
@@ -493,11 +511,60 @@ hipError_t GraphExec::CaptureAQLPackets() { | |
| hipError_t GraphExec::UpdateAQLPacket(hip::GraphNode* node) { | ||
| hipError_t status = hipSuccess; | ||
| if (max_streams_ == 1) { | ||
| status = node->CaptureAndFormPacket(kernArgManager_); | ||
| // Find the node's packets in the mapping | ||
| auto it = nodeToPacketsMap_.find(node); | ||
| if (it == nodeToPacketsMap_.end()) { | ||
| return hipSuccess; // Node not in any batch | ||
| } | ||
|
|
||
| // Capture new packets for this node | ||
| std::vector<uint8_t*> newPackets; | ||
| std::vector<std::string> newKernelNames; | ||
| status = node->CaptureAndFormPacket(kernArgManager_, &newPackets, &newKernelNames); | ||
| if (status != hipSuccess) { | ||
| return status; | ||
| } | ||
|
|
||
| // Simple case: assume same number of packets, just update content | ||
| // This is much simpler and covers the common case | ||
| for (size_t i = 0; i < it->second.size() && i < newPackets.size(); ++i) { | ||
| it->second[i]->packet = newPackets[i]; | ||
| it->second[i]->kernelName = newKernelNames[i]; | ||
| } | ||
| } | ||
| return status; | ||
| } | ||
|
|
||
| // ================================================================================================ | ||
| hipError_t GraphExec::UpdatePacketBatchesForNodeEnableDisable(hip::GraphNode* node, bool isEnabled) { | ||
| hipError_t status = hipSuccess; | ||
| if (max_streams_ != 1) { | ||
| // Only handle single stream case for now | ||
| return hipSuccess; | ||
| } | ||
|
|
||
| // Find the node's packets in the mapping | ||
| auto it = nodeToPacketsMap_.find(node); | ||
| if (it == nodeToPacketsMap_.end()) { | ||
| return hipSuccess; // Node not in any batch | ||
| } | ||
|
|
||
| if (isEnabled) { | ||
| // Node is being enabled - just mark packets as enabled | ||
| // No need to re-capture packets, just mark them as active | ||
| for (auto packetNode : it->second) { | ||
| packetNode->node = node; // Mark as enabled | ||
| } | ||
| } else { | ||
| // Node is being disabled - mark packets as disabled | ||
| for (auto packetNode : it->second) { | ||
| packetNode->node = nullptr; // Mark as disabled | ||
| } | ||
| } | ||
|
|
||
| return status; | ||
| } | ||
|
|
||
| // ================================================================================================ | ||
|
|
||
| void GraphExec::DecrementRefCount(cl_event event, cl_int command_exec_status, void* user_data) { | ||
|
|
@@ -508,8 +575,6 @@ void GraphExec::DecrementRefCount(cl_event event, cl_int command_exec_status, vo | |
| // ================================================================================================ | ||
|
|
||
| hipError_t GraphExec::EnqueueGraphWithSingleList(hip::Stream* hip_stream) { | ||
| // Accumulate command tracks all the AQL packet batch that we submit to the HW. For now | ||
| // we track only kernel nodes. | ||
| amd::AccumulateCommand* accumulate = nullptr; | ||
| hipError_t status = hipSuccess; | ||
| if (DEBUG_CLR_GRAPH_PACKET_CAPTURE) { | ||
|
|
@@ -518,31 +583,56 @@ hipError_t GraphExec::EnqueueGraphWithSingleList(hip::Stream* hip_stream) { | |
|
|
||
| size_t batchIndex = 0; | ||
|
|
||
| // Process nodes in topological order with mixed execution strategy | ||
| // Process nodes in topological order - MAINTAIN THIS! | ||
| for (size_t i = 0; i < topoOrder_.size(); ++i) { | ||
| auto& node = topoOrder_[i]; | ||
|
|
||
| if (!node->GraphCaptureEnabled()) { | ||
| // Skip disabled nodes | ||
| if (node->GetEnabled() == 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (node->GetType() == hipGraphNodeTypeGraph) { | ||
| // Handle child graph nodes | ||
| auto childNode = reinterpret_cast<hip::ChildGraphNode*>(node); | ||
| if (childNode->GetChildGraph()->max_streams_ == 1) { | ||
| // Execute child graph with its own packet capture | ||
| status = childNode->EnqueueGraphWithSingleList(hip_stream); | ||
| if (status != hipSuccess) { | ||
| accumulate->release(); | ||
| return status; | ||
| } | ||
| } | ||
| } else if (!node->GraphCaptureEnabled()) { | ||
| // Node doesn't support capture - execute individually | ||
| node->SetStream(hip_stream); | ||
| status = node->CreateCommand(node->GetQueue()); | ||
| node->EnqueueCommands(hip_stream); | ||
| } else if (i < nodeCaptureStatus_.size() && nodeCaptureStatus_[i]) { | ||
| // Node was successfully captured - find which batch it belongs to | ||
| // and dispatch the entire batch | ||
| // Node was successfully captured - dispatch the entire batch | ||
| if (batchIndex < packetBatches_.size()) { | ||
| // Dispatch this batch | ||
| bool batchStatus = hip_stream->vdev()->dispatchAqlPacketBatch( | ||
| packetBatches_[batchIndex].packets, packetBatches_[batchIndex].kernelNames, accumulate); | ||
| if (!batchStatus) { | ||
| status = hipErrorUnknown; | ||
| accumulate->release(); | ||
| return status; | ||
| // Collect enabled packets from this batch | ||
| std::vector<uint8_t*> activePackets; | ||
| std::vector<std::string> activeKernelNames; | ||
| packetBatches_[batchIndex].getEnabledPackets(activePackets, activeKernelNames); | ||
|
|
||
| if (!activePackets.empty()) { | ||
| // Dispatch this batch | ||
| bool batchStatus = hip_stream->vdev()->dispatchAqlPacketBatch(activePackets, activeKernelNames, accumulate); | ||
| if (!batchStatus) { | ||
| status = hipErrorUnknown; | ||
| accumulate->release(); | ||
| return status; | ||
| } | ||
| } | ||
|
|
||
| // Skip all consecutive captured nodes that belong to this batch | ||
| // Use the tracked node count to skip directly instead of parsing one by one | ||
| i += packetBatches_[batchIndex].capturedNodeCount - 1; // -1 because loop will increment | ||
| // We need to count how many nodes are in this batch | ||
| size_t nodesInBatch = 0; | ||
| for (size_t j = i; j < topoOrder_.size() && j < nodeCaptureStatus_.size() && nodeCaptureStatus_[j]; ++j) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I maintained the capturedNodeCount to avoid doing this loop here during the launch.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
| nodesInBatch++; | ||
| } | ||
| i += nodesInBatch - 1; // -1 because loop will increment | ||
|
|
||
| ++batchIndex; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -873,6 +873,8 @@ class GraphExec : public amd::ReferenceCountedObject, public Graph { | |
| void GetKernelArgSizeForGraph(size_t& kernArgSizeForGraph); | ||
| hipError_t EnqueueGraphWithSingleList(hip::Stream* hip_stream); | ||
| bool TopologicalOrder() { return Graph::TopologicalOrder(topoOrder_); } | ||
| // Handle packetBatches_ updates when nodes are enabled/disabled | ||
| hipError_t UpdatePacketBatchesForNodeEnableDisable(hip::GraphNode* node, bool isEnabled); | ||
|
|
||
| protected: | ||
| //! Topological order of the graph doesn't include nodes embedded as part of the child graph | ||
|
|
@@ -884,22 +886,72 @@ class GraphExec : public amd::ReferenceCountedObject, public Graph { | |
| bool hasHiddenHeap_ = false; //!< Hidden heap indicator for Kernel node | ||
| bool repeatLaunch_ = false; | ||
|
|
||
| //! Structure for batch dispatch optimization - packets and kernel names in aligned memory | ||
| struct PacketBatch { | ||
| std::vector<uint8_t*> packets; | ||
| std::vector<std::string> kernelNames; | ||
| size_t capturedNodeCount; // Number of consecutive captured nodes in this batch | ||
| //! Structure for individual packet in LinkedList | ||
| struct PacketNode { | ||
| uint8_t* packet; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be a vector of packets, ie if each node has more than once packet
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
| std::string kernelName; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same a vector of kernelNames
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
| GraphNode* node; // Which node owns this packet | ||
| PacketNode* next; | ||
| PacketNode* prev; | ||
| PacketNode() : packet(nullptr), node(nullptr), next(nullptr), prev(nullptr) {} | ||
| PacketNode(uint8_t* p, const std::string& name, GraphNode* n) | ||
| : packet(p), kernelName(name), node(n), next(nullptr), prev(nullptr) {} | ||
| }; | ||
|
|
||
| PacketBatch() : capturedNodeCount(0) {} | ||
| PacketBatch(std::vector<uint8_t*>&& p, std::vector<std::string>&& k, size_t nodeCount) | ||
| : packets(std::move(p)), kernelNames(std::move(k)), capturedNodeCount(nodeCount) {} | ||
| //! Structure for batch dispatch optimization using LinkedList | ||
| struct PacketBatch { | ||
| PacketNode* head; | ||
| PacketNode* tail; | ||
| PacketBatch() : head(nullptr), tail(nullptr) {} | ||
| // O(1) insertion after a specific node | ||
| void insertAfter(PacketNode* after, PacketNode* newNode) { | ||
| if (newNode == nullptr) return; | ||
| if (after == nullptr) { | ||
| // Insert at head | ||
| newNode->next = head; | ||
| newNode->prev = nullptr; | ||
| if (head) head->prev = newNode; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use proper braces
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. multiple places here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
| head = newNode; | ||
| if (tail == nullptr) tail = newNode; | ||
| } else { | ||
| // Insert after 'after' | ||
| newNode->next = after->next; | ||
| newNode->prev = after; | ||
| if (after->next) after->next->prev = newNode; | ||
| after->next = newNode; | ||
| if (after == tail) tail = newNode; | ||
| } | ||
| } | ||
| // O(1) removal of a specific node | ||
| void remove(PacketNode* node) { | ||
| if (node == nullptr) return; | ||
| if (node->prev) node->prev->next = node->next; | ||
| else head = node->next; | ||
| if (node->next) node->next->prev = node->prev; | ||
| else tail = node->prev; | ||
| } | ||
| // Get all enabled packets as vectors for dispatch | ||
| void getEnabledPackets(std::vector<uint8_t*>& packets, std::vector<std::string>& kernelNames) const { | ||
| packets.clear(); | ||
| kernelNames.clear(); | ||
| PacketNode* current = head; | ||
| while (current) { | ||
| if (current->node != nullptr) { // Enabled | ||
| packets.push_back(current->packet); | ||
| kernelNames.push_back(current->kernelName); | ||
| } | ||
| current = current->next; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| //! Batches of accumulated packets and kernel names for batch dispatch optimization | ||
| //! Each batch contains packets from consecutive captured nodes | ||
| std::vector<PacketBatch> packetBatches_; | ||
| //! Track which nodes were successfully captured (true) vs need individual execution (false) | ||
| std::vector<bool> nodeCaptureStatus_; | ||
| //! Map from GraphNode to its PacketNodes for O(1) lookup | ||
| std::unordered_map<GraphNode*, std::vector<PacketNode*>> nodeToPacketsMap_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if PacketNode already has vector of packets, this map doesnt need a vector of PacketNodes*, simplifies that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
| }; | ||
|
|
||
| class ChildGraphNode : public GraphNode, public GraphExec { | ||
|
|
||
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 would change, since now the constructor can directly accept the packet/kernelName vectors, no loop needed
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.
Done.