From 2317282e6efe4f3f7d1990a9ae8fa823dbd8102c Mon Sep 17 00:00:00 2001 From: JC Date: Fri, 28 Aug 2026 19:57:51 -0700 Subject: [PATCH] fix(clr): pair graph kernel names with GPU timings by dispatch slot (#10579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation `AccumulateCommand` paired graph kernel names with GPU timestamps positionally: names were appended as each AQL packet was written, timings were appended as each completion signal was drained, and `ReportActivity` zipped the two vectors. That is correct only if both sequences have identical membership and identical order, and on the segmented graph path neither was guaranteed — so graph traces reported kernels with other kernels' durations, which makes per-kernel graph profiling unusable. ## Technical Details Three effects perturbed the pairing: - **Registration order** — pre-patched segment completion signals bypass `ActiveSignal` and were registered in bulk in `submitAccumulate` after every batch had been dispatched, so they landed on the `Timestamp` that `updateCommandsState` drains first and their timings arrived ahead of the per-batch dispatch timings. - **Barrier misclassification** — when every node in a batch is disabled the completion signal is relocated onto a standalone barrier, but the signal was classified from the original packet, so a barrier still counted as a kernel dispatch and contributed a timing with no name behind it. - **Drain order** — signals are not necessarily drained in packet order at all. Queue-pool reuse makes `ActiveSignal` wait on a recycled signal, which extracts that signal's timing immediately, ahead of signals for packets dispatched earlier. The third cannot be fixed by reordering, since it is the signal pool rather than this code that decides when a timing is extracted. The pairing is made order-independent instead: `addKernelDispatch` records the kernel name and the queue it was dispatched on during the packet walk and returns that record's slot index, the packet's `ProfilingSignal` carries the slot in `dispatch_slot_`, and `ExtractSignalTiming` writes the timing back through `setDispatchTiming` rather than appending. Pairing is therefore fixed at the moment the packet is written, and no drain order can disturb it. That also lets some machinery go away. Holding a dispatch slot replaces the `isPacketDispatch_` flag and the packet-type sniffing in `ApplyHwEventPatches`, so a completion signal relocated onto a barrier is no longer mistaken for a dispatch. The queue a kernel ran on now comes from the dispatching vGPU during the packet walk, removing `HwEventPatch::segment_id`, `HwEventPatch::queue_index`, the launch-time loop resolving one into the other, and `ProfilingSignal::queue_index_`. Pre-patched signals are still registered with the `Timestamp` from their own packet, found by HSA handle, with the handle read from the host-side flat buffer because the ring slot is write-combining and cannot be read back reliably on the NT path. A slot whose signal never produced valid timing is skipped by `ReportActivity` instead of shifting every later kernel onto the wrong timestamp. Separately, `__hipUnregisterFatBinary` called only `SyncAllStreams` before `RemoveFatBinary`. That waits for the GPU to finish and for the host to observe the completion signals, but not for the HSA async-handler thread to leave its callback — and that callback reports kernel names pointing into the `Kernel` objects `RemoveFatBinary` is about to destroy, so the final launch's names could reach the trace as freed memory. The handlers are now drained as well. ## Issue Tracking JIRA ID: AIRUNTIME-N ## Test Plan A ten-node fan-out/join HIP graph in which every node runs a uniquely named kernel with a distinct GPU duration (100us through 1000us, spaced 50us apart). A name/timestamp mis-pairing then shows up directly as a kernel reporting a different node's duration, and a dangling kernel name shows up as unparseable bytes in the trace. Run across: - the default graph configuration plus two node-disable patterns, which exercise the relocated completion signal; - segment scheduling modes 0, 1 and 2; - both the NT and MOVDIR64B packet publication paths; - forced signal-pool reuse, pool size 2 against 4096, to provoke out-of-order drain. ## Test Result 15 traces across the default configuration and the two node-disable patterns pair correctly on every node, with the four parallel paths attributed to four distinct queues. Forcing signal reuse keeps all 2800 dispatches present with their medians within 1.67%. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. (cherry picked from commit 5aa428a8e5feabdc58061166971286be9280bbcc) --- .../clr/hipamd/src/hip_graph_internal.cpp | 18 +-- projects/clr/hipamd/src/hip_platform.cpp | 10 +- projects/clr/rocclr/device/device.hpp | 6 - projects/clr/rocclr/device/rocm/rocdevice.cpp | 22 +--- projects/clr/rocclr/device/rocm/rocdevice.hpp | 21 ++- .../clr/rocclr/device/rocm/rocvirtual.cpp | 122 ++++++++++-------- projects/clr/rocclr/platform/activity.cpp | 25 ++-- projects/clr/rocclr/platform/command.hpp | 55 ++++---- 8 files changed, 137 insertions(+), 142 deletions(-) diff --git a/projects/clr/hipamd/src/hip_graph_internal.cpp b/projects/clr/hipamd/src/hip_graph_internal.cpp index c7f8f5c1cba..6d14e2f4dd3 100644 --- a/projects/clr/hipamd/src/hip_graph_internal.cpp +++ b/projects/clr/hipamd/src/hip_graph_internal.cpp @@ -603,13 +603,13 @@ void GraphExecSegmented::BuildSyncPlan() { sync_plan_.patch_list.push_back( {completion_barrier, nullptr, hw_slot, - amd::Device::HwEventPatch::kCompletionSignal, segment.id}); + amd::Device::HwEventPatch::kCompletionSignal}); } else if (!lastBatch.dispatchPackets.empty() && completion_signal_needed) { // Safe to patch the last kernel dispatch directly uint8_t* last_pkt = lastBatch.dispatchPackets.back(); sync_plan_.patch_list.push_back( {last_pkt, nullptr, hw_slot, - amd::Device::HwEventPatch::kCompletionSignal, segment.id}); + amd::Device::HwEventPatch::kCompletionSignal}); // The completion signal is embedded on this specific kernel packet. If the // owning node is later disabled (hipGraphNodeSetEnabled), that packet is // filtered out of the dispatch buffer and the signal would be lost, @@ -2504,23 +2504,15 @@ amd::Command* GraphExecSegmented::EnqueueSegmentedGraph(hip::Stream* launch_stre // Apply pre-computed patches -- writes HW events directly into flatPacketData // via the flat_packet pointers resolved at instantiate time, so no rebuild needed. - // Resolve each completion-signal patch's segment_id (set at BuildSyncPlan time) to - // the actual stream's queue index now that streams are available. if (!sync_plan_.patch_list.empty()) { - for (auto& patch : sync_plan_.patch_list) { - if (patch.dep_slot == amd::Device::HwEventPatch::kCompletionSignal && - patch.segment_id >= 0 && - patch.segment_id < static_cast(segments_.size())) { - patch.queue_index = resolveSegmentStream(segments_[patch.segment_id])->vdev()->index(); - } - } device->ApplyHwEventPatches(sync_plan_.patch_list, segment_hw_events); } // Single AccumulateCommand on launch_stream manages all HW event lifetimes // and serves as the dispatch anchor for all segments across all streams. - // Kernel names are copied into the command at dispatch time (via addKernelName - // in dispatchAqlPacketBatchFlat) — no string borrowing, no GraphExecBase pin. + // Kernel dispatch records are added to the command at dispatch time (via + // addKernelDispatch in dispatchAqlPacketBatchFlat) — no string borrowing, + // no GraphExecBase pin. auto* graph_accumulate = new amd::AccumulateCommand(*launch_stream, {}, nullptr); // Register HW events with graph_accumulate so profiling can read them. diff --git a/projects/clr/hipamd/src/hip_platform.cpp b/projects/clr/hipamd/src/hip_platform.cpp index 207af7cf095..bb8a3135335 100644 --- a/projects/clr/hipamd/src/hip_platform.cpp +++ b/projects/clr/hipamd/src/hip_platform.cpp @@ -323,9 +323,15 @@ void __hipUnregisterFatBinary(void** modules) { if (!HIP_SKIP_ABORT_ON_GPU_ERROR || !amd::Device::IsGPUInError()) { std::call_once(unregister_device_sync, []() { for (const auto& hipDevice : g_devices) { - // By synchronizing devices ensure that all HSA signal handlers - // complete before RemoveFatBinary hipDevice->SyncAllStreams(true); + // SyncAllStreams only guarantees the GPU finished and the host observed + // the completion signals — the HSA async-handler thread can still be + // inside a completion callback. That callback reports kernel names that + // point into the Kernel objects RemoveFatBinary is about to destroy, so + // the handlers have to be drained too, not just the streams. + for (auto* device : hipDevice->devices()) { + device->WaitForHsaAsyncHandlersIdle(); + } } }); } diff --git a/projects/clr/rocclr/device/device.hpp b/projects/clr/rocclr/device/device.hpp index 50fcf80f742..5e462674973 100644 --- a/projects/clr/rocclr/device/device.hpp +++ b/projects/clr/rocclr/device/device.hpp @@ -2257,12 +2257,6 @@ class Device : public RuntimeObject { uint8_t* flat_packet; // pointer into flatPacketData (patched directly at launch) int hw_event_index; int dep_slot; // kCompletionSignal, kExtDispatchDepSignal, or 0-4 for barrier dep_signal[slot] - // Segment that owns this patch (set at BuildSyncPlan time). At launch the - // graph layer resolves it to the actual stream's vGPU index into queue_index. - int segment_id = -1; - // vGPU (queue) index resolved at launch from segment_id. Read by - // ApplyHwEventPatches to attribute the signal to its execution stream. - uint32_t queue_index = std::numeric_limits::max(); }; virtual uint8_t* CreateBarrierPacket() const { return nullptr; } diff --git a/projects/clr/rocclr/device/rocm/rocdevice.cpp b/projects/clr/rocclr/device/rocm/rocdevice.cpp index d0057b79dd9..8abb8192176 100644 --- a/projects/clr/rocclr/device/rocm/rocdevice.cpp +++ b/projects/clr/rocclr/device/rocm/rocdevice.cpp @@ -4095,26 +4095,10 @@ void Device::ApplyHwEventPatches(const std::vector& patches, auto* pkt = reinterpret_cast(raw); pkt->completion_signal = sig; - // Prepare this signal for profiling: mark it as active and classify - // the packet type so checkGpuTime → addTimestamps only fires for - // kernel dispatches (not synthetic barriers). + // Prepare this signal for profiling. The dispatch path assigns this + // launch's slot only when the actual carrier is a kernel dispatch. ps->flags_.done_ = false; - // Record the queue this patched dispatch signal runs on (resolved from the - // owning segment's stream at launch) so profiling attributes it to the - // right stream rather than the graph launch stream. - ps->queue_index_ = patch.queue_index; - uint16_t hdr; - memcpy(&hdr, patch.packet, sizeof(hdr)); - uint8_t pktType = hdr & ((1 << HSA_PACKET_HEADER_WIDTH_TYPE) - 1); - // A kernel dispatch could be a vendor-specific ext-kernel-dispatch - // packet, identified by amd_format (byte 2). Classify it as a dispatch so - // the patched last-node completion signal contributes its GPU timing like - // every other graph kernel node. - const uint8_t amdFormat = patch.packet[2]; - ps->flags_.isPacketDispatch_ = - (pktType == HSA_PACKET_TYPE_KERNEL_DISPATCH) || - (pktType == HSA_PACKET_TYPE_VENDOR_SPECIFIC && - amdFormat == HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH); + ps->dispatch_slot_ = ProfilingSignal::kNoDispatchSlot; } else { // dep_slot >= 0: patch a barrier's dependency signal slot (cross-segment wait) auto* pkt = reinterpret_cast(raw); diff --git a/projects/clr/rocclr/device/rocm/rocdevice.hpp b/projects/clr/rocclr/device/rocm/rocdevice.hpp index 956c74e7f39..5675fc15d26 100644 --- a/projects/clr/rocclr/device/rocm/rocdevice.hpp +++ b/projects/clr/rocclr/device/rocm/rocdevice.hpp @@ -60,24 +60,21 @@ class PrintfDbg; class ProfilingSignal : public amd::ReferenceCountedObject { public: - //! Sentinel for queue_index_ when the owning stream is unknown. - static constexpr uint32_t kInvalidQueueIndex = std::numeric_limits::max(); + //! Sentinel for dispatch_slot_ when this signal doesn't time a reported graph dispatch. + static constexpr uint32_t kNoDispatchSlot = std::numeric_limits::max(); hsa_signal_t signal_; //!< HSA signal to track profiling information Timestamp* ts_; //!< Timestamp object associated with the signal HwQueueEngine engine_; //!< Engine used with this signal - //! vGPU (queue) index of the stream this signal was dispatched on. Graphs span - //! multiple streams under one command, so profiling reads this per-signal to - //! attribute each kernel to the queue it actually ran on. - uint32_t queue_index_ = kInvalidQueueIndex; + //! AccumulateCommand dispatch record this signal supplies timing for. + uint32_t dispatch_slot_ = kNoDispatchSlot; std::recursive_mutex lock_; //!< Signal lock for update typedef union { struct { - uint32_t done_ : 1; //!< True if signal is done - uint32_t isPacketDispatch_ : 1; //!< True if the packet, used with the signal, is dispatch - uint32_t interrupt_ : 1; //!< True if the signal will trigger an interrupt - uint32_t reserved_ : 29; + uint32_t done_ : 1; //!< True if signal is done + uint32_t interrupt_ : 1; //!< True if the signal will trigger an interrupt + uint32_t reserved_ : 30; }; uint32_t data_; } Flags; @@ -96,7 +93,7 @@ class ProfilingSignal : public amd::ReferenceCountedObject { signal_.handle = 0; flags_.data_ = 0; flags_.done_ = true; - queue_index_ = kInvalidQueueIndex; + dispatch_slot_ = kNoDispatchSlot; } virtual ~ProfilingSignal(); @@ -111,7 +108,7 @@ class ProfilingSignal : public amd::ReferenceCountedObject { cached_timing_.start_ = 0; cached_timing_.end_ = 0; cached_timing_.valid_ = false; - queue_index_ = kInvalidQueueIndex; + dispatch_slot_ = kNoDispatchSlot; } //! Check if timing is already cached diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index 66e1fbfcd38..70d9cd8a9d1 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -483,14 +483,14 @@ void Timestamp::ExtractSignalTiming(ProfilingSignal* signal, end = std::max(sig_end, end); } - // Handle AccumulateCommand timestamps (convert ticks to system time). - // Pass signal->queue_index_ so ReportActivity can assign each kernel to - // the internal parallel stream it actually ran on, not the launch stream. - if ((command().type() == CL_COMMAND_TASK) && (signal->flags_.isPacketDispatch_ == true)) { - static_cast(command()).addTimestamps( - static_cast(sig_start * ticksToTime_), - static_cast(sig_end * ticksToTime_), - signal->queue_index_); + // Write graph dispatch timing into the slot assigned when its AQL packet was + // reported. Signals can be drained early during queue-pool reuse, so appending + // here would order timestamps by drain order rather than packet order. + if (command().type() == CL_COMMAND_TASK && + signal->dispatch_slot_ != ProfilingSignal::kNoDispatchSlot) { + static_cast(command()).setDispatchTiming( + signal->dispatch_slot_, static_cast(sig_start * ticksToTime_), + static_cast(sig_end * ticksToTime_)); } signal->flags_.done_ = true; @@ -808,9 +808,7 @@ hsa_signal_t VirtualGPU::HwQueueTracker::ActiveSignal(hsa_signal_value_t init_va Hsa::signal_silent_store_relaxed(prof_signal->signal_, init_val); prof_signal->flags_.done_ = false; prof_signal->engine_ = engine_; - prof_signal->flags_.isPacketDispatch_ = false; prof_signal->ResetCachedTiming(); - prof_signal->queue_index_ = gpu_.index(); // Release any existing HwEvent before setting new one for the same command VirtualGPU::AttachHwEvent(cmd, prof_signal); @@ -1508,9 +1506,6 @@ bool VirtualGPU::dispatchGenericAqlPacket(AqlPacket* packet, uint16_t header, ui packet->reserved2 = timestamp_->command().profilingInfo().correlation_id_; } } - - ProfilingSignal* current_signal = Barriers().GetLastSignal(); - current_signal->flags_.isPacketDispatch_ = true; } // Make sure the slot is free for usage @@ -1792,12 +1787,28 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& auto* first_loc = reinterpret_cast( queueBase + (startIndex & queueMask) * kPacketSize); - // Attach profiling / completion signals to one packet. Used by the MOVDIR64B path, - // which assembles the full packet (body + signal + valid header) in a host staging - // buffer before the atomic 64B store, so signals must be written into |pkt| (staging) - // rather than patched into the ring slot after the body copy (the NT path does the - // latter inline below). |isLast| marks the final packet of the whole batch. - auto attachPacketSignal = [&](hsa_kernel_dispatch_packet_t* pkt, size_t i, bool isLast) { + // A pre-patched packet already carries the completion signal ApplyHwEventPatches + // wrote, so it never goes through ActiveSignal and nothing has registered it with + // the Timestamp. Index the command's HW events by HSA handle so the packet walk + // below can register each one from its own packet, in dispatch order. + std::unordered_map prePatchedSignals; + if (pre_patched && timestamp_ != nullptr) { + for (const auto& [hw_device, hw_events] : vcmd->getHwEvents()) { + for (void* hw_event : hw_events) { + auto* signal = reinterpret_cast(hw_event); + prePatchedSignals.emplace(signal->signal_.handle, signal); + } + } + } + + // Attach profiling / completion signals to one packet and return the signal + // that times it when this is a kernel dispatch. Used by the MOVDIR64B path, + // which assembles the full packet (body + signal + valid header) in a host + // staging buffer before the atomic 64B store, so signals must be written into + // |pkt| (staging) rather than patched into the ring slot after the body copy. + // |isLast| marks the final packet of the whole batch. + auto attachPacketSignal = [&](hsa_kernel_dispatch_packet_t* pkt, size_t i, + bool isLast) -> ProfilingSignal* { const uint16_t hdr = static_cast(validFullHeaders[i]); const uint8_t pktType = extractAqlBits(hdr, HSA_PACKET_HEADER_TYPE, HSA_PACKET_HEADER_WIDTH_TYPE); @@ -1808,29 +1819,43 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& (pktType == HSA_PACKET_TYPE_VENDOR_SPECIFIC && amdFormat == HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH); if (timestamp_ != nullptr) { - // When pre_patched, keep any completion_signal already written by - // ApplyHwEventPatches (carried into staging via the flat-buffer copy). - bool has_prepatched_signal = pre_patched && (pkt->completion_signal.handle != 0); - if (!has_prepatched_signal) { + // Read the pre-patched completion signal from the host-side flat buffer, not + // from |pkt|: on the NT path |pkt| is the write-combining ring slot, which + // cannot be read back reliably. + const auto* hostPkt = reinterpret_cast( + flatPacketData.data() + i * kPacketSize); + const uint64_t prePatchedHandle = pre_patched ? hostPkt->completion_signal.handle : 0; + if (prePatchedHandle == 0) { pkt->completion_signal = Barriers().ActiveSignal(kInitSignalValueOne, timestamp_, true); if (isKernelDispatch) { if (isBaseKernelDispatch && amd::activity_prof::IsEnabled(OP_ID_DISPATCH)) { pkt->reserved2 = timestamp_->command().profilingInfo().correlation_id_; } - Barriers().GetLastSignal()->flags_.isPacketDispatch_ = true; + ProfilingSignal* signal = Barriers().GetLastSignal(); + return signal; + } + } else { + // Keep the completion_signal ApplyHwEventPatches already wrote (carried into + // staging via the flat-buffer copy), and register it with the Timestamp from + // here rather than in bulk at submit time. + if (isBaseKernelDispatch && amd::activity_prof::IsEnabled(OP_ID_DISPATCH)) { + pkt->reserved2 = timestamp_->command().profilingInfo().correlation_id_; + } + auto it = prePatchedSignals.find(prePatchedHandle); + if (it != prePatchedSignals.end()) { + timestamp_->AddProfilingSignal(it->second); + return isKernelDispatch ? it->second : nullptr; } - } else if (has_prepatched_signal && isBaseKernelDispatch && - amd::activity_prof::IsEnabled(OP_ID_DISPATCH)) { - pkt->reserved2 = timestamp_->command().profilingInfo().correlation_id_; } } else if (isLast && (attach_signal || blocking)) { pkt->completion_signal = Barriers().ActiveSignal(); } + return nullptr; }; // Kernel-name collection is required when dispatch activity tracing is on; detailed - // packet logging when LOG_KERN2 / LOG_AQL is on. Both are handled by logBatchPacket. + // packet logging when LOG_KERN2 / LOG_AQL is on. Both are handled by reportBatchPacket. const bool needKernelNamesReported = amd::activity_prof::IsEnabled(OP_ID_DISPATCH); const bool kLogBatch = IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL); @@ -1839,9 +1864,11 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& // from the ring slot: the device-resident ring is write-combining, so reading // kernel_object back from it is unreliable. Kernel names are resolved from the device // KernelMap by kernel_object and, when activity tracing is on, recorded into the command - // via addKernelName(). getDemangledName() returns a reference to a name cached for the - // device's lifetime, so the borrowed pointer stays valid. - auto logBatchPacket = [&](size_t i, uint64_t slotIdx) { + // via addKernelDispatch(). The returned dispatch slot is stored on |packetSignal| so + // timing extraction can fill the same record regardless of signal drain order. The + // borrowed name stays valid until the command reports its activity: + // __hipUnregisterFatBinary syncs every stream before removing a code object. + auto reportBatchPacket = [&](size_t i, uint64_t slotIdx, ProfilingSignal* packetSignal) { const auto* hostPkt = reinterpret_cast( flatPacketData.data() + i * kPacketSize); const uint16_t hdr = @@ -1861,7 +1888,13 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& ? kit->second.getDemangledName().c_str() : ""; if (needKernelNamesReported) { - vcmd->addKernelName(kname); + // index() is this vGPU's slot, i.e. the stream this batch was dispatched + // on — a segmented graph spreads its packets over several of them. + const uint32_t queue_index = index(); + const uint32_t dispatch_slot = vcmd->addKernelDispatch(kname, queue_index); + if (packetSignal != nullptr) { + packetSignal->dispatch_slot_ = dispatch_slot; + } } if (kLogBatch) { ClPrint(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2, @@ -1961,11 +1994,12 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& const uint64_t slotIdx = (startIndex + i) & queueMask; auto* slot = reinterpret_cast( queueBase + slotIdx * kPacketSize); + ProfilingSignal* packetSignal = nullptr; if (timestamp_ != nullptr) { - attachPacketSignal(slot, i, i == numPackets - 1); + packetSignal = attachPacketSignal(slot, i, i == numPackets - 1); } if (needKernelNamesReported || kLogBatch) { - logBatchPacket(i, slotIdx); + reportBatchPacket(i, slotIdx, packetSignal); } } } @@ -2000,7 +2034,8 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& const uint64_t slotIdx = (startIndex + i) & queueMask; alignas(64) hsa_kernel_dispatch_packet_t stg; std::memcpy(&stg, flatPacketData.data() + i * kPacketSize, kPacketSize); - attachPacketSignal(&stg, i, i == numPackets - 1); + ProfilingSignal* packetSignal = + attachPacketSignal(&stg, i, i == numPackets - 1); const uint32_t dword = validFullHeaders[i]; const uint16_t hdr = (i == 0) ? firstHeader : (i == numPackets - 1) ? lastHeader @@ -2010,7 +2045,7 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const amd::AlignedVector64& auto* dst = queueBase + slotIdx * kPacketSize; amd::movdir64b_copy64(dst, &stg); if (needKernelNamesReported || kLogBatch) { - logBatchPacket(i, slotIdx); + reportBatchPacket(i, slotIdx, packetSignal); } } } @@ -5239,21 +5274,6 @@ void VirtualGPU::submitAccumulate(amd::AccumulateCommand& vcmd) { std::scoped_lock lock(execution()); profilingBegin(vcmd); - // Register pre-patched HW event signals with the Timestamp for profiling. - // These signals were configured by ApplyHwEventPatches (isPacketDispatch_, - // done_ flags set there) but bypass ActiveSignal, so they must be added - // here so checkGpuTime → ExtractSignalTiming → addTimestamps picks them up. - if (timestamp_ != nullptr) { - for (const auto& [_, events] : vcmd.getHwEvents()) { - for (void* hw_event : events) { - auto* ps = reinterpret_cast(hw_event); - if (ps != nullptr) { - timestamp_->AddProfilingSignal(ps); - } - } - } - } - const Settings& settings = dev().settings(); if (settings.barrier_value_packet_) { dispatchBarrierValuePacket(kBarrierVendorPacketNopScopeHeader, true); diff --git a/projects/clr/rocclr/platform/activity.cpp b/projects/clr/rocclr/platform/activity.cpp index 8039734d6ef..1e25d62ba1c 100644 --- a/projects/clr/rocclr/platform/activity.cpp +++ b/projects/clr/rocclr/platform/activity.cpp @@ -135,21 +135,18 @@ void ReportActivity(const amd::Command& command) { } if (command.type() == CL_COMMAND_TASK) { - auto timestamps = static_cast(command).getTimestamps(); - const auto& kernel_names = - static_cast(command).getKernelNames(); - // kernel_names and timestamps are both populated only when profiling is active - // at dispatch time. Walk the shorter of the two as a safety bound. - for (uint32_t i = 0; i < kernel_names.size() && i < timestamps.size(); i++) { - auto& ts = timestamps[i]; - record.begin_ns = ts.start; - record.end_ns = ts.end; - record.kernel_name = kernel_names[i]; - // Use per-packet queue_index when available so each kernel is assigned - // to the internal parallel stream it ran on, not the launch stream. - if (ts.queue_index != UINT32_MAX) { - record.queue_id = static_cast(ts.queue_index); + const auto& kernel_dispatches = + static_cast(command).getKernelDispatches(); + for (const auto& dispatch : kernel_dispatches) { + // Missing or invalid signal timing leaves this dispatch's slot empty + // instead of shifting every later kernel onto the wrong timestamp. + if (dispatch.end_ns == 0) { + continue; } + record.begin_ns = dispatch.start_ns; + record.end_ns = dispatch.end_ns; + record.kernel_name = dispatch.kernel_name; + record.queue_id = static_cast(dispatch.queue_index); function(ACTIVITY_DOMAIN_HIP_OPS, operation_id, &record); } } else { diff --git a/projects/clr/rocclr/platform/command.hpp b/projects/clr/rocclr/platform/command.hpp index 21eeb4a298a..b36412d85fb 100644 --- a/projects/clr/rocclr/platform/command.hpp +++ b/projects/clr/rocclr/platform/command.hpp @@ -1674,21 +1674,23 @@ class Marker : public Command { }; class AccumulateCommand : public Command { - private: - //! Stable kernel name pointers — one entry per kernel dispatch slot (base and - //! ext variants). Resolved from KernelMap at dispatch time; point into Kernel - //! objects that live for the device lifetime. Non-dispatch slots (barriers, - //! SDMA) are skipped so kernelNames_ and timestamps_ are always parallel. - //! "" is used when the kernel_object is not found in KernelMap. - std::vector kernelNames_; - //! GPU timestamps — one entry per kernel dispatch slot, parallel to - //! kernelNames_, populated at signal completion time. - struct KernelTimestamp { - uint64_t start; - uint64_t end; - uint32_t queue_index; //!< vGPU index of the stream that ran this kernel + public: + //! One graph kernel dispatch. The name and queue are known when the AQL packet + //! is written; its signal fills the timing later by dispatch slot, so signal + //! drain order cannot change which name receives the timing. + struct KernelDispatch { + const char* kernel_name; + //! vGPU slot of the stream this dispatch ran on; one accumulate command can + //! span several streams when the graph is segmented. + uint32_t queue_index; + uint64_t start_ns; + uint64_t end_ns; }; - std::vector timestamps_; + + private: + //! Graph kernel dispatches in AQL packet order. Non-dispatch packets are + //! omitted; an unprocessed or invalid signal leaves its slot timing at zero. + std::vector kernel_dispatches_; //! HW events that need to be released when this command is destroyed std::unordered_map> hw_events_; //! When false, the destructor does not destroy hw_events_ (an external owner, @@ -1726,20 +1728,23 @@ class AccumulateCommand : public Command { //! them across launches instead. void setOwnsHwEvents(bool owns) { owns_hw_events_ = owns; } - //! Record a stable kernel name pointer for one kernel dispatch slot. - //! Must not be nullptr — use "" when the name cannot be resolved. - void addKernelName(const char* name) { kernelNames_.push_back(name); } - - //! Add GPU timestamps for one dispatch slot (called at signal completion). - void addTimestamps(uint64_t startTs, uint64_t endTs, uint32_t queue_index = UINT32_MAX) { - timestamps_.push_back({startTs, endTs, queue_index}); + //! Reserve one graph kernel dispatch slot and return its index. |name| must + //! not be nullptr — use "" when it cannot be resolved. + uint32_t addKernelDispatch(const char* name, uint32_t queue_index) { + kernel_dispatches_.push_back({name, queue_index, 0, 0}); + return static_cast(kernel_dispatches_.size() - 1); } - //! Return kernel name pointers (one per dispatch slot) - const std::vector& getKernelNames() const { return kernelNames_; } + //! Fill the timing for the dispatch slot owned by a completed signal. + void setDispatchTiming(uint32_t dispatch_slot, uint64_t start_ns, uint64_t end_ns) { + kernel_dispatches_[dispatch_slot].start_ns = start_ns; + kernel_dispatches_[dispatch_slot].end_ns = end_ns; + } - //! Return GPU timestamps (one per dispatch slot, populated at signal completion) - const std::vector& getTimestamps() const { return timestamps_; } + //! Return graph kernel dispatches in AQL packet order. + const std::vector& getKernelDispatches() const { + return kernel_dispatches_; + } //! The command implementation virtual void submit(device::VirtualDevice& device) { device.submitAccumulate(*this); }