[bing-ma]

Motivation

Add gfx12/gfx1250 SPM support to aqlprofile.

Technical Details

This PR adds gfx12/gfx1250 SPM support across the aqlprofile runtime, factory, builder, metadata, and decode layers. The feature set includes:

• SA/WGP-aware SPM topology handling for gfx12/gfx1250
• explicit SPM slot-count metadata
• per-counter SPM depth propagation and validation
• mixed-depth SE/global builder support
• SPM capability/configuration query updates, including supported sample-interval reporting
• overflow-aware SPM decode behavior
• a public aqlprofile_spm_decode_shader_engine(...) helper for decoding the packed caller-facing shader_engine value returned by the SPM decode callback

Some shared-path helper/primitive behavior for gfx9/gfx10/gfx11 is adjusted where needed so the newer gfx12 path builds and behaves consistently, without changing the intended support scope of this PR.

Current validation/support-scope note:

• this PR validates the core gfx12/gfx1250 SPM path
• SP block and SQ / SQG level-counter support are outside the exercised support set for this PR and now warn instead of implying equivalent validation coverage

Issue Tracking

JIRA ID : AIPROFSDK-109

Test Plan

• gfx1201 (Radeon RX 9070 XT), parity and new features: validated using testv2
• gfx1250 (MI455X), parity and new features: validated using testv2
• gfx942 (MI300X), regression: validated using spm-counter-test ctests and rocprofv3

Test Result

Pass for the tests listed in test plan.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

[therock-pr-bot]

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

[therock-pr-bot]

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

• ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

[github-actions]

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.

[ApoKalipse-V]

A few notes:

• This touches a lot of code that gfx9 uses, so I think a MI300 needs to be part of the validation before merging.
• I don't see explicit handling of SP and accum(level) counters. Is this covered by the logic? If not, maybe we need to return an error when we see them for now as to not silently give users wrong results.
• There are a lot of possible states/combinations of programming when mixing 32 and 16 bit counters that doesn't seem possible to cover in integration testing. Maybe this needs unit tests for asserting the registers are set to the right place in these combinations.
• I'm just trusting the register programming is correct :D I did not verify that.
• This is missing formatting, but I'll argue we SHOULDN'T run formatting during review, otherwise the diff will be larger. Just a note it needs formatting eventually.
• This review doesn't cover all of spm_builder.h as I'm still looking at the logic there.

[sauverma93]

Some points from AI review-

Paths below are relative to projects/rocprofiler-sdk/source/lib/aqlprofile/ unless noted otherwise.

Supported-event query

• core/spm_v2.cpp → aqlprofile_spm_is_event_supported(): the GetGpuId() < MI200_GPU_ID || GetGpuId() > MI350_GPU_ID gate is unchanged, so gfx12/gfx1250 still return false from this query even though packet creation now supports them.
• core/spm_v2.cpp → aqlprofile_spm_is_event_supported() vs GetCounter()/resolve_spm_depth(): the depth resolver forces gfx12 SQG to 32-bit, but the query returns true for AQLPROFILE_SPM_DEPTH_32_BITS only when block_name == HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ. AQLPROFILE_BLOCK_NAME_SQG is a distinct enum value, so a caller that queries first gets false for the exact configuration the runtime will produce.
• core/spm_v2.cpp → resolve_spm_depth(): is_gfx9_sq uses GetGpuId() <= GFX10_GPU_ID and is_gfx12_sqg uses GetGpuId() > GFX10_GPU_ID. Per the gpu_id_t ordering in core/pm4_factory.h, gfx10 falls into the first and gfx11 into the second, so the "gfx12 SQG" rule also applies to gfx11 SQG. If that is intended, the naming and comment could reflect it; if not, the guards need narrowing.
• core/spm_v2.cpp → AllocateCounter(): is_gfx12_sqc compares block_des.id (a block table id) against HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ (an HSA enum). On gfx12 these coincide because SqcCounterBlockInfo in gfxip/gfx12/gfx12_block_table.h uses __BLOCK_ID_HSA(SQ); worth confirming the two domains are interchangeable on every target that reaches this path.

Block metadata

• def/gpu_block_info.h (GpuBlockInfo::spm_counter_count) + core/spm_v2.cpp → AllocateCounter(): the bound check moves from counter_count to the new spm_counter_count. gfxip/gfx9/gfx9_block_table.h and gfxip/gfx12/gfx12_block_table.h are updated; the gfx10 and gfx11 tables are not, so the field is 0 there. The query API already excluded those GPUs, but any create_packets path that bypasses the query would now throw "Event is out of block counter registers number limit".
• def/gpu_block_info.h (GpuBlockInfo, counter_des_t): both gain trailing fields that every block table initializes positionally, so a table that is missed gets a silent zero rather than a build error. Designated initializers, an explicit default, or a static_assert on table shape would make future additions safer.

WGP topology

• core/spm_v2.cpp → CountersVec() vs pm4/spm_builder.h → GpuSpmBuilder constructor: CountersVec() sizes the SA/WGP expansion from pm4_factory->GetNumWGPs() (which resolves to GpuPmcBuilder::wgp_per_sa_ in pm4/pmc_builder.h), while the SPM builder recomputes the same cu_num / 2 formula into its own wgp_per_sa_ and writes that into spm_buffer_desc->num_wgp. They agree today but can drift independently, and a mismatch corrupts decode rather than failing. One source of truth would avoid that.
• core/spm_decode.cpp → aqlprofile_spm_decode_stream_v1(): if desc->num_wgp is 0, wgp_count is 0 and size_t(sa_count) * size_t(wgp_count) - 1 underflows, advancing i_exp by ~2^64. Clamping wgp_per_sa_ to at least 1 in the builder and guarding here would contain it.

End() and delay programming

• pm4/spm_builder.h → GpuSpmBuilder::End() + gfxip/gfx12/gfx12_primitives.h → rlc_spm_perfmon_cntl_value(): End() unconditionally writes rlc_spm_perfmon_cntl_value(0, 0) on every generation. The gfx12 implementation computes sampling_rate - 1, so with sampling_rate == 0 the PERFMON_SAMPLE_INTERVAL field wraps to its maximum rather than zero. If the intent is a single flush pulse, the value probably needs to be explicit.
• pm4/spm_builder.h → GpuSpmBuilder::Begin(), guarded by Primitives::SPM_DELAY_PROGRAMMING_REQUIRED (false in gfxip/gfx10/gfx10_primitives.h and gfxip/gfx11/gfx11_primitives.h): this skips delay programming that previously ran on those targets. If those writes were inert because the blocks use BLOCK_DELAY_NONE, that is worth stating in the description; otherwise it is a behavior change.
• pm4/spm_builder.h → GpuSpmBuilder::Begin(): the RLC_SPM_PERFMON_SAMPLE_DELAY_MAX write is no longer preceded by a GRBM_GFX_INDEX broadcast write, so it now executes under whatever GRBM index is current at that point.

Decode path and descriptor contract

• core/spm_decode.cpp → encode_spm_shader_engine(), called from aqlprofile_spm_decode_stream_v1(): this packs wgp << 24 | sa << 16 | se into the shader_engine argument of aqlprofile_spm_decode_callback_v1_t (declared in aql_profile_v2.h), which previously carried -1 for global or a plain SE index. The in-tree consumer at source/lib/rocprofiler-sdk/spm/decode.cpp → decode_cb() still treats < 0 as global and anything else as an SE id, so packed values will be misread. This needs either a documented v1 encoding with consumers updated, or a v2 callback.
• pm4/spm_builder.h → GpuSpmBuilder::Begin() (spm_buffer_desc->num_events = original_event_count) vs core/spm_decode.cpp: the descriptor now reports the original event count while counter_map holds the full expanded vector, so SpmBufferDesc (in core/spm_common.hpp) no longer describes the length of its own map and the decoder walks past num_events using out-of-band knowledge. Note aqlprofile_spm_decode_query() returns this same value for AQLPROFILE_SPM_DECODE_QUERY_EVENT_COUNT. Carrying both counts explicitly would make the descriptor self-describing.
• core/spm_decode.cpp → aqlprofile_spm_decode_stream_v1(): the 32-bit high word is read as datain[index + 16] with a literal stride and no bounds check. The outer datain + seg_elem > data_end guard does not cover index + 16, and the global odd line is only reserved when has_global_32bit_spm is set in pm4/spm_builder.h.
• core/spm_decode.cpp → decode_bufvalue(): all-ones maps to uint64_t(-1), which is indistinguishable from a legitimate saturated value — particularly in 16-bit clamp mode, where 0xFFFF is a reachable counter value. A separate overflow indicator would avoid the ambiguity for consumers.
• core/spm_common.hpp (SPM_COUNTER_MAP_INDEX_MASK): the usable index narrows from 15 bits to 12. Nothing in pm4/spm_builder.h appears to check that even_idx / odd_idx stay under 4096 for large segment sizes.

gfx12 primitives

• gfxip/gfx12/gfx12_primitives.h: mux_info_t changes bitfield widths and ordering (counter 6→5, instance 5→6, fields reordered) and SQ_BLOCK_ID moves from __BLOCK_ID_HSA(SQ) to __BLOCK_ID(SQG). Both feed mux RAM programming and the block dispatch in pm4/spm_builder.h → Begin().

Build hooks

• CMakeLists.txt: DEBUG_TRACE is driven by ENV{CMAKE_DEBUG_TRACE} at configure time. Environment variables are not cached and do not trigger reconfiguration, so a normal cache option would be more predictable.
• core/spm_v2.cpp → _internal_aqlprofile_spm_create_packets() + core/memorymanager.hpp → SPMMemoryManager::CreateSampleCountBuf(): under DEBUG_TRACE this adds a device allocation that can return HSA_STATUS_ERROR_OUT_OF_RESOURCES, so the debug build can fail where release succeeds.
• pm4/spm_builder.h: now includes core/logger.hpp for WARN_LOGGING. Worth checking that standalone consumers of the pm4 headers still build and link cleanly, including pm4/tests/trace_config_test.cpp.

Scope

• The change combines gfx12 SPM support, a cross-generation primitive signature refactor (sq_spm_select_value(), rlc_spm_perfmon_cntl_value(), rlc_spm_perfmon_segment_size_value(), spm_timestamp_muxsel() across all four gfxip/*/*_primitives.h), the End() stop-path fix, and the DEBUG_TRACE tooling in one commit. Splitting these would make the non-gfx12 risk easier to reason about and bisect.

[vlaindic]

Not an expert in the field, so adding couple comments and mostly agreeing with Gio's comments. I'm also adding the full AI review in case it will help. Thanks!

Review — PR #9646

[rocprofiler-sdk][aqlprofile][gfx12][spm] Add gfx12/gfx1250 SPM core support
JIRA: AIPROFSDK-109 · 24 files, ~3019 diff lines, all under projects/rocprofiler-sdk/source/lib/aqlprofile/

Verdict: Request changes.

This is a substantial and largely well-constructed change — the SE/SA/WGP expansion model is
coherent, several long-standing hardcoded assumptions are correctly removed, and the hardware
comments in gfx12_block_table.h and spm_builder.h are genuinely good. The blockers below are
mostly about the edges of the new model: uninitialized table fields, an unguarded arithmetic
underflow, and an encoder/decoder contract that is computed independently in two places.

Note: the PR is currently labeled "Not ready to Review". If this is still WIP, treat the
CRITICAL section as the priority list and ignore the rest for now.

---

CRITICAL

1. spm_counter_count is uninitialized for gfx10, gfx11, and 8 gfx9 blocks — SPM will throw on the first counter

GpuBlockInfo::spm_counter_count is the last struct member and has no default initializer
(def/gpu_block_info.h:137). Every block table uses positional aggregate initialization, so any
table entry that doesn't explicitly supply the field gets 0.

Its only consumer is the allocation guard in core/spm_v2.cpp:286:

if(reg_index >= block_info->spm_counter_count)
    throw std::runtime_error("Event is out of block counter registers number limit");

reg_index starts at 0, so 0 >= 0 is true and this throws on the first SPM counter for any
block with an unset count.

Scope:

• gfx10 — 16 GpuBlockInfo entries, zero spm:: references. gfxip/gfx10/gfx10_block_info.h
  has no namespace spm at all.
• gfx11 — 19 GpuBlockInfo entries, zero spm:: references. Same.
• gfx9 — 29 CounterBlockInfo aggregates, 21 updated → 8 missed. Confirmed by inspection:
  SqGsCounterBlockInfo (carries CounterBlockSqAttr), WdCounterBlockInfo, McVmL2CounterBlockInfo
  all still end at BLOCK_DELAY_NONE};.

This is not dormant code on gfx10/gfx11 — both instantiate GpuSpmBuilder
(core/gfx10_factory.cpp:80, core/gfx11_factory.cpp:53), both define RLC_SPM_PERFMON_CNTL__ADDR
(gfx10_primitives.h:125, gfx11_primitives.h:130), and both have blocks carrying
CounterBlockSpmGlobalAttr (gfx10_block_table.h:436,447,458, gfx11_block_table.h:494,505,516).
SPM is live on those parts today and this PR breaks it.

Fix:

1. Give the field a default so omission is safe rather than silently fatal:

   uint32_t spm_counter_count = 0;

2. Populate spm_counter_count for the gfx10 and gfx11 tables, and for the 8 remaining gfx9 blocks.
3. Replace the bare >= guard with an explicit unsupported-block diagnostic so a future missed
   entry produces a clear message instead of "out of block counter registers number limit":

   if(block_info->spm_counter_count == 0)
       throw std::runtime_error(std::string("SPM not supported for block ") + block_info->name);
   if(reg_index >= block_info->spm_counter_count)
       throw std::runtime_error("Event is out of block counter registers number limit");

---

2. Unsigned underflow programs the maximum sample interval on every SPM stop

pm4/spm_builder.h:781-783 (End()):

// Force one last RLC sample pulse before SPM stop so the current partial interval is
// materialized into the stream instead of being dropped when CP_PERFMON_CNTL stops/reset SPM.
builder.BuildWriteUConfigRegPacket(cmd_buffer,
                                   Primitives::RLC_SPM_PERFMON_CNTL__ADDR,
                                   Primitives::rlc_spm_perfmon_cntl_value(0, 0));

gfx12_primitives.h:545 now computes sampling_rate - 1:

SET_REG_FIELD_BITS(RLC_SPM_PERFMON_CNTL, PERFMON_SAMPLE_INTERVAL, sampling_rate - 1)

With sampling_rate == 0 this underflows to 0xFFFFFFFF and the field is programmed to all-ones —
the largest possible interval, which is the exact opposite of the comment's stated intent of forcing
a pulse. This fires on every SPM stop, not an edge case.

Fix — two parts:

1. Guard the arithmetic so no caller can underflow it:

   const uint32_t encoded_interval = (sampling_rate > 0) ? (sampling_rate - 1) : 0u;

2. Confirm with hardware whether PERFMON_SAMPLE_INTERVAL = 0 actually forces a sample pulse. If it
   does not, the End() write doesn't do what the comment claims and should be removed rather than
   fixed — the comment is currently the only evidence the mechanism works.

---

3. num_sa is hardcoded to 2 while the encoder expands using the real SA count

Three places disagree about the same value:

Location	Value used
core/spm_v2.cpp:480	trace_config.sa_number = pm4_factory->GetGpuId() >= GFX10_GPU_ID ? 2 : 0;
core/spm_v2.cpp:303 (CountersVec())	pm4_factory->GetShaderArraysNumber()
core/spm_decode.cpp:100	sa_count = is_sa ? desc->num_sa : 1;

spm_builder.h:709 copies sa_number into SpmBufferDesc::num_sa, which the decoder then uses as
its stride. The encoder expands counters using the real shader_arrays_per_se; the decoder
walks them assuming 2. On any gfx10+ part where shader_arrays_per_se != 2 the two disagree and
every counter is attributed to the wrong SA/WGP — silently, with no error and no obviously wrong
values.

Fix: drive trace_config.sa_number from pm4_factory->GetShaderArraysNumber(), the same source
CountersVec() uses. If gfx9 genuinely needs 0 as a "not applicable" sentinel, keep that branch
but derive the gfx10+ branch from the agent info.

---

4. wgp_per_sa is computed independently in two places

pm4/spm_builder.h:92-105 (constructor):

wgp_per_sa_ = (agent_info->cu_num / 2 + sa_number * se_number - 1) / (se_number * sa_number);
wgp_per_sa_ /= xcc_number;

pm4/pmc_builder.h:372-374 (GetNumWGPs()) computes the same quantity separately, and
CountersVec() in spm_v2.cpp uses pm4_factory->GetNumWGPs() — i.e. the other one.

So the encode side (spm_builder.h) and the expansion side (spm_v2.cpp) are driven by two
independent computations of a value that must agree exactly for decode to work. Any future edit to
one is a silent correctness bug.

Two secondary problems in the spm_builder.h copy:

• No zero-guard on xcc_number before wgp_per_sa_ /= xcc_number.
• cu_num / 2 + sa_number * se_number - 1 underflows if cu_num == 0 and sa_number * se_number == 0
  — the if(se_number && sa_number) guard covers the divisor but the numerator is evaluated inside it.

Fix: compute this once and thread it through TraceConfig alongside the existing
se_number / sa_number / xcc_number fields, e.g. add wgp_number. Delete the constructor copy
and have GpuSpmBuilder read config->wgp_number.

---

5. counter_map writes are unbounded — user-controllable overflow into the SPM data region

SPM_DESC_SIZE is a fixed 4 KB. SpmBufferDesc is followed by a raw uint16_t* counter_map with
roughly 2030 usable slots, and the SPM data region begins immediately after at
output_buffer_ptr + SPM_DESC_SIZE (spm_v2.cpp:561).

Before this PR the number of counter_map writes equalled the user's event count. After it, the
expansion in CountersVec() makes it num_events × sa_count × wgp_count — on gfx1250 that is
2 SA × ~8 WGP = 16×. There is no bound check anywhere on the write path. Roughly 127
user-specified WGP events is enough to run past the descriptor and corrupt the beginning of the SPM
data stream.

profile.event_count is user-controllable, so this is reachable from the public API without any
privileged input.

Fix: define the capacity explicitly and reject over-subscription with a real error rather than
overflowing:

static constexpr size_t kMaxCounterMapEntries =
    (SPM_DESC_SIZE - sizeof(SpmBufferDesc)) / sizeof(uint16_t);

if(counters_vec.size() > kMaxCounterMapEntries)
    return HSA_STATUS_ERROR_OUT_OF_RESOURCES;

Place the check after expansion in CountersVec() so it accounts for the SA/WGP multiplier.

---

6. Three different bit layouts for the same SE/SA/WGP index, plus a 6-bit overflow

The encoded block index has three inconsistent representations:

Location	Layout
gfx12_factory.cpp:64-73, gfx1250_factory.cpp:42-51	(sa_index << 5) | (wgp_index << 1) | inst_index
gfx12_primitives.h:491-496 (decode_spm_instance_index)	& 0x1 for WGP blocks, & 0x1F for SA blocks
gfx12_primitives.h:294 (grbm_inst_se_sh_wgp_index_value)	(wgp_side << 6) | (wgp_index << 2) | instance_index

Concretely:

• The SA-block mask 0x1F covers bits 0-4, which includes the entire wgp_index field (bits
  1-4). It does not isolate the instance index.
• The encoder puts WGP at bit 1; the GRBM helper puts it at bit 2. These describe the same logical
  quantity with different shifts.
• The encoded value is stored into mux_info_t.gfx.instance, which this PR narrows to 6 bits
  (gfx12_primitives.h:172). With sa_index = 1, sa << 5 alone is 0x20; wgp_index >= 16
  pushes the value to 0x40+ and truncates into the adjacent block field, corrupting the muxsel
  word.

This also makes the validation in core/pm4_factory.h:199,215 check the wrong quantity, since it
now bounds-checks DecodeSpmInstanceIndex(info, event->block_index) against instance_count.

Fix: define the layout once, next to mux_info_t, and derive everything from it:

static constexpr uint32_t SPM_INST_BITS  = 1;
static constexpr uint32_t SPM_WGP_BITS   = 4;
static constexpr uint32_t SPM_WGP_SHIFT  = SPM_INST_BITS;
static constexpr uint32_t SPM_SA_SHIFT   = SPM_INST_BITS + SPM_WGP_BITS;
static_assert(SPM_SA_SHIFT + 1 <= 6, "encoded index must fit mux_info_t.gfx.instance");

Then express the encoder, both decode masks, and the GRBM helper in terms of those constants, and
add a runtime check that wgp_index < (1u << SPM_WGP_BITS).

---

IMPORTANT

7. Dead code: #if PER_WGP_INST_SELECT

pm4/spm_builder.h:629-647 is guarded by PER_WGP_INST_SELECT, which is never defined anywhere in
the repository — a grep across projects/rocprofiler-sdk/ returns exactly one hit, the #if
itself. The block never compiles, so it is never type-checked.

It also uses (wgp << 2), contradicting EncodeSpmBlockIndex's (wgp << 1) — so if it were ever
enabled it would disagree with finding #6's encoder. And #if on an undefined macro warns under
-Wundef.

Fix: delete the block. If it's a genuine alternative that may be needed, move it to a design note
or a tracked ticket rather than leaving uncompiled code in the header.

---

8. DEBUG_TRACE plumbing has five separate problems

a. Env-var-driven build configuration (CMakeLists.txt):

if(DEFINED ENV{CMAKE_DEBUG_TRACE})
    if("$ENV{CMAKE_DEBUG_TRACE}" STREQUAL "1")
        target_compile_definitions(rocprofiler-sdk-aqlprofile PRIVATE DEBUG_TRACE=1)
    endif()
    ...

This is invisible to cmake -D, doesn't appear in CMakeCache.txt, doesn't show up in
cmake -LAH, and makes builds non-reproducible from the command line alone. It also uses an
un-namespaced CMAKE_-prefixed name, which is reserved-looking.

b. Values other than 1 and 2 are silently ignored — CMAKE_DEBUG_TRACE=3 produces a normal
build with no diagnostic.

c. Production headers carry debug-only members. pm4/trace_config.h:63 and
core/memorymanager.hpp:391-402 (CreateSampleCountBuf / GetSampleCountBuf /
sample_count_buf_) exist solely to serve the DEBUG_TRACE path but are unconditionally compiled
into every build.

d. core/spm_v2.cpp:462-471 uses catch(...), discarding the reason the operation failed.

e. pm4/spm_builder.h:715-737 writes diagnostics with std::cout and pulls
<iomanip>, <iostream>, and <mutex> into a header included widely across the library.

Fix:

option(ROCPROFILER_SDK_AQLPROFILE_SPM_DEBUG_TRACE
       "Enable verbose SPM muxsel tracing (0=off, 1=summary, 2=full)" 0)
target_compile_definitions(rocprofiler-sdk-aqlprofile
    PRIVATE ROCPROFILER_SDK_AQLPROFILE_SPM_DEBUG_TRACE=${ROCPROFILER_SDK_AQLPROFILE_SPM_DEBUG_TRACE})

Route the output through the existing logger rather than std::cout, wrap the debug-only members in
#if, and replace catch(...) with catch(const std::exception& e) plus a log of e.what().

---

9. EncodeSpmBlockIndex / DecodeSpmInstanceIndex are byte-identical in two factories

core/gfx12_factory.cpp:64-73 and core/gfx1250_factory.cpp:42-51 contain the same two overrides
character-for-character. Both delegate to gfx12_cntx_prim::decode_spm_instance_index, so the shared
logic already lives in a shared place — only the override boilerplate is duplicated.

Fix: hoist both overrides into the common gfx12 factory base and let Mi450Factory inherit them.
If Mi450Factory doesn't derive from Gfx12Factory, add a small shared mixin rather than copying.

---

10. Unrelated PMC and SQTT behavior changes are bundled into an SPM PR

Four changes in this diff affect code paths outside SPM:

• gfx12_primitives.h:80 — SQ_PERFCOUNTER_MASK_ADDR changes from Register(0xD9E1) to Register{}.
• pm4/sqtt_builder.h:227-231 — the write of config->perfMASK is now conditional on that register
  being non-null. Combined with the line above, gfx12 thread trace silently stops programming
  perfMASK.
• pm4/pmc_builder.h:661 — the guard changes from Primitives::GFXIP_LEVEL == 9 to a non-null
  register check, altering which generations write SQ_PERFCOUNTER_MASK_ADDR on the PMC path.
• core/aql_profile.cpp — trace_config.spm_sq_32bit_mode = true; removed from the v1 start path.

None of these are SPM. The SQTT one is a user-visible behavior change to thread trace with no test
and no mention in the description. A future PMC or ATT bisect will land on a commit titled "gfx12
SPM support".

Separately, 0xD9E1 is a bare offset with no HWIP/base-index, unlike every other register in these
files — worth a comment explaining why, if the value is retained anywhere.

Fix: split the PMC and SQTT changes into their own PR with their own justification and test
coverage. If they must ship together, state explicitly in the description that gfx12 thread trace no
longer programs perfMASK and why that is correct.

---

11. Runtime if where if constexpr is used elsewhere in the same file

pm4/spm_builder.h:510-513 and 552-555 use if(Primitives::GFXIP_LEVEL >= 11) on a
compile-time constant, while lines 96, 415, and 494 in the same file correctly use if constexpr.

Fix: make these if constexpr for consistency. Better still, key off the capability rather than
the level — if constexpr(!(Primitives::SQG_PERFCOUNTER_CTRL2_ADDR == Register())) expresses the
actual dependency and won't need editing when gfx13 lands.

---

12. aqlprofile_spm_is_event_supported() skips the bounds check on the 32-bit path

core/spm_v2.cpp:788-793:

const bool is_sq_path = event.block_name == HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ;
if(depth == AQLPROFILE_SPM_DEPTH_32_BITS) return is_sq_path;
if(depth == AQLPROFILE_SPM_DEPTH_16_BITS)
    return !is_sq_path && event.block_name < blocks.size() && blocks.at(event.block_name);
return false;

Two issues:

• The 32-bit branch returns without the event.block_name < blocks.size() check the 16-bit branch
  performs.
• It disagrees with resolve_spm_depth() (spm_v2.cpp:225-255), which permits explicit 32-bit depth
  on blocks other than SQ. So is_event_supported() reports false for configurations the
  allocator will happily accept.
• AQLPROFILE_SPM_DEPTH_64_BITS falls through to return false with no diagnostic.

Fix: rewrite as a switch over the depth enum with no default: label, so adding a depth
produces a -Wswitch warning rather than a silent false. Apply the same block-name bounds check on
every branch, and make it consult resolve_spm_depth() so the two agree by construction.

---

13. get_original_event_count() conflates "unset" with "zero"

pm4/cmd_config.h:53-56:

void   set_original_event_count(size_t count) { original_event_count_ = count; }
size_t get_original_event_count() const
{
    return original_event_count_ ? original_event_count_ : this->size();
}

The this->size() fallback is exactly the pre-PR behavior this field exists to correct. If any code
path constructs the config without calling the setter, the bug returns silently with no way to
detect it. original_event_count_ is also not initialized in the member list.

Fix: use std::optional<size_t> and let the accessor assert or throw when unset, so a missed
setter call fails loudly at the point of the mistake rather than producing subtly wrong counts.

---

14. No workflow comment block for the SPM subsystem

SPM is a subsystem with a clear lifecycle — Begin() → sampling → End(), with SPMMemoryManager
holding resources across the whole span, plus descriptor setup, muxsel RAM programming, and buffer
teardown. This PR substantially rewrites all three phases (spm_builder.h alone is +414/-151), and
there is no comment anywhere describing the sequence.

For a reader arriving at spm_builder.h cold, the relationship between Begin(), the muxsel
programming loop, the counter-map writes, and End() has to be reconstructed from the code.

Fix: add a block at the top of pm4/spm_builder.h:

// SPM workflow:
//   1. Initialization — TraceConfig is populated (se/sa/wgp counts, depth policy); the SPM
//      descriptor is allocated at output_buffer_ptr; muxsel RAM is programmed from the
//      expanded counter vector; RLC_SPM_* segment sizes and sample interval are set.
//   2. Usage          — RLC samples autonomously into the ring at PERFMON_SAMPLE_INTERVAL;
//      no host interaction is expected between Begin() and End(). Not reentrant; one active
//      SPM session per queue.
//   3. Destruction    — End() issues a final sample pulse, stops CP_PERFMON_CNTL, and the
//      buffer is handed to the decoder. counter_map in the descriptor must outlive decode.

Fill in the reentrancy and ownership lines with whatever is actually true — those are the parts a
reviewer cannot infer.

---

15. Description doesn't call out the workarounds

Three non-obvious things in this PR are documented in the code but invisible to someone reading
the description first:

• CounterRegInfo::select1_addr is repurposed for SQC/SP to hold the odd
  SQ_PERFCOUNTERn_SELECT rather than its nominal meaning (gfx12_block_table.h:217-222).
• The gfx1250 SP block reuses SQC's SPM identity — SPM_SE_BLOCK_NAME_SQC and
  spm::SqcCounterBlockNumCounters — with no explanation of why SP shares SQC's counter block.
• GFX12 requires GRBM broadcast rather than explicit SA/WGP topology addressing in the muxsel
  path (spm_builder.h:616-623).

The first and third have good in-code comments. The second has none. All three are the kind of thing
a reviewer should be told about up front rather than discovering mid-diff.

Fix: add a "Workarounds and non-obvious decisions" paragraph to the PR description naming all
three, and add an inline comment above the SP block entry explaining the SQC identity reuse.

---

SUGGESTIONS

16. Missing docstrings and inline comments on the new logic

Functions without docstrings:

• EncodeSpmBlockIndex / DecodeSpmInstanceIndex (both factories) — these define the encode/decode
  contract that findings #3, #4, and #6 all concern. This is the single most valuable docstring in
  the PR: state the bit layout, which side produces it, which side consumes it, and the valid range
  of each field.
• CreateSampleCountBuf / GetSampleCountBuf (memorymanager.hpp:391-402) — ownership and
  lifetime of the returned pointer are not obvious.
• counter_map_value lambda (spm_builder.h) — the flag encoding it produces.
• set_original_event_count / get_original_event_count (cmd_config.h) — why the distinction from
  size() exists at all.

Changes without inline comments:

• mux_info_t field reorder and resize (gfx12_primitives.h:172) — counter 6→5 bits, instance
  5→6 bits, and the field order changes. This is a wire-format change to the muxsel word with no
  comment explaining what hardware requires it.
• SPM_COUNTER_MAP_INDEX_MASK narrowing from 0x7FFF to 0x0FFF (spm_common.hpp:22) — the index
  space shrinks from 15 to 12 bits to make room for the new flags. Worth stating the resulting
  maximum index.
• TCP SPM clamp switched from PERFMON_SPM_MODE_32BIT_CLAMP to PERFMON_SPM_MODE_16BIT_CLAMP in
  both TCP select functions — a counter-semantics change with no rationale.
• SQ_BLOCK_ID changed from __BLOCK_ID_HSA(SQ) to __BLOCK_ID(SQG).
• spm_timestamp_muxsel() changed from a constant 0xF0F0 to a computed per-index value.
• counter_des.index / 2 and index = counter_des.index >> 2 / select = (counter_des.index % 4) >> 1
  in the select-register addressing — the div/shift relationship to the register layout is not
  self-evident.
• The asymmetric counter_map[index] assignment in the odd-lane loops (both global and SE): the
  32-bit branch writes mux_ram but omits counter_map[index], while the 16-bit branch assigns it.
  If intentional, say so; it reads as an omission.

17. Test coverage is effectively zero

The only test change in 3019 lines is two swapped assertions in
pm4/tests/trace_config_test.cpp — asserting a struct default, not any new behavior. No exemption
applies: this is new hardware functionality, not a build, docs, test-only, or rename change.

pm4/tests/pmc_builder_harvest_gfx12_tests.cpp already constructs synthetic AgentInfo values and
tests GetNumWGPs() — it is the natural place to extend rather than starting a new file.

Four concrete tests, each covering one of the CRITICAL findings:

1. Encode/decode round-trip over the full (sa, wgp, inst) domain for gfx12 and gfx1250 — assert
   DecodeSpmInstanceIndex(EncodeSpmBlockIndex(sa, wgp, inst)) == inst and that no encoded value
   exceeds 6 bits. Catches #6.
2. Expansion count and contiguity — for a known AgentInfo, assert CountersVec() produces
   exactly num_events × sa × wgp entries with contiguous indices, and that the sa/wgp used
   match TraceConfig::sa_number. Catches #3 and #4.
3. rlc_spm_perfmon_cntl_value(0, 0) — assert the PERFMON_SAMPLE_INTERVAL field is not
   all-ones. Catches #2.
4. Counter-map capacity — assert an event count that would exceed kMaxCounterMapEntries after
   expansion returns an error rather than writing past the descriptor. Catches #5.

A fifth, cheap and high-value: iterate every GpuBlockInfo in every generation's table and assert
that any block with an SPM attribute has spm_counter_count > 0. That single test catches #1 across
all four generations and prevents the next table from regressing.

18. decode_cb's se argument silently becomes a packed triple

core/spm_decode.cpp:49-53:

static inline int
encode_spm_shader_engine(uint32_t se_index, uint32_t sa_index = 0, uint32_t wgp_index = 0)
{
    return int((wgp_index << 24) | (sa_index << 16) | se_index);
}

This value is passed to aqlprofile_spm_decode_callback_v1_t as the se_index parameter. Existing
callers that treat it as a shader-engine index will now receive a packed
(wgp << 24) | (sa << 16) | se value and misbehave — with no compile error, since the type is
unchanged.

Fix: either document the new encoding in the public header alongside the callback typedef and
bump the interface version, or add a v2 callback and keep v1 emitting a bare SE index. Silently
changing the meaning of an existing public parameter is the one option that shouldn't ship.

19. Minor items

• pm4/spm_builder.h:410-413 — missing braces on a multi-line if body.
• spm_v2.cpp:333 and spm_builder.h:340 — static aqlprofile_spm_depth_t depth[] should be
  static constexpr std::array<aqlprofile_spm_depth_t, N>.
• spm_decode.cpp:90,114,124 — i_exp / event_exp_start / event_i_exp are hard to keep straight
  in a nested loop; expanded_idx / event_base / event_expanded_idx would read better. Same for
  mxinf / mxinf_filler → mux_info / mux_info_filler.
• spm_builder.h:553,597,610 — int je, jo; compared against .size() triggers sign-compare
  warnings; use size_t.
• spm_decode.cpp:77,81 — return uint64_t(-1); should be a named constexpr
  (kInvalidCounterValue) and documented, since it reaches user callbacks as a sentinel.
• spm_v2.cpp:267-294 — is_gfx12_sqc tests HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ, not SQC. Rename
  to match what it checks.
• spm_v2.cpp:446-454 — the REFCLK gfx12+ restriction returns a bare
  HSA_STATUS_ERROR_INVALID_ARGUMENT with no log line. Add a WARN_LOGGING naming the unsupported
  combination, and document the restriction where spm_sample_interval_type is declared.
• Conflicting SPM select programming is warned about but not rejected (spm_builder.h:697-704) —
  consider whether a conflicting select should be an error rather than a warning followed by
  whichever write lands last.

---

What's good here

Worth saying explicitly, since the list above is long:

• SPM_COUNTER_MAP_*_FLAG named constants (spm_common.hpp:18-22) replace bare literals in the
  counter-map encoding — a clear readability win.
• (4 * se_nlines) → (se_number * se_nlines) in rlc_spm_perfmon_segment_size_value removes a
  hardcoded 4-SE assumption that would have been a real bug on non-4-SE parts.
• The decode_bufvalue lambda (spm_decode.cpp:74-83) de-duplicates four copies of the
  16/32-bit value reconstruction and centralizes the sentinel handling.
• Splitting GetCounter from AllocateCounter cleanly separates depth policy from register
  assignment.
• if constexpr(SPM_DELAY_PROGRAMMING_REQUIRED) is the right tool for a compile-time capability
  check (see #11 — just apply it consistently).
• WARN_LOGGING on conflicting SPM select is a good instinct even if it should arguably be an
  error.
• Two genuinely excellent hardware comments: the SQC select1_addr repurposing explanation
  (gfx12_block_table.h:217-222) and the GFX12 SA/WGP GRBM broadcast constraint block
  (spm_builder.h:616-623). Both explain why, name the constraint, and would have taken a reviewer
  a long time to derive. More of these, please — particularly on the mux_info_t reorder and the TCP
  clamp change.

---

Hygiene summary

Check	Result
A — JIRA ID	Pass — AIPROFSDK-109, correct format
A — Description completeness	Fail — no test-status section
A2 — Workflow comment block	Fail — see #14
B — Test coverage	Fail — one default-value assertion; no exemption applies
C — Test adequacy	Fail — no new behavior is exercised
D — Feature flag	Fail — new GPU path + PMC/SQTT behavior changes ship unconditionally
E — Adjacent test reuse	Extend pm4/tests/pmc_builder_harvest_gfx12_tests.cpp
F — Risk	High — new GPU code path and changes to default runtime behavior on three shipping generations, without tests and without a flag
G — Unicode / injection scan	Pass — no non-ASCII, zero-width, bidi, or homoglyph characters

License header year (2026): no newly added files in this diff — all 24 are pre-existing. Exemption
applies and no header is flagged. pm4/spm_builder.h (+414/-151) is close to the "substantial
rewrite" threshold if you want to bump it.

---

Suggested priority

1. #1 spm_counter_count — breaks SPM on gfx10/gfx11 today
2. #5 counter_map overflow — user-reachable memory corruption
3. #2 underflow in End() — fires on every stop
4. #3 / #4 / #6 encoder/decoder contract — silent wrong data
5. #10 split out the PMC/SQTT changes
6. #17 add the five tests, which lock down 1-6

[bing-ma]

@ApoKalipse-V, this is the reply to your general notes:

A few notes:

• This touches a lot of code that gfx9 uses, so I think a MI300 needs to be part of the validation before merging.
• I don't see explicit handling of SP and accum(level) counters. Is this covered by the logic? If not, maybe we need to return an error when we see them for now as to not silently give users wrong results.
• There are a lot of possible states/combinations of programming when mixing 32 and 16 bit counters that doesn't seem possible to cover in integration testing. Maybe this needs unit tests for asserting the registers are set to the right place in these combinations.
• I'm just trusting the register programming is correct :D I did not verify that.
• This is missing formatting, but I'll argue we SHOULDN'T run formatting during review, otherwise the diff will be larger. Just a note it needs formatting eventually.
• This review doesn't cover all of spm_builder.h as I'm still looking at the logic there.

1. MI300 validation breadth
Agreed. Because this PR touches shared gfx9/gfx10/gfx11-facing plumbing, MI300 regression validation should be part of the merge criteria. The intent there is to confirm that existing MI300 behavior still works after this PR; it is not meant to validate the new gfx12-specific feature work itself. I already had that validation in plan and will add the results and any relevant observations to the Test Plan / Test Results once that run is completed.

2. SP / accum(level) handling
That is fair. SP / level-counter handling was intentionally deferred in this PR because it would significantly increase the implementation and validation surface.

I agree that we should not silently imply the same validation coverage for those paths as for the exercised set. I have now added a warning in the SPM path when the SP block or an SQ / SQG level counter is requested, and I will also make the current limitation explicit in the PR description.

These paths are not part of the core SPM functionality exercised by this PR, so this does not affect the main gfx12/gfx1250 SPM support being added here.

3. Mixed 16/32 coverage breadth
I agree that integration testing alone cannot prove every possible 16-bit / 32-bit programming combination.

That said, I did not intentionally narrow the integration coverage here. I exercised the mixed-width combinations I could identify, including same-block and cross-block cases, and repeated the representative reruns to make sure the behavior stayed stable. So the current integration coverage is meant to be broad and representative, not minimal.

I also agree that corner cases may still exist. Those remaining corner cases, if any, are more likely to come from real usage patterns than from a fully enumerable combination matrix in unit tests. The integration-test work itself will also come in a separate PR.

[vlaindic]

Thanks for the follow-up commit. Re-reviewed against d5959b8a41. Three items are genuinely resolved, but the correctness defects from my previous review are still open, and two items the commit message reports as addressed aren't fixed in the code.

Fixed ✅

• #if PER_WGP_INST_SELECT dead branch removed
• stable_sort applied to both ACCUM buckets (spm_builder.h:222-229)
• SetGrbmGfxIndex() helper with redundant-write elision (spm_builder.h:88-94)

Reported fixed, but not ⚠️

• Bit layout / encode-decode round-trip. Moving encode_spm_block_index / decode_spm_instance_index into gfx12_primitives.h and having both factories delegate is the right structure, but the constants underneath still disagree. encode_spm_block_index places WGP at <<1 (gfx12_primitives.h:494) while grbm_inst_se_sh_wgp_index_value places it at <<2 (:294), and decode_spm_instance_index masks SA blocks with 0x1F — which sits entirely below the sa_index << 5 field that encode wrote. Encode and decode are still not inverses; the delegation centralized the mismatch rather than resolving it. Please add a static_assert tying the shift constants to the mux_info_t field widths (counter:5, instance:6, block:5, :167-176) so a future divergence fails at compile time — note that 6 instance bits also silently truncate sa_index << 5 for sa_index >= 1.
• Decode callback documentation. aqlprofile_spm_decode_shader_engine() is well documented, thank you. But the callback it exists to decode (aql_profile_v2.h:876-880) is still undocumented, and spm_decode.cpp passes two different encodings through the same shader_engine parameter in one decode pass — bare j at :147, packed (se, sa, wgp) at :166. Document the packed layout on the typedef and make the two sites consistent, otherwise existing v1 callers silently misinterpret the field.

Still open — correctness

1. spm_counter_count has no default initializer (def/gpu_block_info.h:137). gfx10 and gfx11 block tables contain zero spm_counter_count entries, so spm_v2.cpp:302 rejects every counter on those targets. In gfx9, 9 entries are still unpopulated: gfx9_block_table.h lines 980, 989, 998, 1007, 1016, 1139, 1176, 1187, 1198. Add = 0 and log the block name when the check trips, so the failure is diagnosable.
2. End() calls rlc_spm_perfmon_cntl_value(0, 0) (spm_builder.h:749), and gfx12_primitives.h:545 computes sampling_rate - 1 — unsigned underflow to 0xFFFFFFFF in PERFMON_SAMPLE_INTERVAL on every SPM stop.
3. sa_number is still hardcoded to 2 (spm_v2.cpp:496) while CountersVec() uses GetShaderArraysNumber() (:319) and the decoder uses desc->num_sa (spm_decode.cpp:125). Three sources of truth for one value.
4. counter_map[] writes (spm_builder.h:333, 339, 358, 388, 393, 412) are still unbounded against the 4 KB SPM_DESC_SIZE. With SA/WGP expansion this is user-reachable: add a kMaxCounterMapEntries constant and fail the request when a config would exceed it.
5. Three unrelated changes remain bundled. SQ_PERFCOUNTER_MASK_ADDR → Register{} (gfx12_primitives.h:82) silently stops programming the SQ counter mask for PMC and SQTT, not just SPM — that is a behavior change to two shipping paths in an SPM-enablement PR. Please split it out with its own justification, along with the TCP CNTR_MODE 3→1 clamp and the SQ_BLOCK_ID switch to SQG.
6. SP / SQ-ACCUM / SQG-ACCUM produce warnings only, with the comment "Preserve current behavior" (spm_v2.cpp:226-240). @ApoKalipse-V asked for an error so users don't collect silently-wrong data — a warning in a stream of profiling output will be missed.
7. The three requested comments were not added: spm_decode.cpp:153 (if((sa == 0) && (wgp == 0)) continue; — why is this pair skipped?), pmc_builder.h:661 and sqtt_builder.h:227 (if(!(... == Register()))).
8. wgp_per_sa_: the if(se_number && sa_number) guard is good, but xcc_number is still used as a divisor at spm_builder.h:106 before any check.

Testing. Still one test change in the PR — a default-value swap in trace_config_test.cpp. New public API (aqlprofile_spm_decode_shader_engine), new counter-expansion math, and new bit-packing all ship untested. pm4/tests/pmc_builder_harvest_gfx12_tests.cpp already covers gfx12 GRBM indexing and is the natural place to extend — at minimum an encode→decode round-trip test would have caught item 1 above.

Debug hygiene. DEBUG_TRACE is still driven by an env var read at configure time (CMakeLists.txt:27-34), and values other than 1/2 are silently ignored. Use option(AQLPROFILE_DEBUG_TRACE ...) so it's discoverable via cmake -L and settable per build directory. <iostream>/<iomanip>/<mutex> are still unconditionally included in spm_builder.h with std::cout at :685-694.

Risk: High — new GPU code path, no test coverage, no feature flag, and unrelated behavior changes to PMC/SQTT bundled in. The PR also still carries the Not ready to Review label.

[bing-ma]

Thanks for the follow-up commit. Re-reviewed against d5959b8a41. Three items are genuinely resolved, but the correctness defects from my previous review are still open, and two items the commit message reports as addressed aren't fixed in the code.

Fixed ✅

• #if PER_WGP_INST_SELECT dead branch removed
• stable_sort applied to both ACCUM buckets (spm_builder.h:222-229)
• SetGrbmGfxIndex() helper with redundant-write elision (spm_builder.h:88-94)

Reported fixed, but not ⚠️

• Bit layout / encode-decode round-trip. Moving encode_spm_block_index / decode_spm_instance_index into gfx12_primitives.h and having both factories delegate is the right structure, but the constants underneath still disagree. encode_spm_block_index places WGP at <<1 (gfx12_primitives.h:494) while grbm_inst_se_sh_wgp_index_value places it at <<2 (:294), and decode_spm_instance_index masks SA blocks with 0x1F — which sits entirely below the sa_index << 5 field that encode wrote. Encode and decode are still not inverses; the delegation centralized the mismatch rather than resolving it. Please add a static_assert tying the shift constants to the mux_info_t field widths (counter:5, instance:6, block:5, :167-176) so a future divergence fails at compile time — note that 6 instance bits also silently truncate sa_index << 5 for sa_index >= 1.
• Decode callback documentation. aqlprofile_spm_decode_shader_engine() is well documented, thank you. But the callback it exists to decode (aql_profile_v2.h:876-880) is still undocumented, and spm_decode.cpp passes two different encodings through the same shader_engine parameter in one decode pass — bare j at :147, packed (se, sa, wgp) at :166. Document the packed layout on the typedef and make the two sites consistent, otherwise existing v1 callers silently misinterpret the field.

Still open — correctness

1. spm_counter_count has no default initializer (def/gpu_block_info.h:137). gfx10 and gfx11 block tables contain zero spm_counter_count entries, so spm_v2.cpp:302 rejects every counter on those targets. In gfx9, 9 entries are still unpopulated: gfx9_block_table.h lines 980, 989, 998, 1007, 1016, 1139, 1176, 1187, 1198. Add = 0 and log the block name when the check trips, so the failure is diagnosable.
2. End() calls rlc_spm_perfmon_cntl_value(0, 0) (spm_builder.h:749), and gfx12_primitives.h:545 computes sampling_rate - 1 — unsigned underflow to 0xFFFFFFFF in PERFMON_SAMPLE_INTERVAL on every SPM stop.
3. sa_number is still hardcoded to 2 (spm_v2.cpp:496) while CountersVec() uses GetShaderArraysNumber() (:319) and the decoder uses desc->num_sa (spm_decode.cpp:125). Three sources of truth for one value.
4. counter_map[] writes (spm_builder.h:333, 339, 358, 388, 393, 412) are still unbounded against the 4 KB SPM_DESC_SIZE. With SA/WGP expansion this is user-reachable: add a kMaxCounterMapEntries constant and fail the request when a config would exceed it.
5. Three unrelated changes remain bundled. SQ_PERFCOUNTER_MASK_ADDR → Register{} (gfx12_primitives.h:82) silently stops programming the SQ counter mask for PMC and SQTT, not just SPM — that is a behavior change to two shipping paths in an SPM-enablement PR. Please split it out with its own justification, along with the TCP CNTR_MODE 3→1 clamp and the SQ_BLOCK_ID switch to SQG.
6. SP / SQ-ACCUM / SQG-ACCUM produce warnings only, with the comment "Preserve current behavior" (spm_v2.cpp:226-240). @ApoKalipse-V asked for an error so users don't collect silently-wrong data — a warning in a stream of profiling output will be missed.
7. The three requested comments were not added: spm_decode.cpp:153 (if((sa == 0) && (wgp == 0)) continue; — why is this pair skipped?), pmc_builder.h:661 and sqtt_builder.h:227 (if(!(... == Register()))).
8. wgp_per_sa_: the if(se_number && sa_number) guard is good, but xcc_number is still used as a divisor at spm_builder.h:106 before any check.

Testing. Still one test change in the PR — a default-value swap in trace_config_test.cpp. New public API (aqlprofile_spm_decode_shader_engine), new counter-expansion math, and new bit-packing all ship untested. pm4/tests/pmc_builder_harvest_gfx12_tests.cpp already covers gfx12 GRBM indexing and is the natural place to extend — at minimum an encode→decode round-trip test would have caught item 1 above.

Debug hygiene. DEBUG_TRACE is still driven by an env var read at configure time (CMakeLists.txt:27-34), and values other than 1/2 are silently ignored. Use option(AQLPROFILE_DEBUG_TRACE ...) so it's discoverable via cmake -L and settable per build directory. <iostream>/<iomanip>/<mutex> are still unconditionally included in spm_builder.h with std::cout at :685-694.

Risk: High — new GPU code path, no test coverage, no feature flag, and unrelated behavior changes to PMC/SQTT bundled in. The PR also still carries the Not ready to Review label.

Thanks @vlaindic for the careful re-review. I went through the remaining items one by one.

1. Reported fixed, but not

1.1 Bit layout / encode-decode round-trip

The internal pair here is encode_spm_block_index() / decode_spm_instance_index(). grbm_inst_se_sh_wgp_index_value() uses a different encoding for GRBM_GFX_INDEX, so it is not expected to round-trip with the internal block_index representation.

For the current internal encoding, WGP blocks recover the physical instance with block_index & 0x1, while SA-only blocks recover it with block_index & 0x1F. The check order in decode_spm_instance_index() is intentional because WGP blocks also carry CounterBlockSaAttr, so the WGP case must be matched first.

I added a short comment above the helper pair to make that explicit.

1.2 Decode callback documentation

Agreed. I added the missing typedef-level documentation for aqlprofile_spm_decode_callback_v1_t, and I also made the base non-global path use the same packed shader_engine encoding as the expanded (se, sa, wgp) path, so the contract is now consistent and explicit for callers.

2. Still open - correctness

2.1 spm_counter_count default / missing block coverage

For this PR, gfx10/gfx11 SPM enablement is not part of the intended support scope. The shared gfx10/gfx11 changes here are only the minimum compatibility work needed so the newer builder/decode interfaces still compile cleanly.

I did update the diagnostics part, though: when spm_counter_count == 0, the SPM path now fails explicitly with the block name instead of falling through to a generic limit error.

2.2 End() underflow / forced final sample

Agreed. I fixed this in two parts: the stop-path write no longer uses rlc_spm_perfmon_cntl_value(0, 0), and the forced pre-stop sample path is now gated explicitly in the SPM config.

When that path is enabled, it now uses rlc_spm_perfmon_cntl_value(1, 0), which programs the minimum valid interval instead of underflowing the field.

2.3 sa_number hardcoded to 2

Agreed. I changed trace_config.sa_number to use pm4_factory->GetShaderArraysNumber().

2.4 counter_map[] writes unbounded

Agreed. I added an explicit counter-map capacity check after expansion.

2.5 Unrelated bundled changes

I do not think these are unrelated changes. The gfx12 SPM work touches shared primitive paths that are also used by PMC/SQTT, so some corresponding updates there are required to keep the code coherent and compiling. This is a common practice for builders when the same primitives are used across them.

Also, this does not introduce a new behavior change there. SQ_PERFCOUNTER_MASK_ADDR does not exist on some GPUs and should not be programmed on those paths. The older check used builder-side GPU-level gating; the current version moves that capability decision into the primitive, which is the preferred place for it.

For the TCP CNTR_MODE 3 -> 1 change, that is an SPM bug fix. spm_even_select_value() and spm_odd_select_value() are the 16-bit SPM helpers, and their comments already reflect that 16-bit SPM requires CNTR_MODE = 1.

For the SQ_BLOCK_ID switch, gfx12 has both SQ and SQG perfmon blocks. SQG is the one that programs similarly to gfx9's SQ, so using SQG there lets the shared SQ_BLOCK_ID handling in spm_builder stay common between gfx9 and gfx12.

2.6 SP / SQ-ACCUM / SQG-ACCUM are warnings only

This is intentional here. aqlprofile is the backend and is used by the SDK, so the normal feature gating belongs on the SDK side. The warning in aqlprofile is mainly a debug aid in case that gating is missing.

Changing the SDK-facing gating behavior is outside the scope of this PR.

2.7 Requested comments were not added

Partially agreed. I added the missing spm_decode.cpp:153 comment for the (sa == 0 && wgp == 0) skip case.

For the pmc_builder.h / sqtt_builder.h checks, Register() here is the primitive-side sentinel for “this register does not exist on this GPU/path”, so I still think the condition is straightforward enough as written and does not need an extra comment there.

2.8 wgp_per_sa_ / xcc_number divisor concern

Agreed. agent_info->xcc_num should not be 0 in the normal path, but I still added the extra guard there by normalizing it to 1 in that case, so the divisor is always valid.

3. Testing

Testing here is split by layer.

1. aqlprofile unit tests for the newly added API, flags, and helper behavior in this PR. I addressed this part directly by adding focused aqlprofile-core-tests coverage for aqlprofile_spm_decode_shader_engine(...), plus aqlprofile-pm4-tests coverage for the new spm_force_sample_before_stop flag and the gfx12 internal SPM block-index encode/decode helpers.
2. testv2, which is the aqlprofile integration path. That coverage is being prepared separately because it has different code dependencies and will come in a separate PR.
3. rocprofiler-sdk-side testing, which is also separate work and will come through separate PRs once that path is ready.

4. Debug hygiene

4.1 DEBUG_TRACE is env-var driven

This is legacy aqlprofile behavior and was not introduced by this PR.

4.2 Other DEBUG_TRACE values are silently ignored

This is also legacy aqlprofile behavior and was not introduced by this PR.

4.3 Debug-only includes / std::cout in spm_builder.h

This part was specific to the new debug dump added in this PR. I switched that dump from std::cout to printf and removed the unneeded stream-formatting headers to keep it consistent with the older aqlprofile debug style. Any broader DEBUG_TRACE cleanup is future work and not in scope for this PR.

5. Risk

I agree there is still real risk here, especially because the SDK-side integration is still TBD and this PR is a prerequisite for that later work.

At the same time, the risk should be scoped accurately. The gfx1200/gfx1201 and MI455X validation work we planned on the aqlprofile side has been completed; the remaining testv2 item is the migration/merge of that validation path into embedded aqlprofile, not open-ended new hardware bring-up. The separate MI300 run is also still needed, but its purpose is regression safety for existing shared-path behavior rather than validation of the new gfx12-specific feature surface itself.

More broadly, SPM is still a beta feature, so the tolerance for some remaining integration risk is higher than it would be for a fully mature path.

[sauverma93]

Here is the second round of AI review-

Re-reviewed against 34b85419, limited to the earlier comments.

Addressed

• End() no longer programs rlc_spm_perfmon_cntl_value(0, 0). The gfx12 stop pulse is gated by spm_force_sample_before_stop and uses rlc_spm_perfmon_cntl_value(1, 0).
• Packed shader_engine now has a documented v1 contract, aqlprofile_spm_decode_shader_engine(), and a consistent encode_spm_shader_engine(j) for the base SE sample.
• GpuBlockInfo::spm_counter_count defaults to 0, and AllocateCounter() reports the block name on failure.
• RLC_SPM_PERFMON_SAMPLE_DELAY_MAX now runs after SetGrbmGfxIndex(broadcast) at the start of Begin().

Still open

• aqlprofile_spm_is_event_supported() still returns false outside MI200_GPU_ID–MI350_GPU_ID, so gfx12/gfx1250 fail the query while packet creation succeeds.
• resolve_spm_depth() still forces gfx12 SQG to 32-bit, but the query only accepts 32-bit for HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ.
• is_gfx9_sq / is_gfx12_sqg are still GetGpuId() <= GFX10 / > GFX10.
• AllocateCounter() still compares block_des.id to HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ for is_gfx12_sqc.
• CountersVec() still sizes WGP expansion from GetNumWGPs() while GpuSpmBuilder recomputes wgp_per_sa_. If num_wgp == 0, aqlprofile_spm_decode_stream_v1() still does sa_count * wgp_count - 1 and underflows.
• SPM_DELAY_PROGRAMMING_REQUIRED still skips gfx10/gfx11 delay programming. Worth stating in the description whether those writes were inert.
• spm_buffer_desc->num_events is still original_event_count while counter_map holds the expanded vector.
• source/lib/rocprofiler-sdk/spm/decode.cpp → decode_cb() still treats any non-negative shader_engine as a raw SE id and does not use the new helper.
• 32-bit decode still reads datain[index + 16] without a bounds check.
• decode_bufvalue() still maps all-ones to uint64_t(-1).
• Mux even_idx / odd_idx are still not checked against SPM_COUNTER_MAP_INDEX_MASK (0x0FFF).
• DEBUG_TRACE is still driven by ENV{CMAKE_DEBUG_TRACE}, and the debug sample-count allocation can still fail where release succeeds.

[bing-ma]

Here is the second round of AI review-

Re-reviewed against 34b85419, limited to the earlier comments.

Addressed

• End() no longer programs rlc_spm_perfmon_cntl_value(0, 0). The gfx12 stop pulse is gated by spm_force_sample_before_stop and uses rlc_spm_perfmon_cntl_value(1, 0).
• Packed shader_engine now has a documented v1 contract, aqlprofile_spm_decode_shader_engine(), and a consistent encode_spm_shader_engine(j) for the base SE sample.
• GpuBlockInfo::spm_counter_count defaults to 0, and AllocateCounter() reports the block name on failure.
• RLC_SPM_PERFMON_SAMPLE_DELAY_MAX now runs after SetGrbmGfxIndex(broadcast) at the start of Begin().

Still open

• aqlprofile_spm_is_event_supported() still returns false outside MI200_GPU_ID–MI350_GPU_ID, so gfx12/gfx1250 fail the query while packet creation succeeds.
• resolve_spm_depth() still forces gfx12 SQG to 32-bit, but the query only accepts 32-bit for HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ.
• is_gfx9_sq / is_gfx12_sqg are still GetGpuId() <= GFX10 / > GFX10.
• AllocateCounter() still compares block_des.id to HSA_VEN_AMD_AQLPROFILE_BLOCK_NAME_SQ for is_gfx12_sqc.
• CountersVec() still sizes WGP expansion from GetNumWGPs() while GpuSpmBuilder recomputes wgp_per_sa_. If num_wgp == 0, aqlprofile_spm_decode_stream_v1() still does sa_count * wgp_count - 1 and underflows.
• SPM_DELAY_PROGRAMMING_REQUIRED still skips gfx10/gfx11 delay programming. Worth stating in the description whether those writes were inert.
• spm_buffer_desc->num_events is still original_event_count while counter_map holds the expanded vector.
• source/lib/rocprofiler-sdk/spm/decode.cpp → decode_cb() still treats any non-negative shader_engine as a raw SE id and does not use the new helper.
• 32-bit decode still reads datain[index + 16] without a bounds check.
• decode_bufvalue() still maps all-ones to uint64_t(-1).
• Mux even_idx / odd_idx are still not checked against SPM_COUNTER_MAP_INDEX_MASK (0x0FFF).
• DEBUG_TRACE is still driven by ENV{CMAKE_DEBUG_TRACE}, and the debug sample-count allocation can still fail where release succeeds.

---

Thanks for the second-round AI review. Since it was re-reviewed against 34b85419, a few of the listed items were true at that point but have already been addressed in the later local follow-up changes.

What is already addressed in the newer code:

• aqlprofile_spm_is_event_supported() now uses generic block support via spm_counter_count > 0.
• aqlprofile_spm_is_event_supported() now uses the same shared resolve_spm_depth() policy as the runtime path, so the older query/runtime depth mismatch is removed.
• CountersVec() / GpuSpmBuilder no longer derive WGP-per-SA independently. wgp_per_sa now comes through TraceConfig.
• The invalid WGP-flagged + num_wgp == 0 case is now surfaced with a DEBUG-only warning instead of being silently hidden.
• aqlprofile_spm_query_agent_configurations() and packet creation now use split interval policy:
  • old gfx9/gfx10-style path keeps 32-aligned intervals
  • newer gfx11+/gfx12-style path uses raw 1 .. 0x10000 intervals
  • the newer path also advertises both SCLK and REFCLK
• AQLPROFILE_SPM_PARAMETER_TYPE_SAMPLE_INTERVAL no longer claims a universal multiple-of-32 rule.
• DEBUG_TRACE is no longer env-var driven. It now uses a cached CMake setting: AQLPROFILE_DEBUG_TRACE=0/1/2.

For the items that remain by design:

• The top-level GPU-family gate in aqlprofile_spm_is_event_supported() is still intentionally held back until the SDK side is ready. That is a rollout/coordination choice, not a backend limitation. Under that gate, the backend query/configuration logic is now in much better shape for the later SDK enablement step.
• The shared <= GFX10 / > GFX10 split in resolve_spm_depth() is still intentional for now. Some of that shared code is written in a future-proof way, but that does not expand the support claim of this PR because gfx11 support is still neither functional nor tested here.
• The is_gfx12_sqc comparison is intentional on gfx12. The hardware has both SQG and SQ/SQC, and we use SQ/SQC to support the HSA-defined SQ block, so the block-id / HSA-visible mapping is aligned for this path.
• The num_events vs expanded counter_map descriptor contract is still intentional. The decoder only sees the descriptor and still works from it. Better documentation is reasonable, but the descriptor/decoder interface itself is coherent.
• The index + 16 32-bit high-word read is only used on the 32-bit path, and the builder-side pairing/layout rules guarantee that the reserved odd-lane slot exists for every 32-bit entry that reaches decode. So this is not an independently reachable read unless the descriptor/counter-map state is already corrupted.
• The decode_bufvalue() all-ones sentinel is still an interface tradeoff: we have to choose whether all-ones means “legitimate saturated value” or “overflow sentinel”, and the current code chooses the overflow interpretation.
• The 0x0FFF counter-map index width is still considered safe under the retained RTL-based reasoning and current allocation/layout rules for gfx9 and gfx12; this is not just an unchecked mask shrink.
• For the extra debug sample-count allocation under DEBUG_TRACE: that is still a debug-only path, so it is expected that a debug build can fail for resource reasons where the normal release path still runs. That does not affect the non-debug behavior of this PR.
• For core/logger.hpp in spm_builder.h: this does not require new CMake source wiring in the current in-tree build. The needed compiled support already exists in the existing aqlprofile core build.

[ApoKalipse-V]

LGTM, just one simplification NIT

[systems-assistant]

TheRock Submodule Bump Activity

Newest first

• 2026-09-25 — Included in TheRock via ROCm/TheRock#8508.