diff --git a/projects/clr/hipamd/src/hip_graph_internal.cpp b/projects/clr/hipamd/src/hip_graph_internal.cpp index 13218a4918c0..62004d784c7b 100644 --- a/projects/clr/hipamd/src/hip_graph_internal.cpp +++ b/projects/clr/hipamd/src/hip_graph_internal.cpp @@ -2440,13 +2440,9 @@ amd::Command* GraphExecSegmented::EnqueueSegmentedGraph(hip::Stream* launch_stre // Single AccumulateCommand on launch_stream manages all HW event lifetimes // and serves as the dispatch anchor for all segments across all streams. - // Pass `this` as the kernel-names owner: the command borrows kernel-name - // strings owned by this graph's nodes (via setKernelNamesRef during dispatch) - // and reads them in ReportActivity() at completion, after OnLaunchComplete() - // drops the launch's reference. Tying the GraphExecBase's lifetime to the command - // keeps those strings valid through the report (no copies). We already hold a - // launch reference here, so the retain in the constructor needs no trim lock. - auto* graph_accumulate = new amd::AccumulateCommand(*launch_stream, {}, nullptr, this); + // Kernel names are copied into the command at dispatch time (via addKernelName + // 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. for (auto& hw_event : segment_hw_events) { @@ -2564,13 +2560,11 @@ hipError_t GraphExecSegmented::EnqueueSegment(const Segment& segment, hip::Strea return hipSuccess; } - const std::vector* kernelNamesToDispatch; const std::vector* flatData; const std::vector* flatHdrs; const std::vector* metaData = nullptr; if (packetBatch.disabledNodeCount == 0) { - kernelNamesToDispatch = &packetBatch.dispatchKernelNames; flatData = &packetBatch.flatPacketData; flatHdrs = &packetBatch.validPacketFullHeaders; if (!packetBatch.flatMetadataData.empty()) { @@ -2580,7 +2574,6 @@ hipError_t GraphExecSegmented::EnqueueSegment(const Segment& segment, hip::Strea // Guard against stale filtered buffers: rebuildFlatBuffer (called from // UpdateAQLPacket) invalidates the cache. This is a no-op when valid. packetBatch.rebuildFilteredLists(sync_plan_.patch_list); - kernelNamesToDispatch = &packetBatch.enabledKernelNames; flatData = &packetBatch.filteredFlatPacketData; flatHdrs = &packetBatch.filteredValidPacketFullHeaders; if (!packetBatch.filteredFlatMetadataData.empty()) { @@ -2590,8 +2583,7 @@ hipError_t GraphExecSegmented::EnqueueSegment(const Segment& segment, hip::Strea if (!flatData->empty()) { bool batchStatus = stream->vdev()->dispatchAqlPacketBatchFlat( - *flatData, *flatHdrs, accumulate, attach_signal, kernelNamesToDispatch, true, - false, metaData); + *flatData, *flatHdrs, accumulate, attach_signal, true, false, metaData); if (!batchStatus) { return hipErrorUnknown; } diff --git a/projects/clr/rocclr/device/device.hpp b/projects/clr/rocclr/device/device.hpp index c7959981ce89..5aee5b504f43 100644 --- a/projects/clr/rocclr/device/device.hpp +++ b/projects/clr/rocclr/device/device.hpp @@ -1379,7 +1379,6 @@ class VirtualDevice : public amd::ReferenceCountedObject { const std::vector& validFullHeaders, amd::AccumulateCommand* vcmd = nullptr, bool attach_signal = false, - const std::vector* kernelNames = nullptr, bool pre_patched = false, bool blocking = false, const std::vector* flatMetadataData = nullptr) { diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index ac09e68093f6..9609ce70a5b5 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -491,8 +491,7 @@ void Timestamp::ExtractSignalTiming(ProfilingSignal* signal, 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_); + static_cast(sig_end * ticksToTime_)); } signal->flags_.done_ = true; @@ -1560,9 +1559,12 @@ bool VirtualGPU::dispatchAqlPacket(hsa_barrier_and_packet_t* packet, uint16_t he bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPacketData, const std::vector& validFullHeaders, amd::AccumulateCommand* vcmd, bool attach_signal, - const std::vector* kernelNames, bool pre_patched, bool blocking, const std::vector* flatMetadataData) { + // Both base and ext kernel dispatch packets place kernel_object at the same offset. + static_assert(offsetof(hsa_kernel_dispatch_packet_t, kernel_object) == + offsetof(hsa_amd_ext_kernel_dispatch_packet_t, kernel_object), + "kernel_object offset mismatch between base and ext dispatch packets"); if (vcmd == nullptr || flatPacketData.empty() || validFullHeaders.empty()) { return false; } @@ -1571,15 +1573,10 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack if (flatPacketData.size() != numPackets * 64) { return false; } - std::scoped_lock lock(execution()); profilingBegin(*vcmd); dispatchBlockingWait(nullptr); - if (kernelNames != nullptr && vcmd->profilingInfo().enabled_) { - vcmd->addKernelNames(*kernelNames); - } - const uint32_t queueSize = gpu_queue_->size; const uint32_t queueMask = queueSize - 1; const uint32_t sw_queue_size = queueMask; @@ -1696,8 +1693,11 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack lastSlotPtr->completion_signal = Barriers().ActiveSignal(); } - // Per-packet fixups: profiling signals, kernel-name printing, and inline barrier logging. - if (timestamp_ != nullptr || IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || + // Per-packet fixups: profiling signals, kernel-name collection, kernel-name + // printing, and inline barrier logging. + const bool needKernelNames = amd::activity_prof::IsEnabled(OP_ID_DISPATCH); + if (timestamp_ != nullptr || needKernelNames || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL)) { for (size_t i = chunkStart; i < chunkEnd; ++i) { const uint64_t slotIdx = (startIndex + i) & queueMask; @@ -1736,12 +1736,18 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack slot->reserved2 = timestamp_->command().profilingInfo().correlation_id_; } } - if ((IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || - IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL)) && - kernelNames != nullptr && i < kernelNames->size() && isKernelDispatch) { + if (isKernelDispatch && (needKernelNames || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL))) { + auto kit = dev().KernelMap().find(slot->kernel_object); + const char* kname = kit != dev().KernelMap().end() + ? kit->second.getDemangledName().c_str() + : ""; + if (needKernelNames) { + vcmd->addKernelName(kname); + } ClPrint(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2, "Graph ShaderName : %s, device id : %u", - (*kernelNames)[i] != nullptr ? (*kernelNames)[i]->c_str() : "", - dev().index()); + kname, dev().index()); if (isBaseKernelDispatch) { logAqlDispatchPacket(roc_device_, gpu_queue_, hdr, slot, slotIdx, priority_); } else { diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.hpp b/projects/clr/rocclr/device/rocm/rocvirtual.hpp index cfd8ef110c0b..f5a65c6e83d3 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.hpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.hpp @@ -683,7 +683,6 @@ class VirtualGPU : public device::VirtualDevice { const std::vector& validFullHeaders, amd::AccumulateCommand* vcmd = nullptr, bool attach_signal = false, - const std::vector* kernelNames = nullptr, bool pre_patched = false, bool blocking = false, const std::vector* flatMetadataData = nullptr) override; diff --git a/projects/clr/rocclr/platform/activity.cpp b/projects/clr/rocclr/platform/activity.cpp index 9d8db5759cb4..7b11350814b9 100644 --- a/projects/clr/rocclr/platform/activity.cpp +++ b/projects/clr/rocclr/platform/activity.cpp @@ -138,24 +138,13 @@ void ReportActivity(const amd::Command& command) { auto timestamps = static_cast(command).getTimestamps(); const auto& kernel_names = static_cast(command).getKernelNames(); - // timestamps has one entry per HSA_PACKET_TYPE_KERNEL_DISPATCH packet only. - // kernel_names has one entry per AQL packet slot (nullptr for barriers and SDMA/copy nodes - // that don't generate timestamps). Walk kernel_names; for each non-null entry consume - // the next timestamp. - // Queue id to fall back on when a timestamp did not record its stream. - const uint64_t fallback_queue_id = queue->vdev()->index(); - uint32_t ti = 0; - for (uint32_t ki = 0; ki < kernel_names.size() && ti < timestamps.size(); ki++) { - if (kernel_names[ki] == nullptr) continue; - const auto& it = timestamps[ti++]; - record.begin_ns = it.start; - record.end_ns = it.end; - // Graph kernels can run on different streams under one command; attribute - // each to the queue it actually ran on when known. - record.queue_id = (it.queue_index != amd::AccumulateCommand::kInvalidQueueIndex) - ? static_cast(it.queue_index) - : fallback_queue_id; - record.kernel_name = kernel_names[ki]->c_str(); + // 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 it = timestamps[i]; + record.begin_ns = it.first; + record.end_ns = it.second; + record.kernel_name = kernel_names[i]; function(ACTIVITY_DOMAIN_HIP_OPS, operation_id, &record); } } else { diff --git a/projects/clr/rocclr/platform/command.cpp b/projects/clr/rocclr/platform/command.cpp index cc1d1485639d..99e1e578ee78 100644 --- a/projects/clr/rocclr/platform/command.cpp +++ b/projects/clr/rocclr/platform/command.cpp @@ -63,14 +63,6 @@ Event::~Event() { // ================================================================================================ AccumulateCommand::~AccumulateCommand() { - // Drop the kernel-names owner pin taken in the constructor (e.g. the - // GraphExec). Done first so it always runs, regardless of the owns_hw_events_ - // early return. Releasing here -- after ReportActivity() has read the names -- - // is what makes the borrowed kernel-name strings safe without copying them. - if (kernelNamesOwner_ != nullptr) { - kernelNamesOwner_->release(); - kernelNamesOwner_ = nullptr; - } // When an external owner (e.g. the graph signal pool) reclaims the signals, // do not destroy them here. if (!owns_hw_events_) { diff --git a/projects/clr/rocclr/platform/command.hpp b/projects/clr/rocclr/platform/command.hpp index 3c349dd78eef..0ca6c1ff0d85 100644 --- a/projects/clr/rocclr/platform/command.hpp +++ b/projects/clr/rocclr/platform/command.hpp @@ -1673,32 +1673,15 @@ class Marker : public Command { class AccumulateCommand : public Command { private: - //! Kernel names and timestamps list for activity profiling - std::vector kernelNames_; - const std::vector* kernelNamesRef_ = nullptr; - //! Optional owner of the borrowed kernel-name strings (e.g. the GraphExec - //! whose nodes own them). Retained while this command lives so the strings - //! outlive ReportActivity(), which runs at the end of setStatus(CL_COMPLETE) - //! -- after OnLaunchComplete() may have dropped the launch's reference. This - //! ties the strings' lifetime to the consumer (this command) rather than to - //! the graph launch, with no string copies. Set via the constructor. - ReferenceCountedObject* kernelNamesOwner_ = nullptr; - - public: - //! Sentinel queue index meaning "unknown; fall back to the command's queue". - static constexpr uint32_t kInvalidQueueIndex = 0xFFFFFFFFu; - - //! One kernel dispatch's GPU timing plus the queue (vGPU index) it ran on. - //! Graphs span multiple streams under one command, so the queue is tracked - //! per timestamp rather than taken from the command's queue. - struct KernelTimestamp { - uint64_t start; - uint64_t end; - uint32_t queue_index; - }; - - private: - std::vector tsList_; + //! 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. + std::vector> timestamps_; //! 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, @@ -1706,21 +1689,9 @@ class AccumulateCommand : public Command { bool owns_hw_events_ = true; public: - //! Create a new accumulate command. kernelNamesOwner, when given, is the - //! object that owns the borrowed kernel-name strings (e.g. the GraphExec); - //! it is retained for the command's whole lifetime and released in the - //! destructor, so the borrowed strings stay valid through ReportActivity() - //! even after OnLaunchComplete() drops the launch's reference -- with no - //! string copies. AccumulateCommand(HostQueue& queue, const EventWaitList& eventWaitList = nullWaitList, - const Event* waitingEvent = nullptr, - ReferenceCountedObject* kernelNamesOwner = nullptr) - : Command(queue, CL_COMMAND_TASK, eventWaitList, 0, waitingEvent), - kernelNamesOwner_(kernelNamesOwner) { - if (kernelNamesOwner_ != nullptr) { - kernelNamesOwner_->retain(); - } - } + const Event* waitingEvent = nullptr) + : Command(queue, CL_COMMAND_TASK, eventWaitList, 0, waitingEvent) {} //! Destructor - release all retained HW events virtual ~AccumulateCommand(); @@ -1748,35 +1719,20 @@ class AccumulateCommand : public Command { //! them across launches instead. void setOwnsHwEvents(bool owns) { owns_hw_events_ = owns; } - //! Add kernel name to the list if available - void addKernelName(const std::string* kernelName) { kernelNames_.push_back(kernelName); } - - //! Add multiple kernel names in bulk - void addKernelNames(const std::vector& kernelNames) { - kernelNames_.insert(kernelNames_.end(), kernelNames.begin(), kernelNames.end()); - } - - //! Set kernel names by reference (cheap; borrows the caller's vector and - //! strings). Safe only while that storage outlives this command. Used on the - //! hot path when no profiler is active, where the names are never read. - void setKernelNamesRef(const std::vector* kernelNames) { - kernelNamesRef_ = kernelNames; - } + //! 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 kernel timestamp to the list if available. queue_index is the vGPU - //! index of the stream the kernel ran on (kInvalidQueueIndex when unknown). - void addTimestamps(uint64_t startTs, uint64_t endTs, - uint32_t queue_index = kInvalidQueueIndex) { - tsList_.push_back(KernelTimestamp{startTs, endTs, queue_index}); + //! Add GPU timestamps for one dispatch slot (called at signal completion). + void addTimestamps(uint64_t startTs, uint64_t endTs) { + timestamps_.push_back({startTs, endTs}); } - //! Return the kernel names (pointers to stable strings, no copies) - const std::vector& getKernelNames() const { - return kernelNamesRef_ != nullptr ? *kernelNamesRef_ : kernelNames_; - } + //! Return kernel name pointers (one per dispatch slot) + const std::vector& getKernelNames() const { return kernelNames_; } - //! Return the kernel timestamps - const std::vector& getTimestamps() const { return tsList_; } + //! Return GPU timestamps (one per dispatch slot, populated at signal completion) + const std::vector>& getTimestamps() const { return timestamps_; } //! The command implementation virtual void submit(device::VirtualDevice& device) { device.submitAccumulate(*this); } diff --git a/projects/clr/rocclr/platform/commandqueue.hpp b/projects/clr/rocclr/platform/commandqueue.hpp index dc26bc1418e9..2c3543fd61e2 100644 --- a/projects/clr/rocclr/platform/commandqueue.hpp +++ b/projects/clr/rocclr/platform/commandqueue.hpp @@ -270,22 +270,18 @@ class HostQueue : public CommandQueue { // an invalid access command->retain(); - // Extra retain for the last command - command->retain(); - - // Update lastEnqueueCommand_ before releasing the old one to prevent - // use-after-free if the release triggers re-entrant access to this queue - // (e.g. ~GraphExec calling finish() on graph-internal streams during - // cascading destruction from the old command's release). - Command* prevLastEnqueue = lastEnqueueCommand_; - lastEnqueueCommand_ = command; - - if (prevLastEnqueue != nullptr) { - prevLastEnqueue->release(); + // Release the last command in the batch + if (lastEnqueueCommand_ != nullptr) { + lastEnqueueCommand_->release(); } else { // The queue becomes active. Add it to the set of activeQueues. device_.addToActiveQueues(this); } + + // Extra retain for the last command + command->retain(); + + lastEnqueueCommand_ = command; } //! Flushes submitted commands if the batch size significantly grew