[Do Not review] [rocprofiler_sdk] SPM Review PR. by bwelton · Pull Request #922 · ROCm/rocm-systems

bwelton · 2025-09-10T01:24:59Z

Aggregate PR that breaks down the existing single commit to something a bit more reviewable.

Copilot

Pull Request Overview

This PR introduces Streaming Performance Monitor (SPM) support to the rocprofiler-sdk. SPM enables real-time monitoring of GPU performance counters during kernel execution with minimal overhead, providing an alternative to traditional counter collection methods.

Adds experimental SPM APIs for agent-based and dispatch-based counter collection
Implements SPM data structures, callbacks, and validation tests
Integrates SPM with the rocprofiler-sdk-tool for CSV and JSON output

Reviewed Changes

Copilot reviewed 53 out of 54 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
projects/rocprofiler-sdk/tests/spm/	Validation test framework for SPM dispatch collection
projects/rocprofiler-sdk/tests/rocprofv3/spm/	rocprofv3 tool integration tests comparing SPM vs PMC
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/	Core SPM implementation and service configuration
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm/	Public API headers for SPM functionality
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk-tool/	Tool integration for SPM data collection and output
projects/rocprofiler-sdk/samples/spm_agent_counter_collection/	Sample application demonstrating SPM agent monitoring

Comments suppressed due to low confidence (2)

projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/spm/spm_core.cpp:1

malloc can return nullptr on failure, but the function always returns HSA_STATUS_SUCCESS. Should check for allocation failure and return appropriate error status.

// MIT License

projects/rocprofiler-sdk/source/lib/output/generateStats.cpp:1

Duplicate function declaration. This appears to be a copy-paste error from the rocjpeg function above.

// MIT License

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-10T01:25:41Z

+
+    for record in counter_data:
+
+        if record["agent_id"]["handle"] in agent_counter_map.keys():


Using in agent_counter_map is more efficient than in agent_counter_map.keys() as it avoids creating a temporary view of keys.

Worth trying

Copilot · 2025-09-10T01:25:41Z

+        assert 1.0 * get_counter_value(counters, "SQ_INSTS_SALU") > get_counter_value(
+            counters, "SQ_WAVES"
+        )
+        assert 1.0 * get_counter_value(counters, "SQ_INSTS_VALU") > get_counter_value(
+            counters, "SQ_WAVES"
+        )


The assertion will fail if get_counter_value returns None for any counter. Add null checks or ensure get_counter_value returns a default value of 0.

Copilot · 2025-09-10T01:25:41Z

+    pmc_csv = pmc_csv[pmc_csv["Kernel_Name"].str.contains("matrixTranspose")]
+    spm_csv = spm_csv[spm_csv["Dispatch_Id"] == pmc_csv["Dispatch_Id"].values[0]]


This code assumes pmc_csv is not empty after filtering. If no kernels match 'matrixTranspose', accessing values[0] will raise an IndexError.

Copilot · 2025-09-10T01:25:42Z

+{
+    auto& counters = *reinterpret_cast<counter_vec*>(userdata);
+
+    // aqlprofile currently reports shadder_engine -1 as global counters


Misspelled 'shader' as 'shadder' in the comment.

## Associated JIRA ticket number/Github issue number  ## What type of PR is this? (check all applicable) - [ ] Refactor - [ ] Feature - [ ] Bug Fix - [ ] Optimization - [ ] Documentation Update - [ ] Continuous Integration ## What were the changes?  ## Why are these changes needed?  ## Updated CHANGELOG?  - [ ] Yes - [ ] No, Does not apply to this PR. ## Added/Updated documentation? - [ ] Yes - [ ] No, Does not apply to this PR. ## Additional Checks - [ ] I have added tests relevant to the introduced functionality, and the unit tests are passing locally. - [ ] Any dependent changes have been merged.

jrmadsen

I need some more time to go through all of this but here is some prelim feedback to work on in the meantime

jrmadsen · 2025-10-14T04:01:12Z

+endforeach()
+
+set_source_files_properties(kernel_branch.cpp PROPERTIES COMPILE_FLAGS "-g -O2")
+set_source_files_properties(kernel_branch.cpp PROPERTIES LANGUAGE HIP)


You can set the properties for more than one source file here

Removed samples

jrmadsen · 2025-10-14T04:11:11Z

        update_env("ROCPROF_PC_SAMPLING_UNIT", args.pc_sampling_unit)
        update_env("ROCPROF_PC_SAMPLING_METHOD", args.pc_sampling_method)
        update_env("ROCPROF_PC_SAMPLING_INTERVAL", args.pc_sampling_interval)
+    if args.spm:


Suggested change

if args.spm:

if args.spm:

jrmadsen · 2025-10-14T04:11:45Z

+                "spm: {}".format(" ".join(args.spm)),
+                overwrite=True,
+            )
+        if args.spm_buffer_size:


Suggested change

if args.spm_buffer_size:

if args.spm_buffer_size:

bwelton · 2025-10-14T21:35:56Z

+ * @retval ROCPROFILER_STATUS_SUCCESS if all counters found for agent
+ * @retval ROCPROFILER_STATUS_ERROR_AGENT_NOT_FOUND invalid agent
+ * @retval ROCPROFILER_STATUS_ERROR_INCOMPATIBLE_ABI incompatible aqlprofile version is used
+ * @retbal ROCPROFILER_STATUS_ERROR_AGENT_ARCH_NOT_SUPPORTED agent has no supported SPM counter


typo: retval

bwelton · 2025-10-14T21:42:45Z

+}
+
+bool
+AsyncSignalHandler(hsa_signal_value_t /*signal_v*/, void* data)


What is this async signal handler waiting on? Dispatch start or something else?

vlaindic

The first part of the review.

vlaindic · 2025-12-02T15:10:52Z

        counter_expression,
        counter_dimensions,
        is_hw_constant,
+        spm_support,


Same comment as the one above.

vlaindic · 2025-12-02T15:17:30Z

+    size_t spm_buffer_size             = get_env("ROCPROF_SPM_BUFFER_SIZE", 32768);
+    size_t spm_timeout_ms              = get_env("ROCPROF_SPM_TIMEOUT_MS", 50);
+    size_t spm_frequency_sclk          = get_env("ROCPROF_SPM_FREQUENCY_SCLK", 500000);


I think we should add this as the default arguments in the rocprofv3.py.

Also, for the spm_timeout_ms, I think you mentioned 30ms above, and 50 is used here, so we should make those two values consistent.

I have removed the default comment in rocprofv3.py since I need the values to be None if user has not input these values.

Sure. I just think we need to be consistent (30 or 50) and document the default values somewhere accessible to our users.

- Add experimental SPM API headers under experimental/spm/ - Add main experimental/spm.h include file - Update experimental CMakeLists.txt to include SPM headers - Remove spm.h These headers define the public interface for the SPM (System Performance Monitoring) feature. Add SPM core implementation - Add SPM core service implementation (spm_core.cpp/hpp) - Add SPM data decoding functionality (spm_decode.cpp/hpp) - Add SPM service interface (spm_service.cpp) - Add SPM dynamic loading support (spm_dlsym.cpp/hpp) - Add SPM CMakeLists.txt build configuration This implements the core SPM functionality for system performance monitoring. Add SPM samples and tests - Add SPM agent counter collection sample with GPU kernel examples - Add SPM test infrastructure with validation scripts - Update CMakeLists.txt to include SPM samples and tests Provides example usage and validation for the SPM functionality. Integrate SPM with SDK core - Add SPM to main SDK CMakeLists.txt build - Update registration.cpp to support SPM services - Add SPM context support in context.cpp/hpp Integrates SPM into the existing SDK architecture and service registration. Add SPM output format support - Add SPM domain type support for output generation - Add SPM counter information handling - Update CSV and stats generation for SPM data - Add SPM metadata support Enables SPM data to be output in standard rocprofiler formats. Add SPM support to rocprofv3 tool - Add SPM configuration options to tool config - Add SPM service integration to main tool - Update tool to handle SPM data collection Enables rocprofv3 tool to utilize SPM functionality for profiling. Add SPM support to SDK utilities - Add SPM enum string conversion support - Add SPM serialization support - Add SPM metrics integration - Update counter metrics for SPM - Add SPM hsa integration - Add SPM counters integration - Add SPM packet construct support Integrates SPM with SDK utility and serialization systems. Add SPM documentation and schema updates - Update rocprofv3 schema to include SPM options - Update rocprofv3.py script with SPM support Documents SPM feature in user-facing tools and schemas. Adding rocprofv3-avail support for SPM Revise API fix packet construct Fix CSV output Addressing review comments Removing internal queue files and clean up

* spm schema changes * Fill kernel dispatch table for spm counters * Update the spm tests --------- Co-authored-by: Srihari, Uttanur <srihariu@amd.com>

* use drain function * Removing extra barrier packets * fix

This reverts commit 10909ef.

jrmadsen · 2026-02-24T07:28:48Z

+
+add_executable(@PACKAGE_NAME@::agent_hsaco_targets IMPORTED)
+set_property(
+    TARGET @PACKAGE_NAME@::agent_hsaco_targets
+    PROPERTY
+        IMPORTED_LOCATION
+        ${@PACKAGE_NAME@_ROOT_DIR}/${PACKAGE_NAME}/tests/unit-tests/bin
+    )


What is this doing here?

I wanted to re-use the hsaco target built in counters unit test.

jrmadsen · 2026-02-24T07:32:37Z

@@ -153,6 +154,10 @@ struct config : output_config
    uint64_t att_param_perf_ctrl   = get_env<uint64_t>("ROCPROF_ATT_PARAM_PERFCOUNTER_CTRL", 0);
    uint64_t att_consecutive_kernels = get_env<uint64_t>("ROCPROF_ATT_CONSECUTIVE_KERNELS", 0);

+    size_t spm_buffer_size_kb = get_env<size_t>("ROCPROF_SPM_BUFFER_SIZE", 1 << 15);
+    size_t spm_timeout_ms     = get_env<size_t>("ROCPROF_SPM_TIMEOUT_MS", 0);
+    float  spm_frequency_ghz  = get_env<float>("ROCPROF_SPM_FREQUENCY", 0.5);


No, use an integer.

In a previous review, I said support decimal but convert it to an integer for the tool library.

Why integer? Can we use double here instead?

jrmadsen · 2026-02-24T07:34:06Z

+            }
+            else if constexpr(std::is_floating_point<Tp>::value)
+            {
+                return static_cast<Tp>(std::stof(env_var));


You should use std::stod

jrmadsen · 2026-02-24T07:35:45Z

@@ -146,12 +157,21 @@ SPECIALIZE_GET_ENV(uint8_t)
 SPECIALIZE_GET_ENV(uint16_t)
 SPECIALIZE_GET_ENV(uint32_t)
 SPECIALIZE_GET_ENV(uint64_t)
+SPECIALIZE_GET_ENV(float)


If you are going to add floating point support, include double

jrmadsen · 2026-02-24T07:37:01Z

@@ -163,6 +168,7 @@ struct config : output_config

    std::unordered_set<size_t>         kernel_filter_range    = {};
    std::vector<std::set<std::string>> counters               = {};
+    std::set<std::string>              spm_counters           = {};


Shouldn't this be std::vector<std::set<std::string>> like counters??

Currently, SPM does not support counter groups, so it is not a vector of string set. Can we add this later?

jrmadsen · 2026-02-24T08:16:06Z

+        auto event       = aqlprofile_pmc_event_t{};
+        event.block_name = static_cast<hsa_ven_amd_aqlprofile_block_name_t>(query_info.id);
+        event.event_id =
+            static_cast<uint32_t>(std::stoul(metric.event().c_str(), nullptr) & 0xFFFFFFFF);
+        event.flags = aqlprofile_pmc_event_flags_t{metric.flags()};


Initialize event with these values, do not assign like this.

jrmadsen · 2026-02-24T08:17:59Z

+    aqlprofile_spm_profile_t profile{};
+    profile.events          = events.data();
+    profile.event_count     = events.size();
+    profile.parameter_count = params.size();
+    profile.parameters      = params.data();
+
+    profile.aql_agent  = *aql_agent;
+    profile.hsa_agent  = pool->gpu_agent;
+    profile.alloc_cb   = &(hsa::SPMMemoryPool::Alloc);
+    profile.dealloc_cb = &(hsa::SPMMemoryPool::Free);
+    profile.memcpy_cb  = &(hsa::SPMMemoryPool::Copy);
+    profile.userdata   = pool.get();


Construct the object with these parameters.

jrmadsen · 2026-02-24T08:19:20Z

+    std::map<std::pair<hsa_ven_amd_aqlprofile_block_name_t, uint32_t>, int64_t> counter_count;
+    std::map<std::pair<hsa_ven_amd_aqlprofile_block_name_t, uint32_t>, int64_t> max_allowed;


See contribution guidelines. Go through every line of code in your PR and fix your local variable initialization.

jrmadsen · 2026-02-24T08:20:20Z

@@ -337,6 +337,7 @@ start_context(rocprofiler_context_id_t context_id)

    auto status = ROCPROFILER_STATUS_SUCCESS;

+    if(cfg->dispatch_spm) rocprofiler::spm::start_context(cfg);


What happens if spm::start_context fails?

jrmadsen · 2026-02-24T08:21:04Z


+    std::unique_ptr<spm_dispatch_counter_collection_service> dispatch_spm = {};
+


Suggested change

std::unique_ptr<spm_dispatch_counter_collection_service> dispatch_spm = {};

std::unique_ptr<spm_dispatch_counter_collection_service> dispatch_spm = {};

github-actions · 2026-05-14T17:41:04Z

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

Add new commits
Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

github-actions · 2026-05-19T22:07:16Z

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

Copilot AI review requested due to automatic review settings September 10, 2025 01:25

bwelton requested a review from a team as a code owner September 10, 2025 01:25

github-actions Bot added documentation Improvements or additions to documentation project: rocprofiler-sdk labels Sep 10, 2025

Copilot AI reviewed Sep 10, 2025

View reviewed changes

systems-assistant Bot added the organization: ROCm label Sep 10, 2025

SrirakshaNag force-pushed the bewelton/spm_review branch 3 times, most recently from bf81ba5 to 20d080c Compare September 11, 2025 01:21

bgopesh mentioned this pull request Sep 16, 2025

SPM Feature #820

Closed

1 task

SrirakshaNag force-pushed the bewelton/spm_review branch 5 times, most recently from acb0ba3 to 27b0edb Compare September 17, 2025 17:10

jrmadsen requested changes Sep 22, 2025

View reviewed changes

venkat1361 added the changes requested label Sep 23, 2025

jrmadsen requested changes Oct 14, 2025

View reviewed changes

bwelton commented Oct 14, 2025

View reviewed changes

bwelton assigned bwelton and meserve-amd Nov 11, 2025

SrirakshaNag removed the changes requested label Nov 12, 2025

SrirakshaNag force-pushed the bewelton/spm_review branch 5 times, most recently from 08205bf to bdff877 Compare November 27, 2025 03:23

vlaindic reviewed Dec 2, 2025

View reviewed changes

Comment thread projects/rocprofiler-sdk/source/include/rocprofiler-sdk/counters.h Resolved

Comment thread projects/rocprofiler-sdk/source/include/rocprofiler-sdk/experimental/spm.h Resolved

vlaindic requested changes Dec 2, 2025

View reviewed changes

SrirakshaNag force-pushed the bewelton/spm_review branch 6 times, most recently from ad81434 to 0391b7b Compare February 18, 2026 19:56

SrirakshaNag marked this pull request as draft February 19, 2026 01:56

SrirakshaNag force-pushed the bewelton/spm_review branch from 0391b7b to 7cb8d6f Compare February 19, 2026 01:58

SrirakshaNag marked this pull request as ready for review February 19, 2026 01:59

SrirakshaNag force-pushed the bewelton/spm_review branch 4 times, most recently from a566ff6 to df3d385 Compare February 20, 2026 22:48

Benjamin Welton and others added 12 commits February 23, 2026 17:49

Fix formatting and minor fixes

ec34df3

Fix test failures

7abdba4

Fix test failures

9457dae

Fix failure

d273710

Addresing review comments

54a5e80

Addressing review comments

803cea6

SPM rocpd schema fixes. (#2833)

737716f

* spm schema changes * Fill kernel dispatch table for spm counters * Update the spm tests --------- Co-authored-by: Srihari, Uttanur <srihariu@amd.com>

use drain function (#2827)

c553ec5

* use drain function * Removing extra barrier packets * fix

Revert "use drain function (#2827)"

ab0ffc4

This reverts commit 10909ef.

review comments

299de41

review comments

85f52d0

jrmadsen requested changes Feb 24, 2026

View reviewed changes

bwelton mentioned this pull request Feb 27, 2026

[rocprofiler-sdk] Add float support to get_env and complete set_env specializations #3591

Closed

3 tasks

fix

73e776d


		for record in counter_data:

		if record["agent_id"]["handle"] in agent_counter_map.keys():

		pmc_csv = pmc_csv[pmc_csv["Kernel_Name"].str.contains("matrixTranspose")]
		spm_csv = spm_csv[spm_csv["Dispatch_Id"] == pmc_csv["Dispatch_Id"].values[0]]

		std::map<std::pair<hsa_ven_amd_aqlprofile_block_name_t, uint32_t>, int64_t> counter_count;
		std::map<std::pair<hsa_ven_amd_aqlprofile_block_name_t, uint32_t>, int64_t> max_allowed;

		@@ -337,6 +337,7 @@ start_context(rocprofiler_context_id_t context_id)

		auto status = ROCPROFILER_STATUS_SUCCESS;

		if(cfg->dispatch_spm) rocprofiler::spm::start_context(cfg);


		std::unique_ptr<spm_dispatch_counter_collection_service> dispatch_spm = {};

Conversation

bwelton commented Sep 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrmadsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vlaindic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!