From 194cdecfc2a614294f576a472836bb0c664d5843 Mon Sep 17 00:00:00 2001 From: "Vivek, Kale" Date: Fri, 17 Jul 2026 21:10:11 +0000 Subject: [PATCH 01/21] refactor(rocprofiler-sdk): migrate SPM off the per-queue callback registry Isolates the SPM slice of the callback-removal effort (reference PRs #8730 / #8586) into a small, single-service change, per review guidance to land callback removal one service at a time. Independent of kernel replay. SPM no longer registers a queue_callbacks_t with the HSA queue controller. Instead the HSA write interceptor and async signal handler call explicit free functions: - spm::write_hook / signal_completion_hook / is_any_active (new spm/queue_hooks.{hpp,cpp}) iterating active dispatch_spm contexts and calling the existing pre_kernel_call / post_kernel_call - hsa/queue_hooks/client_ids.hpp: stable inst_pkt producer tags The per-queue callback registry is intentionally retained for counters, thread-trace and pc-sampling; those migrate in follow-up PRs. The WriteInterceptor gate and should_batch predicate detect SPM via is_any_active() (it no longer contributes to queue.get_notifiers()). Removes the now-unused queue_id from spm_counter_callback_info and updates spm tests off iterate_callbacks. Co-authored-by: Cursor --- .../lib/rocprofiler-sdk/hsa/CMakeLists.txt | 1 + .../source/lib/rocprofiler-sdk/hsa/queue.cpp | 29 +++++- .../hsa/queue_hooks/CMakeLists.txt | 9 ++ .../hsa/queue_hooks/client_ids.hpp | 43 +++++++++ .../lib/rocprofiler-sdk/spm/CMakeLists.txt | 5 +- .../source/lib/rocprofiler-sdk/spm/core.cpp | 58 +---------- .../source/lib/rocprofiler-sdk/spm/core.hpp | 3 - .../lib/rocprofiler-sdk/spm/queue_hooks.cpp | 95 +++++++++++++++++++ .../lib/rocprofiler-sdk/spm/queue_hooks.hpp | 66 +++++++++++++ .../rocprofiler-sdk/spm/tests/CMakeLists.txt | 3 +- .../lib/rocprofiler-sdk/spm/tests/core.cpp | 54 +---------- .../spm/tests/queue_hooks_test.cpp | 37 ++++++++ 12 files changed, 294 insertions(+), 109 deletions(-) create mode 100644 projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/CMakeLists.txt create mode 100644 projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp create mode 100644 projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp create mode 100644 projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp create mode 100644 projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/CMakeLists.txt b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/CMakeLists.txt index ab27514b1822..c085f2c7f7cd 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/CMakeLists.txt +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/CMakeLists.txt @@ -39,3 +39,4 @@ target_sources(rocprofiler-sdk-object-library PRIVATE ${ROCPROFILER_LIB_HSA_SOUR ${ROCPROFILER_LIB_HSA_HEADERS}) add_subdirectory(details) +add_subdirectory(queue_hooks) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp index 63561e628687..6c3e024c36f7 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp @@ -40,6 +40,7 @@ #include "lib/rocprofiler-sdk/pc_sampling/hsa_adapter.hpp" #include "lib/rocprofiler-sdk/pc_sampling/service.hpp" #include "lib/rocprofiler-sdk/registration.hpp" +#include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" #include "lib/rocprofiler-sdk/tracing/tracing.hpp" #include @@ -168,6 +169,15 @@ AsyncSignalHandler(hsa_signal_value_t /*signal_v*/, void* data) } }); + // SPM completion is migrated off the callback registry (see WriteInterceptor); invoke + // it explicitly here. + spm::signal_completion_hook(queue_info_session.queue, + packet.kernel_packet, + _session, + packet, + packet.instrumentation_packets, + dispatch_time); + if(packet.is_serialized) { CHECK_NOTNULL(hsa::get_queue_controller()) @@ -305,8 +315,10 @@ WriteInterceptor(const void* packets, auto* gls = ::rocprofiler::hip::graph::current_launch_state(); const bool graph_launch_active = (gls != nullptr); + // SPM no longer registers a queue-controller callback, so it does not count toward + // get_notifiers(); detect it explicitly so an SPM-only run still enters the interceptor. const bool no_real_consumers = - (queue.get_notifiers() == 0 && + (queue.get_notifiers() == 0 && !spm::is_any_active() && context::get_active_contexts(full_packet_instrumentation_context_filter).empty()); if(pkt_count == 0 || (no_real_consumers && !graph_launch_active)) @@ -617,6 +629,18 @@ WriteInterceptor(const void* packets, } }); + // SPM is migrated off the per-queue callback registry: call its hook explicitly + // (the other services still flow through signal_callback above). + spm::write_hook(queue, + kernel_packet, + kernel_id, + dispatch_id, + &_packet_data.user_data, + _packet_data.tracing_data.external_correlation_ids, + corr_id, + _packet_data.instrumentation_packets, + _packet_data.is_serialized); + bool inserted_before = false; if(_packet_data.is_serialized) { @@ -751,6 +775,9 @@ WriteInterceptor(const void* packets, } }); + // SPM requires per-packet mode; it no longer participates in the registry above. + if(spm::is_any_active()) should_batch_packets = false; + if(should_batch_packets) { ROCP_TRACE_IF(pkt_count > 1) << fmt::format( diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/CMakeLists.txt b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/CMakeLists.txt new file mode 100644 index 000000000000..5f5e8f8481f2 --- /dev/null +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/CMakeLists.txt @@ -0,0 +1,9 @@ +# +# Shared, header-only client-id tags used by the per-service queue hooks. +# +set(ROCPROFILER_LIB_HSA_QUEUE_HOOKS_SOURCES) +set(ROCPROFILER_LIB_HSA_QUEUE_HOOKS_HEADERS client_ids.hpp) + +target_sources( + rocprofiler-sdk-object-library PRIVATE ${ROCPROFILER_LIB_HSA_QUEUE_HOOKS_SOURCES} + ${ROCPROFILER_LIB_HSA_QUEUE_HOOKS_HEADERS}) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp new file mode 100644 index 000000000000..9e4879b262bc --- /dev/null +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp @@ -0,0 +1,43 @@ +// MIT License +// +// Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +#pragma once + +#include + +namespace rocprofiler +{ +namespace hsa +{ +namespace queue_hooks +{ +// Tags identifying the producer subsystem of each inst_pkt_t entry. +// Only distinctness matters; values are fixed for test-order stability. +// Services are migrated off the per-queue callback registry one at a time; the +// unused ids are reserved so the tag values stay stable across those PRs. +constexpr int64_t COUNTERS_CLIENT_ID = 1; +constexpr int64_t THREAD_TRACE_CLIENT_ID = 2; +constexpr int64_t PC_SAMPLING_CLIENT_ID = 3; +constexpr int64_t SPM_CLIENT_ID = 4; +} // namespace queue_hooks +} // namespace hsa +} // namespace rocprofiler diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/CMakeLists.txt b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/CMakeLists.txt index 5b3e8eaa7f83..e2ce72848105 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/CMakeLists.txt +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/CMakeLists.txt @@ -1,6 +1,7 @@ set(ROCPROFILER_LIB_SPM_SOURCES core.cpp service.cpp decode.cpp interface.cpp - dispatch_handlers.cpp) -set(ROCPROFILER_LIB_SPM_HEADERS core.hpp interface.hpp decode.hpp dispatch_handlers.hpp) + dispatch_handlers.cpp queue_hooks.cpp) +set(ROCPROFILER_LIB_SPM_HEADERS core.hpp interface.hpp decode.hpp dispatch_handlers.hpp + queue_hooks.hpp) target_sources(rocprofiler-sdk-object-library PRIVATE ${ROCPROFILER_LIB_SPM_SOURCES} ${ROCPROFILER_LIB_SPM_HEADERS}) if(ROCPROFILER_BUILD_TESTS) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp index 35a85fd07040..16668f2fdbbe 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp @@ -260,56 +260,14 @@ start_context(const context::context* ctx) auto* controller = hsa::get_queue_controller(); - bool already_enabled = true; CHECK_NOTNULL(controller)->enable_serialization(); ctx->dispatch_spm->enabled.wlock([&](auto& enabled) { if(enabled) return; - already_enabled = false; - enabled = true; + enabled = true; }); - if(!already_enabled) - { - // Insert our callbacks into HSA Interceptor. This - // turns on counter instrumentation. - for(auto& cb : ctx->dispatch_spm->callbacks) - { - if(cb->queue_id != rocprofiler::hsa::ClientID{-1}) continue; - cb->queue_id = controller->add_callback( - std::nullopt, - hsa::queue_callbacks_t{ - .batch_packets = []() { return false; }, - .write_interceptor = - [=](const hsa::Queue& q, - const hsa::rocprofiler_packet& kern_pkt, - rocprofiler_kernel_id_t kernel_id, - rocprofiler_dispatch_id_t dispatch_id, - rocprofiler_user_data_t* user_data, - const hsa::queue_info_session_t::external_corr_id_map_t& - extern_corr_ids, - const context::correlation_id* correlation_id) { - return pre_kernel_call(ctx, - cb, - q, - kern_pkt, - kernel_id, - dispatch_id, - user_data, - extern_corr_ids, - correlation_id); - }, - .signal_completion = - [=](const hsa::Queue& /* q */, - const hsa::rocprofiler_packet& /* kern_pkt */, - std::shared_ptr& session, - hsa::packet_data_t& /* pkt_data */, - inst_pkt_t& aql, - kernel_dispatch::profiling_time dispatch_time) { - post_kernel_call(ctx, cb, session, aql, dispatch_time); - }}); - } - } - + // SPM no longer registers a per-queue callback with the queue controller; the HSA write + // interceptor calls spm::write_hook / signal_completion_hook directly (see hsa/queue.cpp). return ROCPROFILER_STATUS_SUCCESS; } @@ -334,14 +292,8 @@ stop_context(const context::context* ctx) { hsa::queue_controller_sync(); controller->disable_serialization(); - for(auto& cb : ctx->dispatch_spm->callbacks) - { - if(cb->queue_id != rocprofiler::hsa::ClientID{-1}) - { - controller->remove_callback(cb->queue_id); - cb->queue_id = rocprofiler::hsa::ClientID{-1}; - } - } + // No per-queue callback to remove; spm::write_hook no-ops once dispatch_spm is + // disabled above. } } diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp index 15d022430ccc..eaedee3f9bc3 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp @@ -75,9 +75,6 @@ struct spm_counter_callback_info void* callback_args{nullptr}; // Link to the context this is associated with rocprofiler_context_id_t context{.handle = 0}; - // HSA Queue ClientID. This is an ID we get when we insert a callback into the - // HSA queue interceptor. This ID can be used to disable the callback. - rocprofiler::hsa::ClientID queue_id{-1}; // Buffer to use for storing counter data. Used if callback is not set. std::optional buffer; // Link to the internal context this is associated with diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp new file mode 100644 index 000000000000..7e4f841076bf --- /dev/null +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp @@ -0,0 +1,95 @@ +// MIT License +// +// Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +#include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" +#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" +#include "lib/rocprofiler-sdk/spm/dispatch_handlers.hpp" + +namespace rocprofiler +{ +namespace spm +{ +namespace +{ +auto +active_spm_contexts_filter() +{ + return [](const context::context* ctx) -> bool { return ctx && ctx->dispatch_spm != nullptr; }; +} +} // namespace + +void +write_hook(const hsa::Queue& queue, + const hsa::rocprofiler_packet& kernel_packet, + rocprofiler_kernel_id_t kernel_id, + rocprofiler_dispatch_id_t dispatch_id, + rocprofiler_user_data_t* user_data, + const hsa::queue_info_session_t::external_corr_id_map_t& ext_corr_ids, + const context::correlation_id* correlation_id, + hsa::inst_pkt_t& inst_pkt, + bool& is_serialized) +{ + auto active = context::get_active_contexts(active_spm_contexts_filter()); + for(const auto* ctx : active) + { + for(auto& cb : ctx->dispatch_spm->callbacks) + { + auto [packet, bSerial] = pre_kernel_call(ctx, + cb, + queue, + kernel_packet, + kernel_id, + dispatch_id, + user_data, + ext_corr_ids, + correlation_id); + if(packet) inst_pkt.emplace_back(std::move(packet), hsa::queue_hooks::SPM_CLIENT_ID); + is_serialized |= bSerial; + } + } +} + +void +signal_completion_hook(const hsa::Queue& /*queue*/, + const hsa::rocprofiler_packet& /*kernel_packet*/, + std::shared_ptr& session, + hsa::packet_data_t& /*packet*/, + hsa::inst_pkt_t& inst_pkt, + kernel_dispatch::profiling_time dispatch_time) +{ + auto active = context::get_active_contexts(active_spm_contexts_filter()); + for(const auto* ctx : active) + { + for(auto& cb : ctx->dispatch_spm->callbacks) + { + post_kernel_call(ctx, cb, session, inst_pkt, dispatch_time); + } + } +} + +bool +is_any_active() +{ + return !context::get_active_contexts(active_spm_contexts_filter()).empty(); +} +} // namespace spm +} // namespace rocprofiler diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp new file mode 100644 index 000000000000..34b5f2cba824 --- /dev/null +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp @@ -0,0 +1,66 @@ +// MIT License +// +// Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +#pragma once + +#include "lib/rocprofiler-sdk/context/context.hpp" +#include "lib/rocprofiler-sdk/hsa/queue.hpp" +#include "lib/rocprofiler-sdk/hsa/queue_info_session.hpp" +#include "lib/rocprofiler-sdk/hsa/rocprofiler_packet.hpp" +#include "lib/rocprofiler-sdk/kernel_dispatch/profiling_time.hpp" + +#include + +namespace rocprofiler +{ +namespace spm +{ +// Explicit replacement for the SPM per-queue callback that used to be registered +// with the HSA queue controller. Iterates active dispatch_spm contexts and calls +// each callback's pre_kernel_call; appends produced packets to inst_pkt, +// OR-folding each callback's serialize flag into is_serialized. +void +write_hook(const hsa::Queue& queue, + const hsa::rocprofiler_packet& kernel_packet, + rocprofiler_kernel_id_t kernel_id, + rocprofiler_dispatch_id_t dispatch_id, + rocprofiler_user_data_t* user_data, + const hsa::queue_info_session_t::external_corr_id_map_t& ext_corr_ids, + const context::correlation_id* correlation_id, + hsa::inst_pkt_t& inst_pkt, + bool& is_serialized); + +// Explicit replacement for the SPM completion callback. Iterates active +// dispatch_spm contexts and calls each callback's post_kernel_call. +void +signal_completion_hook(const hsa::Queue& queue, + const hsa::rocprofiler_packet& kernel_packet, + std::shared_ptr& session, + hsa::packet_data_t& packet, + hsa::inst_pkt_t& inst_pkt, + kernel_dispatch::profiling_time dispatch_time); + +// True if any context currently has dispatch SPM active. +bool +is_any_active(); +} // namespace spm +} // namespace rocprofiler diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/CMakeLists.txt b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/CMakeLists.txt index bdd37b5dd8a4..5e5a4fe100c4 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/CMakeLists.txt +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/CMakeLists.txt @@ -14,7 +14,8 @@ find_program( set(_ROCPROFILER_SHARE_DIR "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_DATAROOTDIR}/${PACKAGE_NAME}") -set(ROCPROFILER_LIB_SPM_COUNTER_TEST_SOURCES core.cpp spm_memory_pool_test.cpp) +set(ROCPROFILER_LIB_SPM_COUNTER_TEST_SOURCES core.cpp spm_memory_pool_test.cpp + queue_hooks_test.cpp) add_executable(spm-counter-test) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index 23a859133e87..38a2006a83c1 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -591,19 +591,12 @@ TEST(spm_core, start_stop_callback_ctx) EXPECT_EQ(ctx.dispatch_spm->callbacks.at(0)->record_callback_args, (void*) 0x54321); EXPECT_EQ(ctx.dispatch_spm->callbacks.at(0)->context.handle, get_client_ctx().handle); + // #8730-style: SPM no longer registers a per-queue callback; activeness is observable via + // the context enabled flag (and spm::is_any_active()). bool found = false; ctx.dispatch_spm->enabled.rlock([&](const auto& data) { found = data; }); EXPECT_TRUE(found); - found = false; - hsa::get_queue_controller()->iterate_callbacks([&](auto cid, const auto&) { - if(cid == ctx.dispatch_spm->callbacks.at(0)->queue_id) - { - found = true; - } - }); - EXPECT_TRUE(found); - /** * Check if context can be disabled correctly */ @@ -659,19 +652,12 @@ TEST(spm_core, start_stop_buffered_ctx) ASSERT_TRUE(ctx.dispatch_spm->callbacks.at(0)->buffer); EXPECT_EQ(ctx.dispatch_spm->callbacks.at(0)->buffer->handle, opt_buff_id.handle); + // #8730-style: SPM no longer registers a per-queue callback; activeness is observable via + // the context enabled flag (and spm::is_any_active()). bool found = false; ctx.dispatch_spm->enabled.rlock([&](const auto& data) { found = data; }); EXPECT_TRUE(found); - found = false; - hsa::get_queue_controller()->iterate_callbacks([&](auto cid, const auto&) { - if(cid == ctx.dispatch_spm->callbacks.at(0)->queue_id) - { - found = true; - } - }); - EXPECT_TRUE(found); - /** * Check if context can be disabled correctly */ @@ -942,29 +928,13 @@ TEST(spm_core, stop_context_removes_callbacks) ASSERT_TRUE(ctx.dispatch_spm); ASSERT_EQ(ctx.dispatch_spm->callbacks.size(), 1); - auto pre_stop_queue_id = ctx.dispatch_spm->callbacks.at(0)->queue_id; - bool found = false; - hsa::get_queue_controller()->iterate_callbacks([&](auto cid, const auto&) { - if(cid == pre_stop_queue_id) found = true; - }); - EXPECT_TRUE(found) << "Callback should be registered after start"; - - // Stop exercises queue_controller_sync + remove_callback + // Stop exercises queue_controller_sync + context teardown. ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); bool enabled = true; ctx.dispatch_spm->enabled.rlock([&](const auto& data) { enabled = data; }); EXPECT_FALSE(enabled); - EXPECT_EQ(ctx.dispatch_spm->callbacks.at(0)->queue_id, hsa::ClientID{-1}) - << "queue_id should be reset to -1 after stop"; - - found = false; - hsa::get_queue_controller()->iterate_callbacks([&](auto cid, const auto&) { - if(cid == pre_stop_queue_id) found = true; - }); - EXPECT_FALSE(found) << "Callback should be removed from queue controller after stop"; - registration::set_init_status(1); registration::finalize(); context::pop_client(1); @@ -997,8 +967,6 @@ TEST(spm_core, stop_context_sync_and_restart) auto& ctx = *ctx_p; ASSERT_TRUE(ctx.dispatch_spm); - auto pre_stop_queue_id = ctx.dispatch_spm->callbacks.at(0)->queue_id; - ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); // Restart to verify queue controller is in a clean state after sync + teardown @@ -1008,18 +976,6 @@ TEST(spm_core, stop_context_sync_and_restart) ctx.dispatch_spm->enabled.rlock([&](const auto& data) { enabled = data; }); EXPECT_TRUE(enabled) << "Context should be enabled after restart"; - auto post_restart_queue_id = ctx.dispatch_spm->callbacks.at(0)->queue_id; - EXPECT_NE(post_restart_queue_id, hsa::ClientID{-1}) - << "Restarted context should have a valid queue_id"; - EXPECT_NE(post_restart_queue_id, pre_stop_queue_id) - << "Restarted context should get a fresh callback ID"; - - bool found = false; - hsa::get_queue_controller()->iterate_callbacks([&](auto cid, const auto&) { - if(cid == post_restart_queue_id) found = true; - }); - EXPECT_TRUE(found) << "New callback should be registered after restart"; - ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context final"); registration::set_init_status(1); diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp new file mode 100644 index 000000000000..ce9be8541d35 --- /dev/null +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp @@ -0,0 +1,37 @@ +// MIT License +// +// Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +#include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" + +#include + +namespace +{ +// is_any_active replaces the queue.get_notifiers() signal that the old per-queue +// callback registration used to provide to the HSA write interceptor gate. With +// no SPM context active, it must report inactive. This requires no GPU / HSA +// runtime. +TEST(spm_queue_hooks, is_any_active_false_when_no_context_active) +{ + EXPECT_FALSE(rocprofiler::spm::is_any_active()); +} +} // namespace From 50bc272e6971a8cf93ca241a7cee7b55cd047111 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 30 Jul 2026 04:43:44 +0000 Subject: [PATCH 02/21] fix(rocprofiler-sdk): route SPM completions after stop_context Address review feedback on #8887 with the same completion-routing and stop_context lifecycle fixes applied for counters in #8891. signal_completion_hook iterates registered SPM contexts; stop_context stops queue-interposed services before deactivating the context slot. Adds queue_hooks regression test for stop-then-complete ordering. Co-authored-by: Vivek Kale --- .../lib/rocprofiler-sdk/context/context.cpp | 26 ++-- .../lib/rocprofiler-sdk/spm/queue_hooks.cpp | 13 +- .../lib/rocprofiler-sdk/spm/queue_hooks.hpp | 6 +- .../lib/rocprofiler-sdk/spm/tests/core.cpp | 128 ++++++++++++++++++ 4 files changed, 154 insertions(+), 19 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp index 0bfdeabc7387..75b2d4efb185 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp @@ -368,6 +368,20 @@ stop_context(rocprofiler_context_id_t idx) const context* _expected = itr.load(std::memory_order_acquire); if(_expected && _expected->context_idx == idx.handle) { + // Stop queue-interposed services while the context is still in the active list so + // write_hook can coordinate serialized->unserialized transitions during drain. + if(_expected->dispatch_counter_collection) + { + rocprofiler::counters::stop_context(const_cast(_expected)); + } + + if(_expected->dispatch_spm) + rocprofiler::spm::stop_context(const_cast(_expected)); + + if(_expected->device_thread_trace) _expected->device_thread_trace->stop_context(); + if(_expected->dispatch_thread_trace) + _expected->dispatch_thread_trace->stop_context(); + bool success = itr.compare_exchange_strong(_expected, nullptr); if(success) @@ -375,18 +389,6 @@ stop_context(rocprofiler_context_id_t idx) auto nactive = get_num_active_contexts().load(std::memory_order_acquire); if(nactive > 0) get_num_active_contexts().fetch_sub(1, std::memory_order_release); - if(_expected->dispatch_counter_collection) - { - rocprofiler::counters::stop_context(const_cast(_expected)); - } - - if(_expected->dispatch_spm) - rocprofiler::spm::stop_context(const_cast(_expected)); - - if(_expected->device_thread_trace) _expected->device_thread_trace->stop_context(); - if(_expected->dispatch_thread_trace) - _expected->dispatch_thread_trace->stop_context(); - rocprofiler::hsa::queue_interposition:: notify_queue_interposition_consumer_context_stopped(_expected); diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp index 7e4f841076bf..7ab8b17888df 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp @@ -31,7 +31,7 @@ namespace spm namespace { auto -active_spm_contexts_filter() +spm_contexts_filter() { return [](const context::context* ctx) -> bool { return ctx && ctx->dispatch_spm != nullptr; }; } @@ -48,7 +48,7 @@ write_hook(const hsa::Queue& queue, hsa::inst_pkt_t& inst_pkt, bool& is_serialized) { - auto active = context::get_active_contexts(active_spm_contexts_filter()); + auto active = context::get_active_contexts(spm_contexts_filter()); for(const auto* ctx : active) { for(auto& cb : ctx->dispatch_spm->callbacks) @@ -76,8 +76,11 @@ signal_completion_hook(const hsa::Queue& /*queue*/, hsa::inst_pkt_t& inst_pkt, kernel_dispatch::profiling_time dispatch_time) { - auto active = context::get_active_contexts(active_spm_contexts_filter()); - for(const auto* ctx : active) + // Route by packet provenance, not current activeness: post_kernel_call self-filters via + // packet_return_map, so in-flight dispatches still complete after stop_context removes the + // context from the active list. + auto contexts = context::get_registered_contexts(spm_contexts_filter()); + for(const auto* ctx : contexts) { for(auto& cb : ctx->dispatch_spm->callbacks) { @@ -89,7 +92,7 @@ signal_completion_hook(const hsa::Queue& /*queue*/, bool is_any_active() { - return !context::get_active_contexts(active_spm_contexts_filter()).empty(); + return !context::get_active_contexts(spm_contexts_filter()).empty(); } } // namespace spm } // namespace rocprofiler diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp index 34b5f2cba824..ef89e5e9c10a 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp @@ -49,8 +49,10 @@ write_hook(const hsa::Queue& queue, hsa::inst_pkt_t& inst_pkt, bool& is_serialized); -// Explicit replacement for the SPM completion callback. Iterates active -// dispatch_spm contexts and calls each callback's post_kernel_call. +// Explicit replacement for the SPM completion callback. Iterates registered +// dispatch_spm contexts (not only active ones) and calls each callback's +// post_kernel_call; post_kernel_call self-filters via packet_return_map so +// in-flight dispatches still complete after stop_context. void signal_completion_hook(const hsa::Queue& queue, const hsa::rocprofiler_packet& kernel_packet, diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index 38a2006a83c1..330f724a1575 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -33,6 +33,8 @@ #include "lib/rocprofiler-sdk/registration.hpp" #include "lib/rocprofiler-sdk/spm/decode.hpp" #include "lib/rocprofiler-sdk/spm/dispatch_handlers.hpp" +#include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" +#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include #include @@ -983,3 +985,129 @@ TEST(spm_core, stop_context_sync_and_restart) context::pop_client(1); set_client_ctx(get_client_ctx()); } + +// Regression for callback-registry removal (#8887): a dispatch enqueued while the SPM context +// is active must still complete (and drain packet_return_map) when stop_context runs before the +// GPU completion callback. Exercises the queue_hooks routing layer, not pre/post_kernel_call directly. +TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) +{ + rocprofiler::common::set_env("ROCPROFILER_SPM_BETA_ENABLED", true); + ASSERT_EQ(hsa_init(), HSA_STATUS_SUCCESS); + test_init(); + + registration::init_logging(); + registration::set_init_status(-1); + context::push_client(1); + + ROCPROFILER_CALL(rocprofiler_create_context(&get_client_ctx()), "context creation failed"); + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service(get_client_ctx(), + user_dispatch_cb, + nullptr, + null_record_callback, + nullptr), + "Could not setup SPM service"); + ROCPROFILER_CALL(rocprofiler_start_context(get_client_ctx()), "start context"); + + auto* ctx_p = context::get_mutable_registered_context(get_client_ctx()); + ASSERT_TRUE(ctx_p); + ASSERT_TRUE(ctx_p->dispatch_spm); + auto cb_info = ctx_p->dispatch_spm->callbacks.front(); + + ASSERT_TRUE(hsa::get_queue_controller() != nullptr); + auto agents = hsa::get_queue_controller()->get_supported_agents(); + ASSERT_GT(agents.size(), 0); + + bool found_spm_agent = false; + for(const auto& [_, agent] : agents) + { + auto rocp_agent = CHECK_NOTNULL(agent.get_rocp_agent()); + if(!rocp_agent->runtime_visibility.hsa || !rocp_agent->runtime_visibility.hip) continue; + if(!is_spm_supported_arch(agent)) continue; + + found_spm_agent = true; + auto metrics = getSPMMetrics(agent); + for(auto& metric : metrics) + { + if(!metric.expression().empty()) continue; + + expected_dispatch expected = {}; + rocprofiler_counter_id_t id = {.handle = metric.id()}; + std::vector input_params{}; + rocprofiler_spm_parameters_t param{ + .size = sizeof(rocprofiler_spm_parameters_t), + .type = ROCPROFILER_SPM_PARAMETER_TYPE_SAMPLE_INTERVAL_SCLK_CYCLES, + .value = 1200}; + input_params.push_back(¶m); + + ROCPROFILER_CALL(rocprofiler_spm_create_counter_config(agent.get_rocp_agent()->id, + &id, + 1, + input_params.data(), + input_params.size(), + &expected.id), + "Unable to create profile"); + cb_info->callback_args = &expected; + + rocprofiler_queue_id_t qid = {.handle = 1}; + hsa::FakeQueue fq(agent, qid); + hsa::rocprofiler_packet pkt{}; + context::correlation_id corr_id{.internal = 99}; + + expected.queue_id = qid; + expected.agent_id = agent.get_rocp_agent()->id; + expected.kernel_id = 42; + expected.dispatch_id = 7; + + auto user_data = rocprofiler_user_data_t{.value = corr_id.internal}; + auto ret_pkt = spm::pre_kernel_call(ctx_p, + cb_info, + fq, + pkt, + expected.kernel_id, + expected.dispatch_id, + &user_data, + {}, + &corr_id); + if(!ret_pkt.packet) continue; + + size_t map_size_before_stop = 0; + cb_info->packet_return_map.rlock( + [&](const auto& data) { map_size_before_stop = data.size(); }); + if(map_size_before_stop == 0) continue; + + ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); + EXPECT_FALSE(rocprofiler::spm::is_any_active()); + + auto sess = std::make_shared(hsa::queue_info_session_t{.queue = fq}); + sess->correlation_id = &corr_id; + + spm::inst_pkt_t inst_pkt; + inst_pkt.emplace_back( + std::make_pair(std::move(ret_pkt.packet), hsa::queue_hooks::SPM_CLIENT_ID)); + + spm::signal_completion_hook(fq, + pkt, + sess, + sess->packet_data.emplace_back(), + inst_pkt, + kernel_dispatch::profiling_time{}); + + size_t map_size_after_completion = 1; + cb_info->packet_return_map.rlock( + [&](const auto& data) { map_size_after_completion = data.size(); }); + EXPECT_EQ(map_size_after_completion, 0U) + << "packet_return_map must drain via signal_completion_hook after stop_context"; + + ROCPROFILER_CALL(rocprofiler_spm_destroy_counter_config(expected.id), + "Could not delete profile id"); + registration::set_init_status(1); + registration::finalize(); + context::pop_client(1); + set_client_ctx(get_client_ctx()); + return; + } + } + + if(!found_spm_agent) GTEST_SKIP() << "SPM unavailable"; + FAIL() << "Could not exercise SPM in-flight completion hook path"; +} From d9189aff28848f6a7a728080565965f292906b17 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 30 Jul 2026 22:55:16 +0000 Subject: [PATCH 03/21] refactor(rocprofiler-sdk): drop the GPU drain from spm::stop_context Per review feedback from @SrirackshaNag: the queue_controller_sync() in spm::stop_context was there to handle in-flight dispatches and to avoid dangling callback pointers once the per-queue callbacks were unregistered. Neither reason survives the hook migration. - dangling pointers: nothing is registered any more; signal_completion_hook is a static function that resolves contexts at completion time. - in-flight dispatches: handled by signal_completion_hook routing over registered rather than active contexts (previous commit on this branch). post_kernel_call never gated on the enabled flag, so the completion runs normally. Two further reasons a drain might have been needed, both checked and neither applicable: - serializer transition: profiler_serializer::disable() records the previous state and pushes an hsa_barrier across the queues, and kernel_completion_signal reconciles in-flight dispatches against it. The transition is GPU-ordered and does not require the CPU to observe an idle GPU. - service teardown: spm::stop_context frees no buffer, destroys no profile and stops no hardware, so nothing it touches has a lifetime that ends here. Note the drain was never a guarantee in any case: Queue::sync uses a 5 s hint and only warns on timeout, and _active_kernels counts intercepted dispatches only. Also re-justifies the context::stop_context ordering. It is kept, but not for the reason originally given: with no drain, the point is that disable_serialization() runs before dispatches stop being instrumented, so no dispatch is submitted without serializer packets while the serializer is still enabled. Comments in spm_core.stop_context_sync_and_restart updated to match; the test name is left alone to avoid churn in existing local test invocations. Open question for @SrirackshaNag: the drain also incidentally covered stop_context followed by destroying the counter config. If any tool does that, the guard belongs at spm_destroy_profile rather than at stop. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/context/context.cpp | 7 +++++-- .../source/lib/rocprofiler-sdk/spm/core.cpp | 9 ++++++++- .../source/lib/rocprofiler-sdk/spm/tests/core.cpp | 4 ++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp index 75b2d4efb185..e112423d49e3 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp @@ -368,8 +368,11 @@ stop_context(rocprofiler_context_id_t idx) const context* _expected = itr.load(std::memory_order_acquire); if(_expected && _expected->context_idx == idx.handle) { - // Stop queue-interposed services while the context is still in the active list so - // write_hook can coordinate serialized->unserialized transitions during drain. + // Stop queue-interposed services before clearing the active slot so + // disable_serialization() always runs before dispatches stop being instrumented. + // Clearing the slot first opens the opposite window, in which write_hook sees no + // active context and a dispatch is submitted without serializer packets while the + // serializer is still enabled. if(_expected->dispatch_counter_collection) { rocprofiler::counters::stop_context(const_cast(_expected)); diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp index 16668f2fdbbe..dd58ac66e79c 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp @@ -290,7 +290,14 @@ stop_context(const context::context* ctx) if(controller) { - hsa::queue_controller_sync(); + // No GPU drain here. The sync this replaced existed to (a) avoid dangling callback + // pointers once the per-queue callbacks were unregistered and (b) let in-flight + // dispatches complete. (a) no longer applies -- nothing is registered, and + // signal_completion_hook resolves contexts at completion time. (b) is handled by that + // hook routing over registered rather than active contexts. Neither the serializer + // transition nor anything else in this function requires an idle GPU: + // profiler_serializer::disable() pushes an hsa_barrier carrying the previous state, so + // in-flight serialized dispatches are reconciled GPU-side. controller->disable_serialization(); // No per-queue callback to remove; spm::write_hook no-ops once dispatch_spm is // disabled above. diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index 330f724a1575..e35c8795d3a6 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -930,7 +930,7 @@ TEST(spm_core, stop_context_removes_callbacks) ASSERT_TRUE(ctx.dispatch_spm); ASSERT_EQ(ctx.dispatch_spm->callbacks.size(), 1); - // Stop exercises queue_controller_sync + context teardown. + // Stop disables collection and tears the context down (no GPU drain; see spm::stop_context). ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); bool enabled = true; @@ -971,7 +971,7 @@ TEST(spm_core, stop_context_sync_and_restart) ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); - // Restart to verify queue controller is in a clean state after sync + teardown + // Restart to verify the queue controller is in a clean state after teardown ROCPROFILER_CALL(rocprofiler_start_context(get_client_ctx()), "restart context"); bool enabled = false; From 53ebf48fdc6bbf47cdbab3a169007e581d3b9648 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 31 Jul 2026 22:43:49 +0000 Subject: [PATCH 04/21] docs(rocprofiler-sdk): record what the SPM stop-path fix does and does not address Comment-only. Names the two blocking issues raised in the review of #8887 and how each is handled, so a reader does not have to reconstruct it from the review. spm/queue_hooks.cpp: states the consequence of dropping an in-flight completion -- the DISPATCH_END record, kfd_stop() and barrier-signal cleanup are all skipped and the map entry leaks -- and records why widening to registered contexts stays per-origin: each context's post_kernel_call only claims packets in its own packet_return_map. That matters because, unlike dispatch_counter_collection, there is no conflict guard rejecting a second concurrent dispatch_spm context. spm/core.cpp: marks the drain removal as a deliberate divergence from the review, which asked for the sync to be kept plus an explicit draining state. The serialization window that state would protect is closed instead by context::stop_context running this function before clearing the active slot, so disable_serialization happens while the enter hook can still see the context. The two approaches are not obviously equivalent, so the reasoning is recorded rather than left implicit, and it should be revisited if that ordering changes. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/spm/core.cpp | 9 +++++++++ .../source/lib/rocprofiler-sdk/spm/queue_hooks.cpp | 9 ++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp index dd58ac66e79c..e479bab9e11f 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp @@ -298,6 +298,15 @@ stop_context(const context::context* ctx) // transition nor anything else in this function requires an idle GPU: // profiler_serializer::disable() pushes an hsa_barrier carrying the previous state, so // in-flight serialized dispatches are reconciled GPU-side. + // + // This is a deliberate divergence from one review of #8887, which asked instead for the + // sync to be kept and the service held visible to the enter hook through an explicit + // "draining" state until sync and disable_serialization complete. The serialization window + // that state would protect is closed differently here: context::stop_context now runs this + // function before clearing the active slot, so disable_serialization happens while the + // enter hook can still see the context, and no dispatch is submitted unserialized while the + // serializer is enabled. Recorded here because the two approaches are not obviously + // equivalent and the choice should be revisited if that ordering changes. controller->disable_serialization(); // No per-queue callback to remove; spm::write_hook no-ops once dispatch_spm is // disabled above. diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp index 7ab8b17888df..9c893e7302ac 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp @@ -78,7 +78,14 @@ signal_completion_hook(const hsa::Queue& /*queue*/, { // Route by packet provenance, not current activeness: post_kernel_call self-filters via // packet_return_map, so in-flight dispatches still complete after stop_context removes the - // context from the active list. + // context from the active list. Without this, a dispatch in flight at stop skips the + // DISPATCH_END record, kfd_stop() and the barrier-signal cleanup, and leaks its map entry. + // + // Iterating registered contexts widens the candidate set, but routing stays per-origin because + // each context's post_kernel_call only claims packets present in its own packet_return_map. + // That matters because there is no conflict guard rejecting a second concurrent dispatch_spm + // context, unlike dispatch_counter_collection, so more than one SPM context can be registered + // at once. auto contexts = context::get_registered_contexts(spm_contexts_filter()); for(const auto* ctx : contexts) { From 80e98e0c97cc7f693d1e2c0e4eea06d6eaf6eb11 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 31 Jul 2026 22:54:16 +0000 Subject: [PATCH 05/21] fix(rocprofiler-sdk): keep the drain in spm::stop_context, per review priority Restores the queue_controller_sync() an earlier commit on this branch removed. The review of #8887 asked for it to be kept, with SPM held visible to the enter hook and to serialization through a draining window until it and disable_serialization() complete. That takes precedence over the later suggestion to remove it. The removal argued that the sync's two original purposes were addressed by the migration: dangling callback pointers, and letting in-flight dispatches complete. The first is genuinely gone -- nothing is registered any more. The second does not survive scrutiny: provenance routing guarantees that a completion which arrives is delivered, whereas the drain bounds when completions arrive at all, and the rest of the teardown depends on the latter. The comment now records both the request and why the counter-argument is weaker than it appeared, so this is not relitigated silently. No draining flag is introduced, because the visibility half already comes from ordering: context::stop_context calls this function while the context is still in the active list, so throughout the drain the enter hook still reaches pre_kernel_call, whose disabled path deliberately returns serialize=true to coordinate the serialized->unserialized transition, and the slot is cleared only after this function returns. The comment marks that ordering as load-bearing -- if the slot were cleared first, the enter hook would go blind before disable_serialization() ran and a dispatch could be submitted unserialized while the serializer was still enabled. Also brings this branch's own earlier additions into clang-format compliance; they had not been formatted. Verified with clang-format 11.1.0, the version the repo pins, and confined to lines this branch adds -- nothing pre-existing is reformatted. Not compiled here -- no ROCm toolchain in this environment. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/spm/core.cpp | 37 ++++++++++-------- .../lib/rocprofiler-sdk/spm/tests/core.cpp | 38 +++++++++---------- 2 files changed, 40 insertions(+), 35 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp index e479bab9e11f..2c2f581615ec 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp @@ -290,23 +290,28 @@ stop_context(const context::context* ctx) if(controller) { - // No GPU drain here. The sync this replaced existed to (a) avoid dangling callback - // pointers once the per-queue callbacks were unregistered and (b) let in-flight - // dispatches complete. (a) no longer applies -- nothing is registered, and - // signal_completion_hook resolves contexts at completion time. (b) is handled by that - // hook routing over registered rather than active contexts. Neither the serializer - // transition nor anything else in this function requires an idle GPU: - // profiler_serializer::disable() pushes an hsa_barrier carrying the previous state, so - // in-flight serialized dispatches are reconciled GPU-side. + // Drain in-flight dispatches, then disable serialization. The review of #8887 asked for + // this sync to be kept and for SPM to stay visible to the enter hook and to serialization + // through a "draining" window until both this and disable_serialization() complete. // - // This is a deliberate divergence from one review of #8887, which asked instead for the - // sync to be kept and the service held visible to the enter hook through an explicit - // "draining" state until sync and disable_serialization complete. The serialization window - // that state would protect is closed differently here: context::stop_context now runs this - // function before clearing the active slot, so disable_serialization happens while the - // enter hook can still see the context, and no dispatch is submitted unserialized while the - // serializer is enabled. Recorded here because the two approaches are not obviously - // equivalent and the choice should be revisited if that ordering changes. + // The visibility half comes from ordering rather than a separate flag: + // context::stop_context calls this function while the context is still in the active list, + // so for the duration of the drain the enter hook still reaches pre_kernel_call, whose + // disabled path deliberately returns serialize=true to coordinate the + // serialized->unserialized transition. Only once this function returns does the active slot + // get cleared. Introducing a draining flag as well would duplicate that guarantee, but the + // ordering is load-bearing: if the slot were cleared first, the enter hook would go blind + // before disable_serialization() ran and a dispatch could be submitted unserialized while + // the serializer was still enabled. + // + // A removal of this sync was considered on the grounds that its two original purposes -- + // avoiding dangling callback pointers once the per-queue callbacks were unregistered, and + // letting in-flight dispatches complete -- are both addressed by the migration and by + // routing completions over registered contexts. The first is genuinely gone. The second is + // weaker than it looks: provenance routing ensures a completion that arrives is delivered, + // whereas the drain bounds when completions arrive at all, which is what the rest of the + // teardown depends on. Keeping it. + hsa::queue_controller_sync(); controller->disable_serialization(); // No per-queue callback to remove; spm::write_hook no-ops once dispatch_spm is // disabled above. diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index e35c8795d3a6..b5bf02226190 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -29,12 +29,12 @@ #include "lib/rocprofiler-sdk/hsa/hsa.hpp" #include "lib/rocprofiler-sdk/hsa/queue.hpp" #include "lib/rocprofiler-sdk/hsa/queue_controller.hpp" +#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include "lib/rocprofiler-sdk/kernel_dispatch/profiling_time.hpp" #include "lib/rocprofiler-sdk/registration.hpp" #include "lib/rocprofiler-sdk/spm/decode.hpp" #include "lib/rocprofiler-sdk/spm/dispatch_handlers.hpp" #include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" -#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include #include @@ -930,7 +930,7 @@ TEST(spm_core, stop_context_removes_callbacks) ASSERT_TRUE(ctx.dispatch_spm); ASSERT_EQ(ctx.dispatch_spm->callbacks.size(), 1); - // Stop disables collection and tears the context down (no GPU drain; see spm::stop_context). + // Stop disables collection, drains in-flight dispatches, then tears the context down. ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); bool enabled = true; @@ -971,7 +971,7 @@ TEST(spm_core, stop_context_sync_and_restart) ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); - // Restart to verify the queue controller is in a clean state after teardown + // Restart to verify the queue controller is in a clean state after drain + teardown ROCPROFILER_CALL(rocprofiler_start_context(get_client_ctx()), "restart context"); bool enabled = false; @@ -988,7 +988,8 @@ TEST(spm_core, stop_context_sync_and_restart) // Regression for callback-registry removal (#8887): a dispatch enqueued while the SPM context // is active must still complete (and drain packet_return_map) when stop_context runs before the -// GPU completion callback. Exercises the queue_hooks routing layer, not pre/post_kernel_call directly. +// GPU completion callback. Exercises the queue_hooks routing layer, not pre/post_kernel_call +// directly. TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) { rocprofiler::common::set_env("ROCPROFILER_SPM_BETA_ENABLED", true); @@ -1000,12 +1001,10 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) context::push_client(1); ROCPROFILER_CALL(rocprofiler_create_context(&get_client_ctx()), "context creation failed"); - ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service(get_client_ctx(), - user_dispatch_cb, - nullptr, - null_record_callback, - nullptr), - "Could not setup SPM service"); + ROCPROFILER_CALL( + rocprofiler_spm_configure_callback_dispatch_service( + get_client_ctx(), user_dispatch_cb, nullptr, null_record_callback, nullptr), + "Could not setup SPM service"); ROCPROFILER_CALL(rocprofiler_start_context(get_client_ctx()), "start context"); auto* ctx_p = context::get_mutable_registered_context(get_client_ctx()); @@ -1025,15 +1024,15 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) if(!is_spm_supported_arch(agent)) continue; found_spm_agent = true; - auto metrics = getSPMMetrics(agent); + auto metrics = getSPMMetrics(agent); for(auto& metric : metrics) { if(!metric.expression().empty()) continue; - expected_dispatch expected = {}; - rocprofiler_counter_id_t id = {.handle = metric.id()}; + expected_dispatch expected = {}; + rocprofiler_counter_id_t id = {.handle = metric.id()}; std::vector input_params{}; - rocprofiler_spm_parameters_t param{ + rocprofiler_spm_parameters_t param{ .size = sizeof(rocprofiler_spm_parameters_t), .type = ROCPROFILER_SPM_PARAMETER_TYPE_SAMPLE_INTERVAL_SCLK_CYCLES, .value = 1200}; @@ -1048,10 +1047,10 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) "Unable to create profile"); cb_info->callback_args = &expected; - rocprofiler_queue_id_t qid = {.handle = 1}; - hsa::FakeQueue fq(agent, qid); - hsa::rocprofiler_packet pkt{}; - context::correlation_id corr_id{.internal = 99}; + rocprofiler_queue_id_t qid = {.handle = 1}; + hsa::FakeQueue fq(agent, qid); + hsa::rocprofiler_packet pkt{}; + context::correlation_id corr_id{.internal = 99}; expected.queue_id = qid; expected.agent_id = agent.get_rocp_agent()->id; @@ -1078,7 +1077,8 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); EXPECT_FALSE(rocprofiler::spm::is_any_active()); - auto sess = std::make_shared(hsa::queue_info_session_t{.queue = fq}); + auto sess = + std::make_shared(hsa::queue_info_session_t{.queue = fq}); sess->correlation_id = &corr_id; spm::inst_pkt_t inst_pkt; From 1297d315de633170e7a69ca327d1e3294aa61109 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 18 Aug 2026 00:42:56 +0000 Subject: [PATCH 06/21] fix(spm/tests): destroy counter config before early continue to avoid leak Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com> --- .../source/lib/rocprofiler-sdk/spm/tests/core.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index b5bf02226190..b7285159f003 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -1067,12 +1067,22 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) &user_data, {}, &corr_id); - if(!ret_pkt.packet) continue; + if(!ret_pkt.packet) + { + ROCPROFILER_CALL(rocprofiler_spm_destroy_counter_config(expected.id), + "Could not delete profile id"); + continue; + } size_t map_size_before_stop = 0; cb_info->packet_return_map.rlock( [&](const auto& data) { map_size_before_stop = data.size(); }); - if(map_size_before_stop == 0) continue; + if(map_size_before_stop == 0) + { + ROCPROFILER_CALL(rocprofiler_spm_destroy_counter_config(expected.id), + "Could not delete profile id"); + continue; + } ROCPROFILER_CALL(rocprofiler_stop_context(get_client_ctx()), "stop context"); EXPECT_FALSE(rocprofiler::spm::is_any_active()); From 01d66a329b040a5d892e8923ad3b961d139b49f0 Mon Sep 17 00:00:00 2001 From: "Vivek, Kale" Date: Tue, 18 Aug 2026 05:46:51 +0000 Subject: [PATCH 07/21] fix(rocprofiler-sdk): construct SPM in-flight test correlation_id via its default ctor The designated initializer does not compile: correlation_id is not an aggregate. Same pattern #9586 already used for the counters hook-path test, without which Co-authored-by: Cursor #10098 cannot build. --- .../source/lib/rocprofiler-sdk/spm/tests/core.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index b7285159f003..79a73228506f 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -1050,7 +1050,10 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) rocprofiler_queue_id_t qid = {.handle = 1}; hsa::FakeQueue fq(agent, qid); hsa::rocprofiler_packet pkt{}; - context::correlation_id corr_id{.internal = 99}; + // correlation_id declares constructors and holds private ref counters, so it is not an + // aggregate and cannot take a designated initializer. + context::correlation_id corr_id{}; + corr_id.internal = 99; expected.queue_id = qid; expected.agent_id = agent.get_rocp_agent()->id; From cbc6b92735a5f71557853affc78afe61eaa57e1a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 10 Aug 2026 18:42:25 +0000 Subject: [PATCH 08/21] feat(rocprofiler-sdk): scope SPM dispatch collection to agents Apply per-agent scoping from PR #9868 to the SPM callback-removal branch: - Per-agent serialization ref-counting in queue_controller - agents set on spm_dispatch_counter_collection_service with collects_on/intersects - rocprofiler_spm_dispatch_counting_service_set_agents() public API - Agent filtering in spm::write_hook and disjoint SPM context conflict rules Co-authored-by: Vivek Kale --- .../rocprofiler-sdk/experimental/spm.h | 22 +++ .../lib/rocprofiler-sdk/context/context.cpp | 21 +++ .../lib/rocprofiler-sdk/context/context.hpp | 16 +++ .../rocprofiler-sdk/hsa/queue_controller.cpp | 132 ++++++++++++++---- .../rocprofiler-sdk/hsa/queue_controller.hpp | 55 +++++++- .../source/lib/rocprofiler-sdk/spm/core.cpp | 40 +++++- .../source/lib/rocprofiler-sdk/spm/core.hpp | 5 + .../lib/rocprofiler-sdk/spm/queue_hooks.cpp | 10 +- .../lib/rocprofiler-sdk/spm/service.cpp | 11 ++ 9 files changed, 276 insertions(+), 36 deletions(-) diff --git a/projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h b/projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h index f3686b8ccf3f..dc7c66ffe0bd 100644 --- a/projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h +++ b/projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h @@ -315,4 +315,26 @@ rocprofiler_spm_configure_buffer_dispatch_service( rocprofiler_spm_dispatch_counting_service_cb_t callback, void* callback_data_args) ROCPROFILER_API ROCPROFILER_NONNULL(3); +/** + * @brief (experimental) Restrict an SPM dispatch counting service to a set of GPU agents. + * + * By default an SPM dispatch service applies to every GPU agent. This function narrows + * the service so that agents outside @p agents are neither instrumented nor serialized. + * Restricting the agent set also relaxes the SPM context conflict: two contexts may each + * configure an SPM dispatch service and be active at the same time provided their agent + * sets are disjoint. + * + * Must be called after an SPM dispatch service has been configured on @p context_id and + * before the context is started. Passing @p num_agents of zero restores all agents. + * + * @param [in] context_id context id with a configured SPM dispatch service + * @param [in] agents array of GPU agent ids to restrict collection to + * @param [in] num_agents number of entries in @p agents + * @return ::rocprofiler_status_t + */ +ROCPROFILER_SDK_EXPERIMENTAL rocprofiler_status_t +rocprofiler_spm_dispatch_counting_service_set_agents(rocprofiler_context_id_t context_id, + const rocprofiler_agent_id_t* agents, + size_t num_agents) ROCPROFILER_API; + ROCPROFILER_EXTERN_C_FINI diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp index e112423d49e3..3b3f4be5d4d3 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp @@ -106,6 +106,20 @@ get_active_contexts_impl() } } // namespace +bool +spm_dispatch_counter_collection_service::intersects( + const spm_dispatch_counter_collection_service& rhs) const +{ + if(agents.empty() || rhs.agents.empty()) return true; + const auto& small = (agents.size() < rhs.agents.size()) ? agents : rhs.agents; + const auto& large = (agents.size() < rhs.agents.size()) ? rhs.agents : agents; + for(const auto& agent_id : small) + { + if(large.count(agent_id) > 0) return true; + } + return false; +} + context_array_t& get_registered_contexts(context_array_t& data, context_filter_t filter) { @@ -294,6 +308,13 @@ start_context(rocprofiler_context_id_t context_id) // conflicting context return ROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT; } + else if(cfg->dispatch_spm && itr->dispatch_spm && + cfg->dispatch_spm->intersects(*itr->dispatch_spm)) + { + // Two SPM dispatch contexts can run concurrently as long as they target disjoint + // sets of GPU agents. A context with no agent restriction claims every agent. + return ROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT; + } } uint64_t rocp_tot_contexts = get_registered_contexts_impl()->size(); diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp index 73f075f9b148..27332228da29 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.hpp @@ -35,6 +35,8 @@ #include #include +#include +#include #include #include @@ -42,6 +44,7 @@ #include #include #include +#include namespace rocprofiler { @@ -94,10 +97,23 @@ struct spm_dispatch_counter_collection_service // Contains callback information along with other data needed to collect/process // SPM counters. std::vector> callbacks{}; + // GPU agents this context collects on. An empty set means every GPU agent, which is what a + // plain configure_{buffer,callback}_dispatch call produces; + // rocprofiler_spm_dispatch_counting_service_set_agents narrows it. The set is read on the + // dispatch path and when scoping serialization, so it is fixed at configure time and never + // mutated once the context is started. + std::unordered_set agents{}; // A flag to state whether or not the counter set is currently enabled. This is primarily // to protect against multithreaded calls to enable a context (and enabling already enabled // counters). common::Synchronized enabled{false}; + + bool collects_on(rocprofiler_agent_id_t agent_id) const + { + return agents.empty() || agents.count(agent_id) > 0; + } + + bool intersects(const spm_dispatch_counter_collection_service& rhs) const; }; struct device_counting_service diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp index 1a177757e7ad..9444f1c797cc 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp @@ -391,10 +391,19 @@ common::Synchronized& QueueController::serializer(const Queue* queue) { CHECK(queue); + const auto agent_id = queue->get_agent().get_rocp_agent()->id; + + // Read the refcount before taking the serializer lock: update_serialization() acquires + // them in that order, and reversing it here would deadlock. A concurrent enable/disable + // for this agent can therefore race with the creation below, exactly as the previous + // _serialized_enabled load could, and is resolved the same way -- the next transition + // through update_serialization() reaches the entry once it is in the map. + const bool should_serialize = is_serialization_enabled(agent_id); + common::Synchronized* ret = nullptr; _profiler_serializer.ulock( [&](const auto& m) { - if(auto ptr = m.find(queue->get_agent().get_rocp_agent()->id); ptr != m.end()) + if(auto ptr = m.find(agent_id); ptr != m.end()) { ret = ptr->second.get(); return true; @@ -402,10 +411,10 @@ QueueController::serializer(const Queue* queue) return false; }, [&](auto& m) { - ret = m.emplace(queue->get_agent().get_rocp_agent()->id, + ret = m.emplace(agent_id, std::make_shared>()) .first->second.get(); - if(_serialized_enabled.load() == true) + if(should_serialize) { ret->wlock([&](auto& serializer) { serializer.enable({}); }); } @@ -429,47 +438,114 @@ per_dev_map(const QueueController::queue_map_t& _queues_v) }; // namespace void -QueueController::disable_serialization() +QueueController::update_serialization(const agent_handle_set_t& agents, bool enable) { _queues.rlock([&](const queue_map_t& _queues_v) { - _serialized_enabled.store(false); auto pd_map = per_dev_map(_queues_v); - _profiler_serializer.wlock([&](auto& m) { - for(auto& [k, v] : m) + + // Agents whose effective refcount crossed zero in this call; only those get their + // serializer toggled. Collected under the refcount lock, applied under the serializer + // lock. + auto transitioned = std::vector{}; + bool any_enabled = false; + + _serialization_refcount.wlock([&](auto& state) { + // An agent can be covered by `all` and by an explicit entry at the same time, so + // the transition has to be decided by comparing effective counts across the + // update rather than by watching a single counter hit zero. + auto was_enabled = std::unordered_map{}; + _profiler_serializer.rlock([&](const auto& serializers) { + for(const auto& [agent_id, _] : serializers) + was_enabled.emplace(agent_id, state.enabled(agent_id)); + }); + + if(agents.empty()) + { + if(enable) + ++state.all; + else if(state.all > 0) + --state.all; + } + else { - if(auto it = pd_map.find(k); it != pd_map.end()) + for(const auto& agent_id : agents) { - v->wlock([&](auto& serializer) { serializer.disable(it->second); }); + auto& count = state.per_agent[agent_id]; + if(enable) + ++count; + else if(count > 0) + --count; } - else + } + + _profiler_serializer.rlock([&](const auto& serializers) { + for(const auto& [agent_id, _] : serializers) { - v->wlock([&](auto& serializer) { serializer.disable({}); }); + auto itr = was_enabled.find(agent_id); + if(itr == was_enabled.end()) continue; + if(itr->second != state.enabled(agent_id)) + transitioned.emplace_back(agent_id); } + }); + + any_enabled = state.any(); + }); + + _serialized_enabled.store(any_enabled); + + if(transitioned.empty()) return; + + _profiler_serializer.wlock([&](auto& m) { + for(const auto& agent_id : transitioned) + { + auto itr = m.find(agent_id); + if(itr == m.end() || !itr->second) continue; + + auto queues = hsa_barrier::queue_map_ptr_t{}; + if(auto it = pd_map.find(agent_id); it != pd_map.end()) queues = it->second; + + itr->second->wlock([&](auto& serializer) { + if(enable) + serializer.enable(queues); + else + serializer.disable(queues); + }); } }); }); } +void +QueueController::disable_serialization() +{ + update_serialization({}, false); +} + void QueueController::enable_serialization() { - _queues.rlock([&](const queue_map_t& _queues_v) { - _serialized_enabled.store(true); - auto pd_map = per_dev_map(_queues_v); - _profiler_serializer.wlock([&](auto& m) { - for(auto& [k, v] : m) - { - if(auto it = pd_map.find(k); it != pd_map.end()) - { - v->wlock([&](auto& serializer) { serializer.enable(it->second); }); - } - else - { - v->wlock([&](auto& serializer) { serializer.enable({}); }); - } - } - }); - }); + update_serialization({}, true); +} + +void +QueueController::disable_serialization(const agent_handle_set_t& agents) +{ + update_serialization(agents, false); +} + +void +QueueController::enable_serialization(const agent_handle_set_t& agents) +{ + update_serialization(agents, true); +} + +bool +QueueController::is_serialization_enabled(rocprofiler_agent_id_t agent_id) const +{ + bool enabled = false; + _serialization_refcount.rlock( + [&](const auto& state) { enabled = state.enabled(agent_id); }); + return enabled; } void diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp index 233598863953..433b6fc478fe 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp @@ -29,11 +29,13 @@ #include #include +#include #include #include #include #include +#include #include namespace rocprofiler @@ -89,13 +91,27 @@ class QueueController common::Synchronized& serializer(const Queue*); + // An empty set means every GPU agent. + using agent_handle_set_t = std::unordered_set; + /** - * Disable serialization for QueueController, has no effect if counter collection - * is not in use (which defaults to no serialization mechanism). Should only be used for - * testing. + * Enable/disable serialization for QueueController, has no effect if counter collection + * is not in use (which defaults to no serialization mechanism). + * + * Serialization is reference counted per agent. Counter collection, thread trace and SPM + * each enable it independently, so an agent stays serialized until every subsystem that + * asked for it has released it -- otherwise whichever service stops first would silently + * unserialize the ones still running. The no-argument overloads apply to every GPU agent, + * which is the behavior every caller had before agents could be scoped. */ void enable_serialization(); void disable_serialization(); + void enable_serialization(const agent_handle_set_t& agents); + void disable_serialization(const agent_handle_set_t& agents); + + // Whether the given agent currently has serialization enabled. Exposed for tests and for + // callers that need to reason about scope rather than trigger a transition. + bool is_serialization_enabled(rocprofiler_agent_id_t agent_id) const; // Prints current state of signals for queues, used for debugging. Only prints // serialization related signals if not compiled in debug mode. @@ -121,6 +137,39 @@ class QueueController std::unordered_map>>> _profiler_serializer; + // How many subsystems currently want serialization, per agent. `all` counts the callers + // that asked for every agent; it is kept separate from `per_agent` because an agent's + // serializer is created lazily on first use and an unscoped request has to cover agents + // that do not exist yet. + struct serialization_refcount + { + int64_t all = 0; + std::unordered_map per_agent = {}; + + int64_t count(rocprofiler_agent_id_t agent_id) const + { + auto itr = per_agent.find(agent_id); + return all + ((itr == per_agent.end()) ? 0 : itr->second); + } + + bool enabled(rocprofiler_agent_id_t agent_id) const { return count(agent_id) > 0; } + + bool any() const + { + if(all > 0) return true; + for(const auto& [agent_id, count] : per_agent) + { + if(count > 0) return true; + } + return false; + } + }; + + common::Synchronized _serialization_refcount; + + // Applies a +1/-1 to the refcount of each agent in `agents` (empty == all GPU agents) and + // toggles only the serializers whose effective count crossed zero. + void update_serialization(const agent_handle_set_t& agents, bool enable); }; QueueController* diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp index 2c2f581615ec..a2ad5103aca7 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.cpp @@ -22,6 +22,7 @@ #include "lib/rocprofiler-sdk/spm/core.hpp" #include "lib/common/utility.hpp" +#include "lib/rocprofiler-sdk/agent.hpp" #include "lib/rocprofiler-sdk/context/context.hpp" #include "lib/rocprofiler-sdk/counters/metrics.hpp" #include "lib/rocprofiler-sdk/hsa/queue_controller.hpp" @@ -32,6 +33,7 @@ #include #include #include +#include #include namespace rocprofiler @@ -260,7 +262,9 @@ start_context(const context::context* ctx) auto* controller = hsa::get_queue_controller(); - CHECK_NOTNULL(controller)->enable_serialization(); + // Scope serialization to the agents this context collects on. An empty set still means + // every agent, so an unrestricted context serializes the whole machine as before. + CHECK_NOTNULL(controller)->enable_serialization(ctx->dispatch_spm->agents); ctx->dispatch_spm->enabled.wlock([&](auto& enabled) { if(enabled) return; enabled = true; @@ -312,12 +316,44 @@ stop_context(const context::context* ctx) // whereas the drain bounds when completions arrive at all, which is what the rest of the // teardown depends on. Keeping it. hsa::queue_controller_sync(); - controller->disable_serialization(); + controller->disable_serialization(ctx->dispatch_spm->agents); // No per-queue callback to remove; spm::write_hook no-ops once dispatch_spm is // disabled above. } } +rocprofiler_status_t +set_dispatch_agents(rocprofiler_context_id_t context_id, + const rocprofiler_agent_id_t* agents, + size_t num_agents) +{ + if(num_agents > 0 && agents == nullptr) return ROCPROFILER_STATUS_ERROR_INVALID_ARGUMENT; + + auto* ctx_p = context::get_mutable_registered_context(context_id); + if(!ctx_p) return ROCPROFILER_STATUS_ERROR_CONTEXT_INVALID; + if(!ctx_p->dispatch_spm) return ROCPROFILER_STATUS_ERROR_CONTEXT_NOT_FOUND; + + for(const auto* itr : context::get_active_contexts()) + { + if(itr && itr->context_idx == ctx_p->context_idx) + return ROCPROFILER_STATUS_ERROR_CONFIGURATION_LOCKED; + } + + auto selected = std::unordered_set{}; + selected.reserve(num_agents); + for(size_t i = 0; i < num_agents; ++i) + { + const auto* agent = rocprofiler::agent::get_agent(agents[i]); + if(!agent || agent->type != ROCPROFILER_AGENT_TYPE_GPU) + return ROCPROFILER_STATUS_ERROR_AGENT_NOT_FOUND; + selected.emplace(agents[i]); + } + + ctx_p->dispatch_spm->agents = std::move(selected); + + return ROCPROFILER_STATUS_SUCCESS; +} + } // namespace spm } // namespace rocprofiler diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp index eaedee3f9bc3..0901004e96fc 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/core.hpp @@ -121,5 +121,10 @@ start_context(const context::context*); void stop_context(const context::context*); +rocprofiler_status_t +set_dispatch_agents(rocprofiler_context_id_t context_id, + const rocprofiler_agent_id_t* agents, + size_t num_agents); + } // namespace spm } // namespace rocprofiler diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp index 9c893e7302ac..c44279543b45 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp @@ -21,6 +21,8 @@ // THE SOFTWARE. #include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" +#include "lib/rocprofiler-sdk/hsa/agent_cache.hpp" +#include "lib/rocprofiler-sdk/hsa/queue.hpp" #include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include "lib/rocprofiler-sdk/spm/dispatch_handlers.hpp" @@ -48,9 +50,13 @@ write_hook(const hsa::Queue& queue, hsa::inst_pkt_t& inst_pkt, bool& is_serialized) { + const auto agent_id = CHECK_NOTNULL(queue.get_agent().get_rocp_agent())->id; + auto active = context::get_active_contexts(spm_contexts_filter()); for(const auto* ctx : active) { + if(!ctx->dispatch_spm->collects_on(agent_id)) continue; + for(auto& cb : ctx->dispatch_spm->callbacks) { auto [packet, bSerial] = pre_kernel_call(ctx, @@ -83,9 +89,7 @@ signal_completion_hook(const hsa::Queue& /*queue*/, // // Iterating registered contexts widens the candidate set, but routing stays per-origin because // each context's post_kernel_call only claims packets present in its own packet_return_map. - // That matters because there is no conflict guard rejecting a second concurrent dispatch_spm - // context, unlike dispatch_counter_collection, so more than one SPM context can be registered - // at once. + // Disjoint agent sets allow multiple concurrent SPM contexts when scoped via set_agents(). auto contexts = context::get_registered_contexts(spm_contexts_filter()); for(const auto* ctx : contexts) { diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/service.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/service.cpp index 1ba8fc7c11cd..676734e3a187 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/service.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/service.cpp @@ -148,6 +148,17 @@ rocprofiler_spm_configure_callback_dispatch_service( record_callback_args); } +rocprofiler_status_t +rocprofiler_spm_dispatch_counting_service_set_agents(rocprofiler_context_id_t context_id, + const rocprofiler_agent_id_t* agents, + size_t num_agents) +{ + if(!rocprofiler::spm::is_spm_explicitly_enabled()) + return ROCPROFILER_STATUS_ERROR_NOT_IMPLEMENTED; + + return rocprofiler::spm::set_dispatch_agents(context_id, agents, num_agents); +} + /** * @brief Query Agent Counters Availability. * From 996e64053b4b31b00daafb71acdb388b64f74764 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 10 Aug 2026 23:06:34 +0000 Subject: [PATCH 09/21] fix(rocprofiler-sdk): repair SPM per-agent scoping CI failures Apply clang-format 11 to queue_controller and construct correlation_id via its constructor in the SPM stop_context completion test. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/hsa/queue_controller.cpp | 6 ++---- .../source/lib/rocprofiler-sdk/hsa/queue_controller.hpp | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp index 9444f1c797cc..9d74389a9a8c 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp @@ -483,8 +483,7 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena { auto itr = was_enabled.find(agent_id); if(itr == was_enabled.end()) continue; - if(itr->second != state.enabled(agent_id)) - transitioned.emplace_back(agent_id); + if(itr->second != state.enabled(agent_id)) transitioned.emplace_back(agent_id); } }); @@ -543,8 +542,7 @@ bool QueueController::is_serialization_enabled(rocprofiler_agent_id_t agent_id) const { bool enabled = false; - _serialization_refcount.rlock( - [&](const auto& state) { enabled = state.enabled(agent_id); }); + _serialization_refcount.rlock([&](const auto& state) { enabled = state.enabled(agent_id); }); return enabled; } diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp index 433b6fc478fe..99e47cc08411 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.hpp @@ -143,7 +143,7 @@ class QueueController // that do not exist yet. struct serialization_refcount { - int64_t all = 0; + int64_t all = 0; std::unordered_map per_agent = {}; int64_t count(rocprofiler_agent_id_t agent_id) const From a8615cf019d9613d56b11cdfc65ee9fd61853f07 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 10 Aug 2026 23:23:18 +0000 Subject: [PATCH 10/21] style(rocprofiler-sdk): clang-format context.cpp stop_context The dispatch_thread_trace stop call fits on one line under the project clang-format profile. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/context/context.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp index 3b3f4be5d4d3..e8a312e2de45 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/context/context.cpp @@ -403,8 +403,7 @@ stop_context(rocprofiler_context_id_t idx) rocprofiler::spm::stop_context(const_cast(_expected)); if(_expected->device_thread_trace) _expected->device_thread_trace->stop_context(); - if(_expected->dispatch_thread_trace) - _expected->dispatch_thread_trace->stop_context(); + if(_expected->dispatch_thread_trace) _expected->dispatch_thread_trace->stop_context(); bool success = itr.compare_exchange_strong(_expected, nullptr); From 758072bbdca76c15695c4156a12c52303568fd33 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 12 Aug 2026 18:02:47 +0000 Subject: [PATCH 11/21] fix(rocprofiler-sdk): store effective state in update_serialization; add set_agents/disjoint-context tests Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com> --- .../rocprofiler-sdk/hsa/queue_controller.cpp | 18 +- .../lib/rocprofiler-sdk/spm/tests/core.cpp | 179 ++++++++++++++++++ 2 files changed, 190 insertions(+), 7 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp index 9d74389a9a8c..0af460df7f52 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp @@ -443,10 +443,12 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena _queues.rlock([&](const queue_map_t& _queues_v) { auto pd_map = per_dev_map(_queues_v); - // Agents whose effective refcount crossed zero in this call; only those get their - // serializer toggled. Collected under the refcount lock, applied under the serializer - // lock. - auto transitioned = std::vector{}; + // Agents whose effective state changed in this call; only those get their serializer + // toggled. Each entry stores the agent id together with its new effective state so + // that the serializer action is derived from the post-update refcount rather than + // from the caller's `enable` flag (which could be stale if another thread races + // between the refcount unlock and the serializer lock). + auto transitioned = std::vector>{}; bool any_enabled = false; _serialization_refcount.wlock([&](auto& state) { @@ -483,7 +485,9 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena { auto itr = was_enabled.find(agent_id); if(itr == was_enabled.end()) continue; - if(itr->second != state.enabled(agent_id)) transitioned.emplace_back(agent_id); + bool now_enabled = state.enabled(agent_id); + if(itr->second != now_enabled) + transitioned.emplace_back(agent_id, now_enabled); } }); @@ -495,7 +499,7 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena if(transitioned.empty()) return; _profiler_serializer.wlock([&](auto& m) { - for(const auto& agent_id : transitioned) + for(const auto& [agent_id, now_enabled] : transitioned) { auto itr = m.find(agent_id); if(itr == m.end() || !itr->second) continue; @@ -504,7 +508,7 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena if(auto it = pd_map.find(agent_id); it != pd_map.end()) queues = it->second; itr->second->wlock([&](auto& serializer) { - if(enable) + if(now_enabled) serializer.enable(queues); else serializer.disable(queues); diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index 79a73228506f..0b30afe7b2ac 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -1124,3 +1124,182 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) if(!found_spm_agent) GTEST_SKIP() << "SPM unavailable"; FAIL() << "Could not exercise SPM in-flight completion hook path"; } + +// Verify that rocprofiler_spm_dispatch_counting_service_set_agents restricts serialization +// and write_hook filtering to the configured agents. +TEST(spm_core, set_agents_restricts_collection) +{ + rocprofiler::common::set_env("ROCPROFILER_SPM_BETA_ENABLED", true); + ASSERT_EQ(hsa_init(), HSA_STATUS_SUCCESS); + test_init(); + + registration::init_logging(); + registration::set_init_status(-1); + context::push_client(1); + registration::set_fini_status(0); + + auto agents = hsa::get_queue_controller()->get_supported_agents(); + ASSERT_GT(agents.size(), 0); + + // Pick the first SPM-capable agent to scope the context to. + const rocprofiler::hsa::AgentCache* target_agent = nullptr; + for(const auto& [_, agent] : agents) + { + auto rocp_agent = agent.get_rocp_agent(); + if(!rocp_agent) continue; + if(!rocp_agent->runtime_visibility.hsa || !rocp_agent->runtime_visibility.hip) continue; + if(!is_spm_supported_arch(agent)) continue; + target_agent = &agent; + break; + } + + if(!target_agent) GTEST_SKIP() << "SPM unavailable"; + + rocprofiler_context_id_t ctx_id{}; + ROCPROFILER_CALL(rocprofiler_create_context(&ctx_id), "context creation failed"); + + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service(ctx_id, + null_dispatch_callback, + nullptr, + null_record_callback, + nullptr), + "Could not setup counting service"); + + // Scope the context to target_agent only. + rocprofiler_agent_id_t target_id = target_agent->get_rocp_agent()->id; + ROCPROFILER_CALL( + rocprofiler_spm_dispatch_counting_service_set_agents(ctx_id, &target_id, 1), + "Could not set agents on SPM context"); + + // Verify that the agent set was recorded correctly before starting the context. + auto* ctx_p = context::get_mutable_registered_context(ctx_id); + ASSERT_TRUE(ctx_p && ctx_p->dispatch_spm); + + EXPECT_TRUE(ctx_p->dispatch_spm->collects_on(target_id)) + << "Context should collect on the configured agent"; + + // For every other GPU agent the context must NOT collect. + for(const auto& [_, agent] : agents) + { + auto rocp_agent = agent.get_rocp_agent(); + if(!rocp_agent) continue; + if(rocp_agent->id.handle == target_id.handle) continue; + if(rocp_agent->type != ROCPROFILER_AGENT_TYPE_GPU) continue; + EXPECT_FALSE(ctx_p->dispatch_spm->collects_on(rocp_agent->id)) + << "Context must not collect on an agent that was not configured"; + } + + ROCPROFILER_CALL(rocprofiler_start_context(ctx_id), "start context"); + + bool enabled = false; + ctx_p->dispatch_spm->enabled.rlock([&](const auto& data) { enabled = data; }); + EXPECT_TRUE(enabled); + + ROCPROFILER_CALL(rocprofiler_stop_context(ctx_id), "stop context"); + + registration::set_init_status(1); + registration::finalize(); + context::pop_client(1); + set_client_ctx(get_client_ctx()); +} + +// Verify that two SPM dispatch contexts with disjoint agent sets can start concurrently, +// while two contexts that share an agent cannot. +TEST(spm_core, disjoint_contexts_no_conflict) +{ + rocprofiler::common::set_env("ROCPROFILER_SPM_BETA_ENABLED", true); + ASSERT_EQ(hsa_init(), HSA_STATUS_SUCCESS); + test_init(); + + registration::init_logging(); + registration::set_init_status(-1); + context::push_client(1); + registration::set_fini_status(0); + + auto agents = hsa::get_queue_controller()->get_supported_agents(); + ASSERT_GT(agents.size(), 0); + + // Collect SPM-capable agents. + std::vector spm_agents; + for(const auto& [_, agent] : agents) + { + auto rocp_agent = agent.get_rocp_agent(); + if(!rocp_agent) continue; + if(!rocp_agent->runtime_visibility.hsa || !rocp_agent->runtime_visibility.hip) continue; + if(!is_spm_supported_arch(agent)) continue; + spm_agents.push_back(rocp_agent->id); + } + + if(spm_agents.empty()) GTEST_SKIP() << "SPM unavailable"; + + // ---- Single-agent case: two contexts claiming the same agent must conflict ---- + { + rocprofiler_context_id_t ctx_a{}; + rocprofiler_context_id_t ctx_b{}; + + ROCPROFILER_CALL(rocprofiler_create_context(&ctx_a), "create ctx_a"); + ROCPROFILER_CALL(rocprofiler_create_context(&ctx_b), "create ctx_b"); + + ROCPROFILER_CALL( + rocprofiler_spm_configure_callback_dispatch_service( + ctx_a, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_a"); + ROCPROFILER_CALL( + rocprofiler_spm_configure_callback_dispatch_service( + ctx_b, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_b"); + + // Point both contexts at the same (first) agent. + ROCPROFILER_CALL( + rocprofiler_spm_dispatch_counting_service_set_agents(ctx_a, &spm_agents[0], 1), + "set agents ctx_a"); + ROCPROFILER_CALL( + rocprofiler_spm_dispatch_counting_service_set_agents(ctx_b, &spm_agents[0], 1), + "set agents ctx_b"); + + ROCPROFILER_CALL(rocprofiler_start_context(ctx_a), "start ctx_a"); + auto status_b = rocprofiler_start_context(ctx_b); + EXPECT_EQ(status_b, ROCPROFILER_STATUS_ERROR_CONTEXT_CONFLICT) + << "Two SPM contexts covering the same agent must conflict"; + + ROCPROFILER_CALL(rocprofiler_stop_context(ctx_a), "stop ctx_a"); + } + + // ---- Two-agent case: contexts covering disjoint agents must coexist ---- + if(spm_agents.size() >= 2) + { + rocprofiler_context_id_t ctx_a{}; + rocprofiler_context_id_t ctx_b{}; + + ROCPROFILER_CALL(rocprofiler_create_context(&ctx_a), "create ctx_a (disjoint)"); + ROCPROFILER_CALL(rocprofiler_create_context(&ctx_b), "create ctx_b (disjoint)"); + + ROCPROFILER_CALL( + rocprofiler_spm_configure_callback_dispatch_service( + ctx_a, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_a (disjoint)"); + ROCPROFILER_CALL( + rocprofiler_spm_configure_callback_dispatch_service( + ctx_b, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_b (disjoint)"); + + ROCPROFILER_CALL( + rocprofiler_spm_dispatch_counting_service_set_agents(ctx_a, &spm_agents[0], 1), + "set agents ctx_a (disjoint)"); + ROCPROFILER_CALL( + rocprofiler_spm_dispatch_counting_service_set_agents(ctx_b, &spm_agents[1], 1), + "set agents ctx_b (disjoint)"); + + ROCPROFILER_CALL(rocprofiler_start_context(ctx_a), "start ctx_a (disjoint)"); + ROCPROFILER_CALL(rocprofiler_start_context(ctx_b), + "start ctx_b (disjoint) — must not conflict with ctx_a"); + + ROCPROFILER_CALL(rocprofiler_stop_context(ctx_a), "stop ctx_a (disjoint)"); + ROCPROFILER_CALL(rocprofiler_stop_context(ctx_b), "stop ctx_b (disjoint)"); + } + + registration::set_init_status(1); + registration::finalize(); + context::pop_client(1); + set_client_ctx(get_client_ctx()); +} From ee045e38074c5d2b82c1669905a069288b8a2f7d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 19 Aug 2026 20:27:46 +0000 Subject: [PATCH 12/21] perf(rocprofiler-sdk): SPM exit-hook fast path and gate caching - Skip SPM completion hook when inst_pkt has no SPM_CLIENT_ID tag - Cache is_any_active() once per WriteInterceptor invocation - Add unit test for exit-hook skip path Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/hsa/queue.cpp | 5 +++-- .../lib/rocprofiler-sdk/spm/queue_hooks.cpp | 11 ++++++++++ .../spm/tests/queue_hooks_test.cpp | 22 +++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp index 6c3e024c36f7..537dc5b67233 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp @@ -315,10 +315,11 @@ WriteInterceptor(const void* packets, auto* gls = ::rocprofiler::hip::graph::current_launch_state(); const bool graph_launch_active = (gls != nullptr); + const bool spm_active = spm::is_any_active(); // SPM no longer registers a queue-controller callback, so it does not count toward // get_notifiers(); detect it explicitly so an SPM-only run still enters the interceptor. const bool no_real_consumers = - (queue.get_notifiers() == 0 && !spm::is_any_active() && + (queue.get_notifiers() == 0 && !spm_active && context::get_active_contexts(full_packet_instrumentation_context_filter).empty()); if(pkt_count == 0 || (no_real_consumers && !graph_launch_active)) @@ -776,7 +777,7 @@ WriteInterceptor(const void* packets, }); // SPM requires per-packet mode; it no longer participates in the registry above. - if(spm::is_any_active()) should_batch_packets = false; + if(spm_active) should_batch_packets = false; if(should_batch_packets) { diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp index c44279543b45..dab28e8446bc 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp @@ -82,6 +82,17 @@ signal_completion_hook(const hsa::Queue& /*queue*/, hsa::inst_pkt_t& inst_pkt, kernel_dispatch::profiling_time dispatch_time) { + bool has_spm_packets = false; + for(const auto& tagged_pkt : inst_pkt) + { + if(tagged_pkt.second == hsa::queue_hooks::SPM_CLIENT_ID) + { + has_spm_packets = true; + break; + } + } + if(!has_spm_packets) return; + // Route by packet provenance, not current activeness: post_kernel_call self-filters via // packet_return_map, so in-flight dispatches still complete after stop_context removes the // context from the active list. Without this, a dispatch in flight at stop skips the diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp index ce9be8541d35..c936aa73565a 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp @@ -20,6 +20,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. +#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" #include @@ -34,4 +35,25 @@ TEST(spm_queue_hooks, is_any_active_false_when_no_context_active) { EXPECT_FALSE(rocprofiler::spm::is_any_active()); } + +TEST(spm_queue_hooks, exit_hook_skips_when_inst_pkt_has_no_spm_client_id) +{ + rocprofiler::hsa::inst_pkt_t inst_pkt; + inst_pkt.emplace_back( + std::make_pair(std::make_unique(), + rocprofiler::hsa::queue_hooks::COUNTERS_CLIENT_ID)); + + auto sess = std::make_shared(); + auto packet = rocprofiler::hsa::packet_data_t{}; + auto fq_pkt = rocprofiler::hsa::rocprofiler_packet{}; + + rocprofiler::spm::signal_completion_hook( + *reinterpret_cast(nullptr), + fq_pkt, + sess, + packet, + inst_pkt, + rocprofiler::kernel_dispatch::profiling_time{}); + SUCCEED(); +} } // namespace From 8ca18679e6c582b6b15a6e0fb97af6ae104e1de2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 20 Aug 2026 01:38:27 +0000 Subject: [PATCH 13/21] test(rocprofiler-sdk): set expected correlation_id in SPM in-flight hook test stop_context_in_flight_completion_routes_via_hook_path set corr_id.internal to 99 but never expected.correlation_id, so user_dispatch_cb compared against 0. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/spm/tests/core.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index 0b30afe7b2ac..98bd0accb6b8 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -1055,10 +1055,12 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) context::correlation_id corr_id{}; corr_id.internal = 99; - expected.queue_id = qid; - expected.agent_id = agent.get_rocp_agent()->id; - expected.kernel_id = 42; - expected.dispatch_id = 7; + expected.queue_id = qid; + expected.agent_id = agent.get_rocp_agent()->id; + expected.kernel_id = 42; + expected.dispatch_id = 7; + expected.correlation_id = {.internal = corr_id.internal, + .external = context::null_user_data}; auto user_data = rocprofiler_user_data_t{.value = corr_id.internal}; auto ret_pkt = spm::pre_kernel_call(ctx_p, From 836d8bcfb115af91e8b0659bf3496cb571612e69 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 05:48:17 +0000 Subject: [PATCH 14/21] fix(rocprofiler-sdk): move queue hook tags off the callback registry id range The inst_pkt_t tag space is shared between the per-queue callback registry and the queue hooks. QueueController::add_callback allocates strictly positive ids counting up from 1, which is exactly the range the hook tags used, so a packet tagged by a not-yet-migrated service could compare equal to a hook tag. This was inert while the tags were only carried and never inspected, but counters, thread trace and SPM now each select their own packets by comparing against their tag, so the overlap is reachable. Move the tags to a negative range, which the registry cannot produce, and add a static_assert so the invariant survives future additions. Also converts the tags from separate constexpr int64_t values to an unscoped enum with a fixed int64_t underlying type, keeping the implicit conversion to hsa::ClientID so call sites need no casts. (cherry picked from commit 4e1e51ad1688251418372c832378f96cd91c2564) --- .../hsa/queue_hooks/client_ids.hpp | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp index 9e4879b262bc..b7a45802fef9 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp @@ -1,6 +1,6 @@ // MIT License // -// Copyright (c) 2025 Advanced Micro Devices, Inc. All rights reserved. +// Copyright (c) 2026 Advanced Micro Devices, Inc. All rights reserved. // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal @@ -30,14 +30,35 @@ namespace hsa { namespace queue_hooks { -// Tags identifying the producer subsystem of each inst_pkt_t entry. -// Only distinctness matters; values are fixed for test-order stability. -// Services are migrated off the per-queue callback registry one at a time; the -// unused ids are reserved so the tag values stay stable across those PRs. -constexpr int64_t COUNTERS_CLIENT_ID = 1; -constexpr int64_t THREAD_TRACE_CLIENT_ID = 2; -constexpr int64_t PC_SAMPLING_CLIENT_ID = 3; -constexpr int64_t SPM_CLIENT_ID = 4; +// Tags identifying the producer subsystem of each inst_pkt_t entry, stored in the +// inst_pkt_t pair as an hsa::ClientID. +// +// The underlying type is fixed to int64_t to match hsa::ClientID. The enum is +// deliberately unscoped rather than an enum class: unscoped keeps the implicit +// conversion to ClientID, so the emplace sites need no cast, while still grouping +// the values under one type. +// +// The values are negative because they share the inst_pkt_t tag space with the +// per-queue callback registry, whose QueueController::add_callback hands out +// strictly positive ids counting up from 1. Migrated services (counters, thread +// trace, SPM) select their own packets by comparing against these tags, so a tag +// drawn from the registry's range could match a packet the service does not own. +// Services that have not migrated yet still receive registry ids, so the two +// ranges must stay disjoint until the registry is gone. Any tag added here must +// likewise be negative; -1 is left unused because QueueController::add_callback +// uses it as a local "unassigned" initializer. +enum client_id : int64_t +{ + COUNTERS_CLIENT_ID = -1001, + THREAD_TRACE_CLIENT_ID = -1002, + PC_SAMPLING_CLIENT_ID = -1003, + SPM_CLIENT_ID = -1004, +}; + +static_assert(COUNTERS_CLIENT_ID < 0 && THREAD_TRACE_CLIENT_ID < 0 && PC_SAMPLING_CLIENT_ID < 0 && + SPM_CLIENT_ID < 0, + "queue hook tags must stay disjoint from the positive ids handed out by " + "QueueController::add_callback"); } // namespace queue_hooks } // namespace hsa } // namespace rocprofiler From 095cbd8b0286956d056d0a4cc41e622de3cf62a7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 18 Sep 2026 06:03:47 +0000 Subject: [PATCH 15/21] fix(rocprofiler-sdk): close serializer creation and transition-ordering races Two races in QueueController, both reported in review and both still open. serializer() read the per-agent refcount, released it, and only then looked up or inserted the serializer entry. A concurrent update_serialization() in that gap finds no entry to transition, so the entry is created from a stale observation and keeps the wrong state indefinitely: created from a stale `true` it stays serialized at refcount zero, created from a stale `false` the first dispatch runs unserialized. Hold the refcount read lock across the lookup and insertion so the observation and the insertion are atomic with respect to updates. update_serialization() computed which agents transitioned under the refcount write lock but applied the serializer enable/disable after releasing it. Two callers could therefore apply their transitions in the opposite order from the refcount mutations that produced them, leaving the serializer enabled at refcount zero. Apply the transitions inside the same refcount write lock, and derive the action from the post-update refcount rather than the caller's `enable` flag. Lock order is refcount -> serializer map -> per-agent serializer in both paths, so they cannot deadlock against each other; neither enable() nor disable() calls back into serializer() or the refcount. This is the protocol already used on the counters branch; both branches now carry identical serializer() and update_serialization() bodies. (cherry picked from commit 6b5b59eb4d83bfc91e613aaca4fc6da99c4f0c67) --- .../rocprofiler-sdk/hsa/queue_controller.cpp | 104 ++++++++---------- 1 file changed, 48 insertions(+), 56 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp index 0af460df7f52..d5f96366c6cf 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue_controller.cpp @@ -393,33 +393,34 @@ QueueController::serializer(const Queue* queue) CHECK(queue); const auto agent_id = queue->get_agent().get_rocp_agent()->id; - // Read the refcount before taking the serializer lock: update_serialization() acquires - // them in that order, and reversing it here would deadlock. A concurrent enable/disable - // for this agent can therefore race with the creation below, exactly as the previous - // _serialized_enabled load could, and is resolved the same way -- the next transition - // through update_serialization() reaches the entry once it is in the map. - const bool should_serialize = is_serialization_enabled(agent_id); - common::Synchronized* ret = nullptr; - _profiler_serializer.ulock( - [&](const auto& m) { - if(auto ptr = m.find(agent_id); ptr != m.end()) - { - ret = ptr->second.get(); + // Hold the refcount read lock across the lookup/insertion so that a concurrent + // update_serialization() cannot run between reading the refcount and inserting the + // entry: if it did, it would find no serializer to transition and the new entry would + // be created in the wrong state. update_serialization() holds the refcount write lock + // then the serializer write lock, so taking them in the same order here is deadlock-free. + _serialization_refcount.rlock([&](const auto& state) { + const bool should_serialize = state.enabled(agent_id); + _profiler_serializer.ulock( + [&](const auto& m) { + if(auto ptr = m.find(agent_id); ptr != m.end()) + { + ret = ptr->second.get(); + return true; + } + return false; + }, + [&](auto& m) { + ret = m.emplace(agent_id, + std::make_shared>()) + .first->second.get(); + if(should_serialize) + { + ret->wlock([&](auto& serializer) { serializer.enable({}); }); + } return true; - } - return false; - }, - [&](auto& m) { - ret = m.emplace(agent_id, - std::make_shared>()) - .first->second.get(); - if(should_serialize) - { - ret->wlock([&](auto& serializer) { serializer.enable({}); }); - } - return true; - }); + }); + }); return *ret; } @@ -443,13 +444,10 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena _queues.rlock([&](const queue_map_t& _queues_v) { auto pd_map = per_dev_map(_queues_v); - // Agents whose effective state changed in this call; only those get their serializer - // toggled. Each entry stores the agent id together with its new effective state so - // that the serializer action is derived from the post-update refcount rather than - // from the caller's `enable` flag (which could be stale if another thread races - // between the refcount unlock and the serializer lock). - auto transitioned = std::vector>{}; - bool any_enabled = false; + // Agents whose effective refcount crossed zero in this call; only those get their + // serializer toggled. Both refcount mutation and serializer transitions happen + // together inside the refcount lock to prevent concurrent calls from interleaving. + bool any_enabled = false; _serialization_refcount.wlock([&](auto& state) { // An agent can be covered by `all` and by an explicit entry at the same time, so @@ -480,14 +478,28 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena } } - _profiler_serializer.rlock([&](const auto& serializers) { + _profiler_serializer.wlock([&](auto& serializers) { for(const auto& [agent_id, _] : serializers) { auto itr = was_enabled.find(agent_id); if(itr == was_enabled.end()) continue; - bool now_enabled = state.enabled(agent_id); - if(itr->second != now_enabled) - transitioned.emplace_back(agent_id, now_enabled); + const bool now_enabled = state.enabled(agent_id); + if(itr->second == now_enabled) continue; + + auto queues = hsa_barrier::queue_map_ptr_t{}; + if(auto it = pd_map.find(agent_id); it != pd_map.end()) queues = it->second; + + auto ser_itr = serializers.find(agent_id); + if(ser_itr == serializers.end() || !ser_itr->second) continue; + // Derived from the post-update refcount rather than the caller's `enable` + // flag, so the applied state cannot disagree with the count that produced + // the transition. + ser_itr->second->wlock([&](auto& serializer) { + if(now_enabled) + serializer.enable(queues); + else + serializer.disable(queues); + }); } }); @@ -495,26 +507,6 @@ QueueController::update_serialization(const agent_handle_set_t& agents, bool ena }); _serialized_enabled.store(any_enabled); - - if(transitioned.empty()) return; - - _profiler_serializer.wlock([&](auto& m) { - for(const auto& [agent_id, now_enabled] : transitioned) - { - auto itr = m.find(agent_id); - if(itr == m.end() || !itr->second) continue; - - auto queues = hsa_barrier::queue_map_ptr_t{}; - if(auto it = pd_map.find(agent_id); it != pd_map.end()) queues = it->second; - - itr->second->wlock([&](auto& serializer) { - if(now_enabled) - serializer.enable(queues); - else - serializer.disable(queues); - }); - } - }); }); } From c9dc1a734f2b3937928b33cbb3badc37b0852bed Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 19 Sep 2026 21:28:25 +0000 Subject: [PATCH 16/21] style(rocprofiler-sdk): apply clang-format to the SPM tests Already unformatted on the base branch, which fails the Formatting / source check. No functional change. Co-authored-by: Vivek Kale --- .../lib/rocprofiler-sdk/spm/tests/core.cpp | 40 ++++++++----------- .../spm/tests/queue_hooks_test.cpp | 26 ++++++------ 2 files changed, 28 insertions(+), 38 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index 98bd0accb6b8..caa5066da7c9 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -1160,18 +1160,14 @@ TEST(spm_core, set_agents_restricts_collection) rocprofiler_context_id_t ctx_id{}; ROCPROFILER_CALL(rocprofiler_create_context(&ctx_id), "context creation failed"); - ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service(ctx_id, - null_dispatch_callback, - nullptr, - null_record_callback, - nullptr), + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service( + ctx_id, null_dispatch_callback, nullptr, null_record_callback, nullptr), "Could not setup counting service"); // Scope the context to target_agent only. rocprofiler_agent_id_t target_id = target_agent->get_rocp_agent()->id; - ROCPROFILER_CALL( - rocprofiler_spm_dispatch_counting_service_set_agents(ctx_id, &target_id, 1), - "Could not set agents on SPM context"); + ROCPROFILER_CALL(rocprofiler_spm_dispatch_counting_service_set_agents(ctx_id, &target_id, 1), + "Could not set agents on SPM context"); // Verify that the agent set was recorded correctly before starting the context. auto* ctx_p = context::get_mutable_registered_context(ctx_id); @@ -1242,14 +1238,12 @@ TEST(spm_core, disjoint_contexts_no_conflict) ROCPROFILER_CALL(rocprofiler_create_context(&ctx_a), "create ctx_a"); ROCPROFILER_CALL(rocprofiler_create_context(&ctx_b), "create ctx_b"); - ROCPROFILER_CALL( - rocprofiler_spm_configure_callback_dispatch_service( - ctx_a, null_dispatch_callback, nullptr, null_record_callback, nullptr), - "configure ctx_a"); - ROCPROFILER_CALL( - rocprofiler_spm_configure_callback_dispatch_service( - ctx_b, null_dispatch_callback, nullptr, null_record_callback, nullptr), - "configure ctx_b"); + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service( + ctx_a, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_a"); + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service( + ctx_b, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_b"); // Point both contexts at the same (first) agent. ROCPROFILER_CALL( @@ -1276,14 +1270,12 @@ TEST(spm_core, disjoint_contexts_no_conflict) ROCPROFILER_CALL(rocprofiler_create_context(&ctx_a), "create ctx_a (disjoint)"); ROCPROFILER_CALL(rocprofiler_create_context(&ctx_b), "create ctx_b (disjoint)"); - ROCPROFILER_CALL( - rocprofiler_spm_configure_callback_dispatch_service( - ctx_a, null_dispatch_callback, nullptr, null_record_callback, nullptr), - "configure ctx_a (disjoint)"); - ROCPROFILER_CALL( - rocprofiler_spm_configure_callback_dispatch_service( - ctx_b, null_dispatch_callback, nullptr, null_record_callback, nullptr), - "configure ctx_b (disjoint)"); + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service( + ctx_a, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_a (disjoint)"); + ROCPROFILER_CALL(rocprofiler_spm_configure_callback_dispatch_service( + ctx_b, null_dispatch_callback, nullptr, null_record_callback, nullptr), + "configure ctx_b (disjoint)"); ROCPROFILER_CALL( rocprofiler_spm_dispatch_counting_service_set_agents(ctx_a, &spm_agents[0], 1), diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp index c936aa73565a..21d40a6e95ac 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp @@ -20,8 +20,8 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. -#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" +#include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include @@ -39,21 +39,19 @@ TEST(spm_queue_hooks, is_any_active_false_when_no_context_active) TEST(spm_queue_hooks, exit_hook_skips_when_inst_pkt_has_no_spm_client_id) { rocprofiler::hsa::inst_pkt_t inst_pkt; - inst_pkt.emplace_back( - std::make_pair(std::make_unique(), - rocprofiler::hsa::queue_hooks::COUNTERS_CLIENT_ID)); + inst_pkt.emplace_back(std::make_pair(std::make_unique(), + rocprofiler::hsa::queue_hooks::COUNTERS_CLIENT_ID)); - auto sess = std::make_shared(); - auto packet = rocprofiler::hsa::packet_data_t{}; - auto fq_pkt = rocprofiler::hsa::rocprofiler_packet{}; + auto sess = std::make_shared(); + auto packet = rocprofiler::hsa::packet_data_t{}; + auto fq_pkt = rocprofiler::hsa::rocprofiler_packet{}; - rocprofiler::spm::signal_completion_hook( - *reinterpret_cast(nullptr), - fq_pkt, - sess, - packet, - inst_pkt, - rocprofiler::kernel_dispatch::profiling_time{}); + rocprofiler::spm::signal_completion_hook(*reinterpret_cast(nullptr), + fq_pkt, + sess, + packet, + inst_pkt, + rocprofiler::kernel_dispatch::profiling_time{}); SUCCEED(); } } // namespace From 03c677696fef5b26ef9df83fdb4cb940ba49357b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 19 Sep 2026 22:43:19 +0000 Subject: [PATCH 17/21] fix(rocprofiler-sdk): take a nullable queue in the SPM completion hook spm/tests/queue_hooks_test.cpp built a Queue& by dereferencing a null pointer to reach the hook's early-return path, which clang rejects outright: error: reinterpret_cast from 'std::nullptr_t' to 'rocprofiler::hsa::Queue *' is not allowed The hook never dereferences the queue, so take it as a pointer and let the test pass nullptr. This matches the counters and PC sampling branches. Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/hsa/queue.cpp | 2 +- .../source/lib/rocprofiler-sdk/spm/queue_hooks.cpp | 2 +- .../source/lib/rocprofiler-sdk/spm/queue_hooks.hpp | 4 +++- .../lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp | 8 ++------ 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp index 537dc5b67233..f4447b5f4793 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp @@ -171,7 +171,7 @@ AsyncSignalHandler(hsa_signal_value_t /*signal_v*/, void* data) // SPM completion is migrated off the callback registry (see WriteInterceptor); invoke // it explicitly here. - spm::signal_completion_hook(queue_info_session.queue, + spm::signal_completion_hook(&queue_info_session.queue, packet.kernel_packet, _session, packet, diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp index dab28e8446bc..3b2ea3bea48a 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.cpp @@ -75,7 +75,7 @@ write_hook(const hsa::Queue& queue, } void -signal_completion_hook(const hsa::Queue& /*queue*/, +signal_completion_hook(const hsa::Queue* /*queue*/, const hsa::rocprofiler_packet& /*kernel_packet*/, std::shared_ptr& session, hsa::packet_data_t& /*packet*/, diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp index ef89e5e9c10a..06ea8037259d 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/queue_hooks.hpp @@ -53,8 +53,10 @@ write_hook(const hsa::Queue& queue, // dispatch_spm contexts (not only active ones) and calls each callback's // post_kernel_call; post_kernel_call self-filters via packet_return_map so // in-flight dispatches still complete after stop_context. +// queue is nullable: the hook never dereferences it, and tests exercise the +// early-return path without an HSA runtime to build a queue from. void -signal_completion_hook(const hsa::Queue& queue, +signal_completion_hook(const hsa::Queue* queue, const hsa::rocprofiler_packet& kernel_packet, std::shared_ptr& session, hsa::packet_data_t& packet, diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp index 21d40a6e95ac..6a750efff433 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp @@ -46,12 +46,8 @@ TEST(spm_queue_hooks, exit_hook_skips_when_inst_pkt_has_no_spm_client_id) auto packet = rocprofiler::hsa::packet_data_t{}; auto fq_pkt = rocprofiler::hsa::rocprofiler_packet{}; - rocprofiler::spm::signal_completion_hook(*reinterpret_cast(nullptr), - fq_pkt, - sess, - packet, - inst_pkt, - rocprofiler::kernel_dispatch::profiling_time{}); + rocprofiler::spm::signal_completion_hook( + nullptr, fq_pkt, sess, packet, inst_pkt, rocprofiler::kernel_dispatch::profiling_time{}); SUCCEED(); } } // namespace From f5d4844079372bbc654a3a62dcb4f1dd642b5158 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 19 Sep 2026 23:18:15 +0000 Subject: [PATCH 18/21] fix(rocprofiler-sdk): update the remaining SPM hook call site to pass a pointer spm/tests/core.cpp still passed a FakeQueue by reference after the hook was changed to take const hsa::Queue*: error: cannot convert 'rocprofiler::hsa::FakeQueue' to 'const rocprofiler::hsa::Queue*' Co-authored-by: Vivek Kale --- .../source/lib/rocprofiler-sdk/spm/tests/core.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp index caa5066da7c9..71ae70a57332 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/core.cpp @@ -1100,7 +1100,7 @@ TEST(spm_queue_hooks, stop_context_in_flight_completion_routes_via_hook_path) inst_pkt.emplace_back( std::make_pair(std::move(ret_pkt.packet), hsa::queue_hooks::SPM_CLIENT_ID)); - spm::signal_completion_hook(fq, + spm::signal_completion_hook(&fq, pkt, sess, sess->packet_data.emplace_back(), From f4da0042fe1d84085767fa94ce217f429e18acdd Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 19 Sep 2026 23:49:12 +0000 Subject: [PATCH 19/21] fix(rocprofiler-sdk): construct valid test fixtures in the SPM exit hook test Two errors g++ reports against the libstdc++ headers, so they are easy to miss when scanning diagnostics by source path: error: invalid new-expression of abstract class type 'rocprofiler::hsa::AQLPacket' error: use of deleted function 'rocprofiler::hsa::queue_info_session_t::queue_info_session_t()' AQLPacket is abstract; EmptyAQLPacket is the concrete no-op subclass intended for this. queue_info_session_t holds a Queue& so it has no default constructor, and the hook returns before dereferencing the session, so a null shared_ptr keeps the test free of any HSA runtime. Co-authored-by: Vivek Kale --- .../lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp index 6a750efff433..25fe30fed279 100644 --- a/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp +++ b/projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/tests/queue_hooks_test.cpp @@ -21,6 +21,7 @@ // THE SOFTWARE. #include "lib/rocprofiler-sdk/spm/queue_hooks.hpp" +#include "lib/rocprofiler-sdk/hsa/aql_packet.hpp" #include "lib/rocprofiler-sdk/hsa/queue_hooks/client_ids.hpp" #include @@ -39,10 +40,12 @@ TEST(spm_queue_hooks, is_any_active_false_when_no_context_active) TEST(spm_queue_hooks, exit_hook_skips_when_inst_pkt_has_no_spm_client_id) { rocprofiler::hsa::inst_pkt_t inst_pkt; - inst_pkt.emplace_back(std::make_pair(std::make_unique(), + inst_pkt.emplace_back(std::make_pair(std::make_unique(), rocprofiler::hsa::queue_hooks::COUNTERS_CLIENT_ID)); - auto sess = std::make_shared(); + // The hook returns before it dereferences the session, so a null one keeps this test free of + // any HSA runtime: queue_info_session_t holds a Queue& and cannot be default constructed. + auto sess = std::shared_ptr{}; auto packet = rocprofiler::hsa::packet_data_t{}; auto fq_pkt = rocprofiler::hsa::rocprofiler_packet{}; From 53c015800dbbdec5e7a979bdaed54ce5c94162de Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 21 Sep 2026 05:54:33 +0000 Subject: [PATCH 20/21] ci(rocprofiler-sdk): repair code coverage and ROCm release compatibility jobs Both workflows failed before compiling any rocprofiler-sdk source. Code coverage died installing ROCR-Runtime with 'CMake 3.25 or higher is required. You are running version 3.22.1', and release compatibility died resolving find_dependency(NUMA) from hsakmt-config.cmake on the 'latest' ROCm container. This branch predates the fixes for both on develop, so take develop's versions of the two workflows and of the rocprofiler-sdk-util action they depend on. The action change is additive -- python-venv-path defaults to empty and is guarded -- so the continuous integration workflow, which does not pass it, is unaffected. The pinned CMake is held at 3.25 rather than develop's 4.x. The external submodules on this branch (json, pybind11, sqlite, yaml-cpp) still declare cmake_minimum_required below 3.5, which CMake 4 rejects outright, so 4.x would trade one configure failure for another. Co-authored-by: Vivek Kale --- .../actions/rocprofiler-sdk-util/action.yml | 61 +++- .../resolve-ci-config/action.yml | 58 +++- .../resolve-ci-config/resolve_ci_config.py | 82 +++-- .../test_resolve_ci_config.py | 326 ++++++++++++++++++ .../rocprofiler-sdk-code_coverage.yml | 34 +- ...rofiler-sdk-rocm_release_compatibility.yml | 12 +- 6 files changed, 531 insertions(+), 42 deletions(-) create mode 100644 .github/actions/rocprofiler-sdk-util/resolve-ci-config/test_resolve_ci_config.py diff --git a/.github/actions/rocprofiler-sdk-util/action.yml b/.github/actions/rocprofiler-sdk-util/action.yml index a34f2b51ea44..7dd1e0a8bf64 100644 --- a/.github/actions/rocprofiler-sdk-util/action.yml +++ b/.github/actions/rocprofiler-sdk-util/action.yml @@ -6,6 +6,10 @@ inputs: description: 'Path to ROCm installation' required: false default: '/opt/rocm' + python-venv-path: + description: 'Python virtual environment with the pinned cmake, relative to the workspace' + required: false + default: '' gpu-target: description: 'GPU target (e.g., gfx94X, gfx110X, gfx120X)' required: true @@ -37,6 +41,10 @@ inputs: description: 'Enable ROCJPEG build' required: false default: 'false' + enable-hipfile-build: + description: 'Enable hipFILE build' + required: false + default: 'false' enable-rocprof-trace-decoder-build: description: 'Enable rocprof-trace-decoder build' required: false @@ -70,13 +78,6 @@ inputs: runs: using: 'composite' steps: - - name: Setup environment variables - shell: bash - run: | - echo "PKG_CONFIG_PATH=/opt/amdgpu/lib64/pkgconfig:/opt/amdgpu/lib/pkgconfig:/opt/amdgpu/lib/$(uname -p)-linux-gnu/pkgconfig:${PKG_CONFIG_PATH}" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${{ inputs.rocm-path }}/lib64:${{ inputs.rocm-path }}/lib:${{ inputs.rocm-path }}/llvm/lib:${LD_LIBRARY_PATH}" >> $GITHUB_ENV - echo "PATH=${{ inputs.rocm-path }}/bin:${{ inputs.rocm-path }}/llvm/bin:$HOME/.local/bin:${PATH}" >> $GITHUB_ENV - - name: Setup ccache if: ${{ inputs.enable-ccache == 'true' }} uses: hendrikmuhs/ccache-action@63069e3931dedbf3b63792097479563182fe70d1 # v1.2.18 @@ -98,6 +99,16 @@ runs: echo "Warning: Compiler launcher ${{ inputs.compiler-launcher }} not found or not executable" fi + - name: Setup environment variables + shell: bash + run: | + if [ -n "${{ inputs.python-venv-path }}" ]; then + echo "${GITHUB_WORKSPACE}/${{ inputs.python-venv-path }}/bin" >> $GITHUB_PATH + fi + echo "PKG_CONFIG_PATH=/opt/amdgpu/lib64/pkgconfig:/opt/amdgpu/lib/pkgconfig:/opt/amdgpu/lib/$(uname -p)-linux-gnu/pkgconfig:${PKG_CONFIG_PATH}" >> $GITHUB_ENV + echo "LD_LIBRARY_PATH=${{ inputs.rocm-path }}/lib64:${{ inputs.rocm-path }}/lib:${{ inputs.rocm-path }}/llvm/lib:${LD_LIBRARY_PATH}" >> $GITHUB_ENV + echo "PATH=${{ inputs.rocm-path }}/bin:${{ inputs.rocm-path }}/llvm/bin:$HOME/.local/bin:${PATH}" >> $GITHUB_ENV + - name: Build and Install ROCR-Runtime if: ${{ inputs.enable-rocr-build == 'true' }} shell: bash @@ -149,7 +160,7 @@ runs: shell: bash working-directory: projects/rocprofiler-register run: | - export PATH=${{ inputs.rocm-path }}/bin:${{ inputs.rocm-path }}/llvm/bin:/usr/local/bin:~/.local/bin:$PATH + export PATH=${{ inputs.rocm-path }}/bin:${{ inputs.rocm-path }}/llvm/bin:${PATH}:/usr/local/bin echo "Install ROCProfiler-Register" cmake -B build-rocprofiler-register \ -DCMAKE_BUILD_TYPE=RelWithDebInfo \ @@ -163,6 +174,40 @@ runs: cmake --build build-rocprofiler-register --target install echo "✅ ROCProfiler-Register Installation complete!" + - name: Build and Install hipFILE + if: ${{ inputs.enable-hipfile-build == 'true' }} + shell: bash + working-directory: projects/hipfile + run: | + export ROCM_PATH=${{ inputs.rocm-path }} + export HIP_PLATFORM=amd + export LD_LIBRARY_PATH=${{ inputs.rocm-path }}/lib:${{ inputs.rocm-path }}/lib/rocm_sysdeps/lib:${{ inputs.rocm-path }}/llvm/lib:$LD_LIBRARY_PATH + export PATH=${{ inputs.rocm-path }}/bin:${{ inputs.rocm-path }}/llvm/bin:${PATH}:/usr/local/bin + # Temporary workaround until hipFILE adds the ROCm sysdeps include path for libmount. + if [ -f "${ROCM_PATH}/lib/rocm_sysdeps/include/libmount/libmount.h" ]; then + install -D -m 0644 \ + "${ROCM_PATH}/lib/rocm_sysdeps/include/libmount/libmount.h" \ + "${ROCM_PATH}/include/libmount/libmount.h" + fi + echo "Install hipFILE..." + cmake -B build-hipfile \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo \ + -DCMAKE_CXX_COMPILER=${{ inputs.rocm-path }}/llvm/bin/amdclang++ \ + -DCMAKE_HIP_PLATFORM=amd \ + -DCMAKE_PREFIX_PATH='${{ inputs.rocm-path }};${{ inputs.rocm-path }}/llvm;${{ inputs.rocm-path }}/lib/rocm_sysdeps' \ + -DCMAKE_LIBRARY_PATH=${{ inputs.rocm-path }}/lib/rocm_sysdeps/lib \ + -DCMAKE_INCLUDE_PATH=${{ inputs.rocm-path }}/lib/rocm_sysdeps/include \ + -DCMAKE_INSTALL_PREFIX=${{ inputs.rocm-path }} \ + -DCMAKE_C_COMPILER_LAUNCHER=${{ inputs.compiler-launcher }} \ + -DCMAKE_CXX_COMPILER_LAUNCHER=${{ inputs.compiler-launcher }} \ + -DBUILD_TESTING=OFF \ + -DAIS_INSTALL_EXAMPLES=OFF \ + -DAIS_INSTALL_TOOLS=OFF \ + . + cmake --build build-hipfile --target hipfile --parallel ${{ inputs.build-jobs }} + cmake --build build-hipfile --target install + echo "✅ hipFILE Installation complete!" + - name: Build and Install Aqlprofile if: ${{ inputs.enable-aqlprofile-build == 'true' }} shell: bash diff --git a/.github/actions/rocprofiler-sdk-util/resolve-ci-config/action.yml b/.github/actions/rocprofiler-sdk-util/resolve-ci-config/action.yml index 6060d1560c7b..a5a878ceed99 100644 --- a/.github/actions/rocprofiler-sdk-util/resolve-ci-config/action.yml +++ b/.github/actions/rocprofiler-sdk-util/resolve-ci-config/action.yml @@ -1,5 +1,5 @@ name: Resolve CI config -description: Resolve runner labels from ROCm/therock-ci-config +description: Resolve build and GPU runner labels from ROCm/therock-ci-config inputs: config-path: @@ -14,14 +14,63 @@ inputs: description: Sandbox runner label to use if the shared CI config cannot be loaded required: false default: linux-mi325-gpu-rocm-cpu-sandbox + fallback-gfx950-runner: + description: Runner label for gfx950 if the shared CI config cannot be loaded + required: false + default: linux-gfx950-1gpu-ccs-ossci-rocm + fallback-gfx90a-runner: + description: Runner label for gfx90a if the shared CI config cannot be loaded + required: false + default: linux-gfx90a-gpu-rocm + fallback-gfx103x-runner: + description: Runner label for gfx103x if the shared CI config cannot be loaded + required: false + default: linux-gfx1030-gpu-rocm + fallback-gfx110x-runner: + description: Runner label for gfx110x if the shared CI config cannot be loaded + required: false + default: linux-gfx110X-gpu-rocm + fallback-gfx120x-runner: + description: Runner label for gfx120x if the shared CI config cannot be loaded + required: false + default: linux-gfx120X-gpu-rocm + fallback-gfx1151-runner: + description: Runner label for gfx1151 if the shared CI config cannot be loaded + required: false + default: linux-gfx1151-gpu-rocm + fallback-linux-build-runner: + description: Linux build runner label if the shared CI config cannot be loaded + required: false + default: aws-linux-scale-rocm-prod outputs: + linux_build_runner: + description: Linux CPU build runner label + value: ${{ steps.runner.outputs.linux_build_runner }} + gfx90a_runner: + description: Linux gfx90a test runner label + value: ${{ steps.runner.outputs.gfx90a_runner }} gfx94x_runner: description: Linux gfx94x test runner label value: ${{ steps.runner.outputs.gfx94x_runner }} gfx94x_sandbox_runner: description: Linux gfx94x sandbox test runner label value: ${{ steps.runner.outputs.gfx94x_sandbox_runner }} + gfx950_runner: + description: Linux gfx950 test runner label + value: ${{ steps.runner.outputs.gfx950_runner }} + gfx103x_runner: + description: Linux gfx103x (Navi 2x) test runner label + value: ${{ steps.runner.outputs.gfx103x_runner }} + gfx110x_runner: + description: Linux gfx110x (Navi 3x) test runner label + value: ${{ steps.runner.outputs.gfx110x_runner }} + gfx120x_runner: + description: Linux gfx120x (Navi 4x) test runner label + value: ${{ steps.runner.outputs.gfx120x_runner }} + gfx1151_runner: + description: Linux gfx1151 test runner label + value: ${{ steps.runner.outputs.gfx1151_runner }} runs: using: composite @@ -41,4 +90,11 @@ runs: CI_CONFIG_PATH: ${{ inputs.config-path }} FALLBACK_GFX94X_RUNNER: ${{ inputs.fallback-gfx94x-runner }} FALLBACK_GFX94X_SANDBOX_RUNNER: ${{ inputs.fallback-gfx94x-sandbox-runner }} + FALLBACK_GFX950_RUNNER: ${{ inputs.fallback-gfx950-runner }} + FALLBACK_GFX90A_RUNNER: ${{ inputs.fallback-gfx90a-runner }} + FALLBACK_GFX103X_RUNNER: ${{ inputs.fallback-gfx103x-runner }} + FALLBACK_GFX110X_RUNNER: ${{ inputs.fallback-gfx110x-runner }} + FALLBACK_GFX120X_RUNNER: ${{ inputs.fallback-gfx120x-runner }} + FALLBACK_GFX1151_RUNNER: ${{ inputs.fallback-gfx1151-runner }} + FALLBACK_LINUX_BUILD_RUNNER: ${{ inputs.fallback-linux-build-runner }} run: python3 "${GITHUB_ACTION_PATH}/resolve_ci_config.py" diff --git a/.github/actions/rocprofiler-sdk-util/resolve-ci-config/resolve_ci_config.py b/.github/actions/rocprofiler-sdk-util/resolve-ci-config/resolve_ci_config.py index 2247dd81a99d..4dba6c129615 100644 --- a/.github/actions/rocprofiler-sdk-util/resolve-ci-config/resolve_ci_config.py +++ b/.github/actions/rocprofiler-sdk-util/resolve-ci-config/resolve_ci_config.py @@ -7,29 +7,58 @@ from pathlib import Path from typing import Any +GPU_FAMILIES = [ + "gfx94x", + "gfx950", + "gfx90a", + "gfx103x", + "gfx110x", + "gfx120x", + "gfx1151", +] +TRIGGER_TYPES = ["presubmit", "postsubmit", "nightly"] + def warn(message: str) -> None: print(f"::warning::{message}") -def load_gfx94x_linux_config(config_path: Path) -> dict[str, Any]: +def load_ci_configs( + config_path: Path, +) -> tuple[dict[str, dict[str, Any]], dict[str, Any]]: if not config_path.exists(): warn("CI config checkout missing. Using fallback runner labels.") - return {} + return {}, {} sys.path.insert(0, str(config_path)) try: from ci_config_api import load_config_v1 config = load_config_v1(config_path) - return ( - config.get_gpu_families(["presubmit"]) - .get("gfx94x", {}) - .get("linux", {}) - ) + all_families = config.get_gpu_families(TRIGGER_TYPES) + gpu_configs = { + family: all_families.get(family, {}).get("linux", {}) + for family in GPU_FAMILIES + } + return gpu_configs, config.build_runners except Exception as exc: warn(f"Failed to load CI config: {exc}. Using fallback runner labels.") - return {} + return {}, {} + + +def load_gpu_configs(config_path: Path) -> dict[str, dict[str, Any]]: + return load_ci_configs(config_path)[0] + + +def get_linux_build_runner(build_runners: dict[str, Any], fallback: str) -> str: + return next( + ( + runner["label"] + for runner in build_runners.get("linux", {}).get("default", []) + if runner.get("label") and runner.get("weight", 0) > 0 + ), + fallback, + ) def set_outputs(outputs: dict[str, str]) -> None: @@ -40,23 +69,32 @@ def set_outputs(outputs: dict[str, str]) -> None: def main() -> None: config_path = Path(os.environ["CI_CONFIG_PATH"]) - fallback_runner = os.environ["FALLBACK_GFX94X_RUNNER"] - fallback_sandbox_runner = os.environ["FALLBACK_GFX94X_SANDBOX_RUNNER"] + gpu_configs, build_runners = load_ci_configs(config_path) - gfx94x_linux_config = load_gfx94x_linux_config(config_path) - runner = gfx94x_linux_config.get("test-runs-on") or fallback_runner - sandbox_runner = ( - gfx94x_linux_config.get("test-runs-on-sandbox") or fallback_sandbox_runner + outputs: dict[str, str] = {} + fallback_linux_build_runner = os.environ.get("FALLBACK_LINUX_BUILD_RUNNER", "") + linux_build_runner = get_linux_build_runner( + build_runners, fallback_linux_build_runner ) - - print(f"Using gfx94x runner: {runner}") + outputs["linux_build_runner"] = linux_build_runner + print(f"Using Linux build runner: {linux_build_runner}") + + for family in GPU_FAMILIES: + linux_config = gpu_configs.get(family, {}) + fallback_env = f"FALLBACK_{family.upper()}_RUNNER" + fallback = os.environ.get(fallback_env, "") + + runner = linux_config.get("test-runs-on") or fallback + outputs[f"{family}_runner"] = runner + print(f"Using {family} runner: {runner}") + + fallback_sandbox = os.environ.get("FALLBACK_GFX94X_SANDBOX_RUNNER", "") + gfx94x_config = gpu_configs.get("gfx94x", {}) + sandbox_runner = gfx94x_config.get("test-runs-on-sandbox") or fallback_sandbox + outputs["gfx94x_sandbox_runner"] = sandbox_runner print(f"Using gfx94x sandbox runner: {sandbox_runner}") - set_outputs( - { - "gfx94x_runner": runner, - "gfx94x_sandbox_runner": sandbox_runner, - } - ) + + set_outputs(outputs) if __name__ == "__main__": diff --git a/.github/actions/rocprofiler-sdk-util/resolve-ci-config/test_resolve_ci_config.py b/.github/actions/rocprofiler-sdk-util/resolve-ci-config/test_resolve_ci_config.py new file mode 100644 index 000000000000..ac4fb1db4808 --- /dev/null +++ b/.github/actions/rocprofiler-sdk-util/resolve-ci-config/test_resolve_ci_config.py @@ -0,0 +1,326 @@ +#!/usr/bin/env python3 + +from __future__ import annotations + +import json +import os +import tempfile +import textwrap +import unittest +from pathlib import Path +from unittest.mock import patch + +from resolve_ci_config import GPU_FAMILIES, TRIGGER_TYPES, load_gpu_configs, main + +SAMPLE_RUNNER_CONFIG = { + "version": "1", + "build_runners": { + "linux": { + "default": [ + {"label": "disabled-linux-runner", "weight": 0.0}, + {"label": "configured-linux-runner", "weight": 1.0}, + ] + } + }, + "gpu_families": { + "presubmit": { + "gfx94x": { + "linux": { + "test-runs-on": "linux-gfx942-1gpu-ccs-csp-ossci-rocm", + "test-runs-on-sandbox": "linux-mi325-gpu-rocm-cpu-sandbox", + "family": "gfx94X-dcgpu", + "fetch-gfx-targets": ["gfx942"], + } + }, + "gfx110x": { + "linux": { + "test-runs-on": "linux-gfx110X-gpu-rocm", + "family": "gfx110X-all", + "fetch-gfx-targets": [], + } + }, + "gfx120x": { + "linux": { + "test-runs-on": "linux-gfx120X-gpu-rocm", + "family": "gfx120X-all", + "fetch-gfx-targets": ["gfx1200", "gfx1201"], + } + }, + "gfx1151": { + "linux": { + "test-runs-on": "linux-gfx1151-gpu-rocm", + "family": "gfx1151", + "fetch-gfx-targets": ["gfx1151"], + } + }, + }, + "postsubmit": { + "gfx950": { + "linux": { + "test-runs-on": "linux-gfx950-1gpu-ccs-ossci-rocm", + "family": "gfx950-dcgpu", + "fetch-gfx-targets": ["gfx950"], + } + } + }, + "nightly": { + "gfx90a": { + "linux": { + "test-runs-on": "linux-gfx90a-gpu-rocm", + "family": "gfx90a", + "fetch-gfx-targets": ["gfx90a"], + } + }, + "gfx103x": { + "linux": { + "test-runs-on": "linux-gfx1030-gpu-rocm", + "family": "gfx103X-all", + "fetch-gfx-targets": ["gfx1030"], + } + }, + }, + }, +} + + +def _write_config(tmpdir: Path, config: dict) -> None: + (tmpdir / "runner-config.json").write_text(json.dumps(config)) + ci_api = textwrap.dedent("""\ + import json + from pathlib import Path + from dataclasses import dataclass + from typing import Any + + @dataclass + class ConfigV1: + build_runners: dict + gpu_families: dict + _raw: dict + def get_gpu_families(self, trigger_types): + result = {} + for tt in trigger_types: + if tt in self.gpu_families: + for name, cfg in self.gpu_families[tt].items(): + result[name] = cfg + return result + + def load_config_v1(config_path=None): + if config_path is None: + config_path = Path(__file__).parent + with open(config_path / "runner-config.json") as f: + raw = json.load(f) + return ConfigV1( + build_runners=raw["build_runners"], + gpu_families=raw["gpu_families"], + _raw=raw, + ) + """) + (tmpdir / "ci_config_api.py").write_text(ci_api) + + +class TestLoadGpuConfigs(unittest.TestCase): + def test_returns_empty_when_path_missing(self): + result = load_gpu_configs(Path("/nonexistent/path")) + self.assertEqual(result, {}) + + def test_loads_all_families_from_config(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = Path(tmpdir) + _write_config(tmpdir, SAMPLE_RUNNER_CONFIG) + result = load_gpu_configs(tmpdir) + + self.assertEqual( + result["gfx94x"]["test-runs-on"], + "linux-gfx942-1gpu-ccs-csp-ossci-rocm", + ) + self.assertEqual( + result["gfx950"]["test-runs-on"], + "linux-gfx950-1gpu-ccs-ossci-rocm", + ) + self.assertEqual( + result["gfx90a"]["test-runs-on"], + "linux-gfx90a-gpu-rocm", + ) + self.assertEqual( + result["gfx103x"]["test-runs-on"], + "linux-gfx1030-gpu-rocm", + ) + self.assertEqual( + result["gfx110x"]["test-runs-on"], + "linux-gfx110X-gpu-rocm", + ) + self.assertEqual( + result["gfx120x"]["test-runs-on"], + "linux-gfx120X-gpu-rocm", + ) + self.assertEqual( + result["gfx1151"]["test-runs-on"], + "linux-gfx1151-gpu-rocm", + ) + + def test_missing_family_returns_empty_dict(self): + config = { + "version": "1", + "build_runners": {}, + "gpu_families": { + "presubmit": { + "gfx94x": { + "linux": { + "test-runs-on": "some-runner", + "family": "f", + "fetch-gfx-targets": [], + } + } + } + }, + } + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = Path(tmpdir) + _write_config(tmpdir, config) + result = load_gpu_configs(tmpdir) + + self.assertEqual(result["gfx94x"]["test-runs-on"], "some-runner") + self.assertEqual(result["gfx950"], {}) + self.assertEqual(result["gfx90a"], {}) + self.assertEqual(result["gfx103x"], {}) + self.assertEqual(result["gfx110x"], {}) + self.assertEqual(result["gfx120x"], {}) + self.assertEqual(result["gfx1151"], {}) + + def test_returns_empty_on_import_error(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = Path(tmpdir) + (tmpdir / "ci_config_api.py").write_text("raise ImportError('broken')") + (tmpdir / "runner-config.json").write_text("{}") + result = load_gpu_configs(tmpdir) + self.assertEqual(result, {}) + + def test_sandbox_runner_available(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = Path(tmpdir) + _write_config(tmpdir, SAMPLE_RUNNER_CONFIG) + result = load_gpu_configs(tmpdir) + self.assertEqual( + result["gfx94x"]["test-runs-on-sandbox"], + "linux-mi325-gpu-rocm-cpu-sandbox", + ) + + +class TestMain(unittest.TestCase): + def _run_main(self, config=None, env_overrides=None): + with tempfile.TemporaryDirectory() as tmpdir: + tmpdir = Path(tmpdir) + output_file = tmpdir / "github_output" + output_file.write_text("") + + if config is not None: + config_dir = tmpdir / "ci-config" + config_dir.mkdir() + _write_config(config_dir, config) + else: + config_dir = tmpdir / "no-config" + + env = { + "CI_CONFIG_PATH": str(config_dir), + "GITHUB_OUTPUT": str(output_file), + "FALLBACK_GFX94X_RUNNER": "fallback-gfx94x", + "FALLBACK_GFX94X_SANDBOX_RUNNER": "fallback-gfx94x-sandbox", + "FALLBACK_GFX950_RUNNER": "fallback-gfx950", + "FALLBACK_GFX90A_RUNNER": "fallback-gfx90a", + "FALLBACK_GFX103X_RUNNER": "fallback-gfx103x", + "FALLBACK_GFX110X_RUNNER": "fallback-gfx110x", + "FALLBACK_GFX120X_RUNNER": "fallback-gfx120x", + "FALLBACK_GFX1151_RUNNER": "fallback-gfx1151", + "FALLBACK_LINUX_BUILD_RUNNER": "fallback-linux-runner", + } + if env_overrides: + env.update(env_overrides) + + with patch.dict(os.environ, env, clear=False): + main() + + outputs = {} + for line in output_file.read_text().strip().splitlines(): + key, _, value = line.partition("=") + outputs[key] = value + return outputs + + def test_uses_config_values_when_available(self): + outputs = self._run_main(config=SAMPLE_RUNNER_CONFIG) + + self.assertEqual(outputs["linux_build_runner"], "configured-linux-runner") + self.assertEqual( + outputs["gfx94x_runner"], "linux-gfx942-1gpu-ccs-csp-ossci-rocm" + ) + self.assertEqual(outputs["gfx950_runner"], "linux-gfx950-1gpu-ccs-ossci-rocm") + self.assertEqual(outputs["gfx90a_runner"], "linux-gfx90a-gpu-rocm") + self.assertEqual(outputs["gfx103x_runner"], "linux-gfx1030-gpu-rocm") + self.assertEqual(outputs["gfx110x_runner"], "linux-gfx110X-gpu-rocm") + self.assertEqual(outputs["gfx120x_runner"], "linux-gfx120X-gpu-rocm") + self.assertEqual(outputs["gfx1151_runner"], "linux-gfx1151-gpu-rocm") + self.assertEqual( + outputs["gfx94x_sandbox_runner"], "linux-mi325-gpu-rocm-cpu-sandbox" + ) + + def test_falls_back_to_env_when_config_missing(self): + outputs = self._run_main(config=None) + + self.assertEqual(outputs["linux_build_runner"], "fallback-linux-runner") + self.assertEqual(outputs["gfx94x_runner"], "fallback-gfx94x") + self.assertEqual(outputs["gfx950_runner"], "fallback-gfx950") + self.assertEqual(outputs["gfx90a_runner"], "fallback-gfx90a") + self.assertEqual(outputs["gfx103x_runner"], "fallback-gfx103x") + self.assertEqual(outputs["gfx110x_runner"], "fallback-gfx110x") + self.assertEqual(outputs["gfx120x_runner"], "fallback-gfx120x") + self.assertEqual(outputs["gfx1151_runner"], "fallback-gfx1151") + self.assertEqual(outputs["gfx94x_sandbox_runner"], "fallback-gfx94x-sandbox") + + def test_partial_config_uses_fallback_for_missing_families(self): + partial_config = { + "version": "1", + "build_runners": {}, + "gpu_families": { + "presubmit": { + "gfx94x": { + "linux": { + "test-runs-on": "configured-gfx94x", + "test-runs-on-sandbox": "configured-sandbox", + "family": "f", + "fetch-gfx-targets": [], + } + } + } + }, + } + outputs = self._run_main(config=partial_config) + + self.assertEqual(outputs["linux_build_runner"], "fallback-linux-runner") + self.assertEqual(outputs["gfx94x_runner"], "configured-gfx94x") + self.assertEqual(outputs["gfx94x_sandbox_runner"], "configured-sandbox") + self.assertEqual(outputs["gfx950_runner"], "fallback-gfx950") + self.assertEqual(outputs["gfx90a_runner"], "fallback-gfx90a") + self.assertEqual(outputs["gfx103x_runner"], "fallback-gfx103x") + self.assertEqual(outputs["gfx110x_runner"], "fallback-gfx110x") + self.assertEqual(outputs["gfx120x_runner"], "fallback-gfx120x") + self.assertEqual(outputs["gfx1151_runner"], "fallback-gfx1151") + + def test_all_gpu_families_have_output(self): + outputs = self._run_main(config=SAMPLE_RUNNER_CONFIG) + for family in GPU_FAMILIES: + self.assertIn(f"{family}_runner", outputs, f"Missing output for {family}") + self.assertTrue(outputs[f"{family}_runner"], f"Empty output for {family}") + self.assertIn("gfx94x_sandbox_runner", outputs) + + +class TestConstants(unittest.TestCase): + def test_gpu_families_not_empty(self): + self.assertTrue(len(GPU_FAMILIES) > 0) + + def test_trigger_types_cover_all_scopes(self): + self.assertIn("presubmit", TRIGGER_TYPES) + self.assertIn("postsubmit", TRIGGER_TYPES) + self.assertIn("nightly", TRIGGER_TYPES) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/rocprofiler-sdk-code_coverage.yml b/.github/workflows/rocprofiler-sdk-code_coverage.yml index c7f713dfb38a..6f0b1e5b2451 100644 --- a/.github/workflows/rocprofiler-sdk-code_coverage.yml +++ b/.github/workflows/rocprofiler-sdk-code_coverage.yml @@ -10,6 +10,7 @@ on: - develop paths: - 'projects/rocprofiler-sdk/**' + - '.github/actions/rocprofiler-sdk-util/**' - '.github/workflows/rocprofiler-sdk-code_coverage.yml' - '.github/actions/rocprofiler-sdk-util/action.yml' - '.github/actions/rocprofiler-sdk-util/resolve-ci-config/**' @@ -25,6 +26,7 @@ on: pull_request: paths: - 'projects/rocprofiler-sdk/**' + - '.github/actions/rocprofiler-sdk-util/**' - '.github/workflows/rocprofiler-sdk-code_coverage.yml' - '.github/actions/rocprofiler-sdk-util/action.yml' - '.github/actions/rocprofiler-sdk-util/resolve-ci-config/**' @@ -45,6 +47,11 @@ concurrency: env: # TODO(jrmadsen): replace LD_RUNPATH_FLAG, GPU_TARGETS, etc. with internal handling in cmake ROCM_PATH: "/opt/rocm" + PYTHON_VENV_PATH: ".venv-rocprofiler-sdk" + # Held at 3.25 rather than develop's 4.x: this branch's external submodules + # (json, pybind11, sqlite, yaml-cpp) still declare cmake_minimum_required + # below 3.5, which CMake 4 rejects outright. + CMAKE_VERSION_DEB: "3.25.0" GPU_TARGETS: "gfx906 gfx908 gfx90a gfx942 gfx950 gfx1030 gfx1100 gfx1101 gfx1102 gfx1201" PATH: "/usr/bin:$PATH" SUBMODULE_PATHS: "projects/rocprofiler-sdk projects/aqlprofile projects/rocprofiler-register projects/rocr-runtime projects/clr projects/hip" @@ -74,6 +81,7 @@ env: ENABLE_AQLPROFILE_BUILD: "true" ENABLE_ROCDECODE_BUILD: "true" ENABLE_ROCJPEG_BUILD: "true" + ENABLE_HIPFILE_BUILD: "true" jobs: ci_config: @@ -137,6 +145,8 @@ jobs: projects/rocr-runtime projects/clr projects/hip + projects/hrr + projects/hipfile submodules: false set-safe-directory: true @@ -163,6 +173,8 @@ jobs: submods-scoped-v1-${{ runner.os }}- - name: Init/Update submodules + timeout-minutes: 15 + continue-on-error: true run: git submodule update --init --recursive --jobs 16 -- ${SUBMODULE_PATHS} - name: Clone ROCDecode @@ -195,12 +207,6 @@ jobs: run: | if [ -d .codecov ]; then cp -r .codecov .codecov.ref; fi - - name: Configure Env - shell: bash - run: | - echo "${PATH}:/usr/local/bin:${HOME}/.local/bin" >> $GITHUB_PATH - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/local/lib:${HOME}/.local/lib" >> $GITHUB_ENV - - name: Install requirements timeout-minutes: 10 shell: bash @@ -208,9 +214,12 @@ jobs: run: | git config --global --add safe.directory '*' apt-get update - apt-get install -y build-essential ccache python3-pip gcovr wkhtmltopdf xvfb xfonts-base xfonts-75dpi xfonts-100dpi xfonts-utils xfonts-encodings libfontconfig libdw-dev libsqlite3-dev libdrm-dev file autoconf pkg-config rpm libzstd-dev - python3 -m pip install -U --user cmake==3.25.0 - python3 -m pip install -U --user -r requirements.txt + apt-get install -y build-essential ccache cmake python3-pip python3-venv gcovr wkhtmltopdf xvfb xfonts-base xfonts-75dpi xfonts-100dpi xfonts-utils xfonts-encodings libfontconfig libdw-dev libsqlite3-dev libdrm-dev file autoconf pkg-config rpm libzstd-dev + python3 -m venv --system-site-packages ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }} + source ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }}/bin/activate + python3 -m pip install --upgrade pip + python3 -m pip install --upgrade -r requirements.txt + python3 -m pip install --upgrade cmake==${{ env.CMAKE_VERSION_DEB }} - name: Sync gcov with compilers timeout-minutes: 10 @@ -233,6 +242,7 @@ jobs: uses: ./.github/actions/rocprofiler-sdk-util with: rocm-path: ${{ env.ROCM_PATH }} + python-venv-path: ${{ env.PYTHON_VENV_PATH }} gpu-target: ${{ matrix.system.gpu-target }} compiler-launcher: /usr/bin/ccache enable-rocr-build: ${{ env.ENABLE_ROCR_BUILD }} @@ -241,6 +251,7 @@ jobs: enable-aqlprofile-build: ${{ env.ENABLE_AQLPROFILE_BUILD }} enable-rocdecode-build: ${{ env.ENABLE_ROCDECODE_BUILD }} enable-rocjpeg-build: ${{ env.ENABLE_ROCJPEG_BUILD }} + enable-hipfile-build: ${{ env.ENABLE_HIPFILE_BUILD }} ccache-key: ccache-${{ matrix.system.os }}-${{ needs.ci_config.outputs.gfx94x_runner }}-${{ matrix.system.gpu }} ccache-variant: ccache @@ -249,6 +260,7 @@ jobs: shell: bash working-directory: projects/rocprofiler-sdk run: + source ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }}/bin/activate && python3 ./source/scripts/run-ci.py -B build --name ${{ github.repository }}-${{ github.ref_name }}-${{ matrix.system.os }}-${{ matrix.system.gpu }}-codecov --build-jobs 16 @@ -270,6 +282,7 @@ jobs: shell: bash working-directory: projects/rocprofiler-sdk run: + source ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }}/bin/activate && find build -type f | egrep '\.gcda$' | xargs rm && python3 ./source/scripts/run-ci.py -B build --name ${{ github.repository }}-${{ github.ref_name }}-${{ matrix.system.os }}-${{ matrix.system.gpu }}-codecov-tests @@ -292,6 +305,7 @@ jobs: working-directory: projects/rocprofiler-sdk shell: bash run: + source ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }}/bin/activate && find build -type f | egrep '\.gcda$' | xargs rm && python3 ./source/scripts/run-ci.py -B build --name ${{ github.repository }}-${{ github.ref_name }}-${{ matrix.system.os }}-${{ matrix.system.gpu }}-codecov-samples @@ -324,6 +338,7 @@ jobs: working-directory: projects/rocprofiler-sdk shell: bash run: | + source ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }}/bin/activate echo "PWD: ${PWD}" ls -la @@ -424,6 +439,7 @@ jobs: shell: bash working-directory: projects/rocprofiler-sdk run: | + source ${GITHUB_WORKSPACE}/${{ env.PYTHON_VENV_PATH }}/bin/activate pushd build # # if following fails, there is a test that does not have diff --git a/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml b/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml index fe7df53d01b9..2619d7f29c4a 100644 --- a/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml +++ b/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml @@ -41,6 +41,7 @@ concurrency: env: ROCM_PATH: "/opt/rocm" PATH: "/usr/bin:$PATH" + CMAKE_PREFIX_PATH: "/opt/rocm/lib/rocm_sysdeps:/opt/rocm" jobs: rocm-release-compatibility: @@ -80,7 +81,7 @@ jobs: cd projects/rocprofiler-sdk git config --global --add safe.directory '*' apt update - apt install -y wget build-essential cmake g++-11 g++-12 python3-pip libdw-dev libsqlite3-dev libdrm-dev pkg-config + apt install -y wget build-essential g++-11 g++-12 python3-pip libdw-dev libsqlite3-dev libdrm-dev pkg-config # Older ROCm versions don't provide these packages. apt install -y rccl-dev rccl-unittests || true @@ -89,6 +90,14 @@ jobs: update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-11 10 --slave /usr/bin/g++ g++ /usr/bin/g++-11 --slave /usr/bin/gcov gcov /usr/bin/gcov-11 update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-12 20 --slave /usr/bin/g++ g++ /usr/bin/g++-12 --slave /usr/bin/gcov gcov /usr/bin/gcov-12 python3 -m pip install -U --user -r requirements.txt + # Held at 3.25 rather than the 4.x that requirements.txt resolves to: this + # branch's external submodules (json, pybind11, sqlite, yaml-cpp) still declare + # cmake_minimum_required below 3.5, which CMake 4 rejects outright. This must + # run after requirements.txt so the pin is not upgraded away. + python3 -m pip install -U --user cmake==3.25.0 + + # Add pip packages to path + echo "$HOME/.local/bin" >> "$GITHUB_PATH" - name: List Files shell: bash @@ -117,7 +126,6 @@ jobs: -- -DROCPROFILER_DEP_ROCMCORE=ON -DROCPROFILER_BUILD_{TESTS,SAMPLES,DOCS}=OFF - -DROCPROFILER_DISABLE_ATT_QUICK_SCAN=ON -DROCPROFILER_BUILD_AQLPROFILE=${{ matrix.aqlprofile == 'internal' && 'ON' || 'OFF' }} -DCMAKE_BUILD_TYPE=${{ matrix.build-type }} -DCMAKE_INSTALL_PREFIX="$(realpath /opt/rocm)" From 965dafd9da8dc7f515df9118427ed47d920a44cd Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 21 Sep 2026 06:07:10 +0000 Subject: [PATCH 21/21] ci(rocprofiler-sdk): keep ATT quick scan disabled in release compatibility Develop dropped -DROCPROFILER_DISABLE_ATT_QUICK_SCAN=ON because it builds rocprof-trace-decoder, but this branch does not, so rocprofiler-sdk-tool's find_package(rocprof-trace-decoder REQUIRED) fails to configure against every ROCm release container. Restore the flag. Co-authored-by: Vivek Kale --- .github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml b/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml index 2619d7f29c4a..cb4bbf6667c7 100644 --- a/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml +++ b/.github/workflows/rocprofiler-sdk-rocm_release_compatibility.yml @@ -126,6 +126,7 @@ jobs: -- -DROCPROFILER_DEP_ROCMCORE=ON -DROCPROFILER_BUILD_{TESTS,SAMPLES,DOCS}=OFF + -DROCPROFILER_DISABLE_ATT_QUICK_SCAN=ON -DROCPROFILER_BUILD_AQLPROFILE=${{ matrix.aqlprofile == 'internal' && 'ON' || 'OFF' }} -DCMAKE_BUILD_TYPE=${{ matrix.build-type }} -DCMAKE_INSTALL_PREFIX="$(realpath /opt/rocm)"