[vlaindic]

Motivation

This PR redesigns the v2 PC sampling API to be ABI backward-compatible and easily extensible as new GPU architectures introduce additional PC sampling details. The old API exposed per-architecture stochastic snapshot fields through explicit bitfields (arbiter state, dual issue, etc.), which required breaking ABI changes every time a new field appeared. The new approach replaces explicit bitfields with versioned record types (V0 through V4) of increasing detail, a packed ext_data field for snapshot information decoded through query/extract helpers, and API flags that decouple method selection from record layout. Record kinds are gated at the API level so unsupported versions are cleanly rejected per-architecture, allowing client code to iterate from the most detailed record kind downward and gracefully fall back to what the hardware supports. Together, these changes ensure that future architectures can add new fields, counters, or cluster information without breaking existing tools compiled against earlier versions of the SDK.

Technical Details

This work requires refactoring of the whole PC sampling service. To ease PR reviewing process,
we decided to only propose API updates, implementation and a sample. Unit and integration tests will be added later.

PC sampling not supported on the GFX11 yet, and the TODOs remain from previous work.

Pending work to support clusters on the ROCr side blocks clusters support in this PR, that's why the fields are zero initialized.

JIRA ID

AIPROFSDK-178

Test Plan

Will be provided by forthcoming PRs.

Test Result

Please check section above.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

[vlaindic]

1st part of the review commends impacting a sample

[github-actions]

Code Coverage Report

Code Coverage Report

Tests Only

Samples Only

Tests + Samples

[vlaindic]

PC Sampling v2 API — Code Review

8 findings: 4 IMPORTANT, 4 SUGGESTIONS. No CRITICAL.

---

⚠️ IMPORTANT

1. Verify runtime feature gating for v2 entry points

The ROCPROFILER_SDK_EXPERIMENTAL annotation on configure_pc_sampling_service_v2 and query_agent_configurations_v2 is compile-time only. Runtime gating via ROCPROFILER_PC_SAMPLING_BETA_ENABLED is what actually protects users from picking up the experimental surface by accident.

Action: Confirm the env-flag check is enforced at both v2 API entry points, or add it.

---

2. is_invalid_sample<v2> / <v4> may drop legitimately predicated waves

In parser/correlation.hpp, the V2 and V4 specializations treat timestamp == 0 && exec_mask == 0 as the invalid sentinel. A legitimately predicated wave can have exec_mask == 0, which would silently drop a valid stochastic sample.

Action: Either use a dedicated sentinel field, or document the HW guarantee that valid samples always carry a non-zero timestamp and drop the exec_mask == 0 term.

---

3. ROCP_FATAL on wave_id mismatch terminates the entire process

In the GFX1250 copySample path, a wave_id mismatch triggers ROCP_FATAL. A parser-level sanity check disagreement on a single record should not be a process-killer — one malformed record from HW would take down all consumers in the process.

Action: Replace ROCP_FATAL with ROCP_ERROR (or ROCP_WARNING) and mark the record invalid via mark_sample_invalid so the buffer pipeline continues.

---

4. _get_parse_func_for_record_kind nullptr return must be checked

_get_parse_func_for_record_kind<GFXIP> returns nullptr for V3/V4 on non-GFX1250 architectures. If the caller in parse() does not null-check before dispatching the function pointer, this is a segfault on a misconfigured client.

Action: Verify the call site in parser/pc_record_interface.cpp::parse() null-checks the returned function pointer; otherwise add the check and return an explicit error status (e.g., ROCPROFILER_STATUS_ERROR_INCOMPATIBLE_KERNEL or equivalent).

---

💡 SUGGESTIONS

5. Dead code in query_agent_configurations_v2

In pc_sampling.cpp:

• Unused variable preferred_end with (void) preferred_end;
• Empty conditional: if (!is_required) { /* For soft preferences ... but include all */ }

Action: Delete the dead variable and either implement or delete the empty conditional.

---

6. Redundant zero-init pattern in parser/translation.hpp

rocprofiler_pc_sampling_record_v2_t invalid{};
std::memset(&invalid, 0, sizeof(invalid));

The {} value-initializes the struct to zero already. The memset is redundant.

Action: Drop the memset.

---

7. Doc inconsistencies — V0-V5 / V3-V5 references in comments

Comments reference "V3-V5 reserved" and "V0-V5", but the enum defines only V0-V4.

Action: Reconcile comments to match the enum (V0-V4).

---

8. _get_parse_func_for_record_kind docstring does not document nullptr return

The docstring does not state that the function returns nullptr for V3/V4 on non-GFX1250 architectures. Callers cannot tell from the signature that null-checking is required.

Action: Add a @return nullptr if the requested record kind is not supported by the GFXIP line to the docstring.

[vlaindic]

PC Sampling v2 API — Code Review

8 findings: 4 IMPORTANT, 4 SUGGESTIONS. No CRITICAL.

⚠️ IMPORTANT

1. Verify runtime feature gating for v2 entry points

The ROCPROFILER_SDK_EXPERIMENTAL annotation on configure_pc_sampling_service_v2 and query_agent_configurations_v2 is compile-time only. Runtime gating via ROCPROFILER_PC_SAMPLING_BETA_ENABLED is what actually protects users from picking up the experimental surface by accident.

Action: Confirm the env-flag check is enforced at both v2 API entry points, or add it.

2. is_invalid_sample<v2> / <v4> may drop legitimately predicated waves

In parser/correlation.hpp, the V2 and V4 specializations treat timestamp == 0 && exec_mask == 0 as the invalid sentinel. A legitimately predicated wave can have exec_mask == 0, which would silently drop a valid stochastic sample.

Action: Either use a dedicated sentinel field, or document the HW guarantee that valid samples always carry a non-zero timestamp and drop the exec_mask == 0 term.

3. ROCP_FATAL on wave_id mismatch terminates the entire process

In the GFX1250 copySample path, a wave_id mismatch triggers ROCP_FATAL. A parser-level sanity check disagreement on a single record should not be a process-killer — one malformed record from HW would take down all consumers in the process.

Action: Replace ROCP_FATAL with ROCP_ERROR (or ROCP_WARNING) and mark the record invalid via mark_sample_invalid so the buffer pipeline continues.

4. _get_parse_func_for_record_kind nullptr return must be checked

_get_parse_func_for_record_kind<GFXIP> returns nullptr for V3/V4 on non-GFX1250 architectures. If the caller in parse() does not null-check before dispatching the function pointer, this is a segfault on a misconfigured client.

Action: Verify the call site in parser/pc_record_interface.cpp::parse() null-checks the returned function pointer; otherwise add the check and return an explicit error status (e.g., ROCPROFILER_STATUS_ERROR_INCOMPATIBLE_KERNEL or equivalent).

💡 SUGGESTIONS

5. Dead code in query_agent_configurations_v2

In pc_sampling.cpp:

• Unused variable preferred_end with (void) preferred_end;
• Empty conditional: if (!is_required) { /* For soft preferences ... but include all */ }

Action: Delete the dead variable and either implement or delete the empty conditional.

6. Redundant zero-init pattern in parser/translation.hpp

rocprofiler_pc_sampling_record_v2_t invalid{};
std::memset(&invalid, 0, sizeof(invalid));

The {} value-initializes the struct to zero already. The memset is redundant.

Action: Drop the memset.

7. Doc inconsistencies — V0-V5 / V3-V5 references in comments

Comments reference "V3-V5 reserved" and "V0-V5", but the enum defines only V0-V4.

Action: Reconcile comments to match the enum (V0-V4).

8. _get_parse_func_for_record_kind docstring does not document nullptr return

The docstring does not state that the function returns nullptr for V3/V4 on non-GFX1250 architectures. Callers cannot tell from the signature that null-checking is required.

Action: Add a @return nullptr if the requested record kind is not supported by the GFXIP line to the docstring.

Thanks for the careful review! For findings 1, 2, and 4, I believe the
protections are already in place — pointers below. Let me know if I'm
missing something.

1. Runtime env gating at v2 entry points

Both v2 API entry points call is_pc_sampling_explicitly_enabled() before
any other work and short-circuit with ROCPROFILER_STATUS_ERROR_NOT_IMPLEMENTED
when the env flag is unset:

• rocprofiler_pc_sampling_configure_service_v2 — pc_sampling.cpp:562
• rocprofiler_pc_sampling_query_agent_configurations_v2 — pc_sampling.cpp:675

The helper itself reads ROCPROFILER_PC_SAMPLING_BETA_ENABLED at
pc_sampling.cpp:312-324. So the ROCPROFILER_SDK_EXPERIMENTAL annotation
is a compile-time hint as you noted, with runtime gating layered on top.

2. is_invalid_sample<v2> / <v4> and predicated waves

Good catch on the original code — this was already fixed in commit
ff347ad9fd ("Gate V3/V4 record kinds to GFX12+ and fix invalid sample
detection"). The V2 and V4 specializations now only check
sample.timestamp == 0, dropping the exec_mask == 0 term, since
copySample produces invalid samples via zero-init and the HW timestamp
is never zero for valid samples:

• V2 — parser/correlation.hpp:270-277
• V4 — parser/correlation.hpp:288-295

4. _get_parse_func_for_record_kind nullptr call site

The call site in parser/pc_record_interface.cpp::parse() does null-check
the returned pointer before dispatch:

if(parseSample_func == nullptr)
{
    return PCSAMPLE_STATUS_INVALID_METHOD;
}
auto status = (this->*parseSample_func)(upcoming, data_);

at pc_record_interface.cpp:242-245. I'll also tighten the docstring
on _get_parse_func_for_record_kind to make the nullptr contract
explicit (per your finding 8)

[vlaindic]

Review comments for the last implementation part.

The following comment addresses the my review from here: #4548 (review)

Thanks for the thorough review. All of the implementation-level findings are now addressed across a few commits on this branch (the sample-file comments — client.cpp, main.cpp, pcs.cpp — will be handled in a separate follow-up commit). Summary below, grouped by theme.

Method is now threaded through the parser

Several findings shared one root cause: the parser hardcoded the host-trap vs stochastic assumption per record kind ("the rest of this code assumes V0, V1, and V3 are only used for host-trap", "no one prevents the user from picking V0 and stochastic", "I suppose we're missing v3 and v4?").

• copySample, is_invalid_sample, add_upcoming_samples, _parse, and correct_pc_address are now templated on the negotiated method; dispatch keys on the (record_kind, method) pair.
• Added the missing stochastic specializations for the host-trap-layout records (V0/V1/V3), so e.g. V0×STOCHASTIC is correct rather than silently assuming host-trap.
• V2/V4 stay stochastic-only; (V2, HOST_TRAP) / (V4, HOST_TRAP) are rejected upstream and can never reach the dispatch table.

Flag / record method conflicts are validated

Addresses "what would happen if there's a conflict between a flag and a record in terms of method?" and the host-trap-with-stochastic-record scenario.

• New validate_record_kinds_against_flags() rejects REQUIRE_HOST_TRAP + V2/V4 with INVALID_ARGUMENT, wired into both configure and query.
• PREFER_HOST_TRAP is a soft hint: when the requested kinds are stochastic-only it is silently honored as stochastic (mirrors PREFER_STOCHASTIC).
• Neither configure's method fallback nor query's config filter offers host-trap as a substitute for a stochastic-only record, so an unsatisfiable request surfaces honestly (configure returns the genuine NOT_AVAILABLE; query returns an empty config list).
• pc_sampling.h now documents all of the above for both entry points.

Invalid-sample handling unified + INVALID-only delivery

• The four scattered emplace_records_in_buffer specializations are collapsed into one method-aware template; invalid delivery is an explicit opt-in policy (deliver_invalid).
• Added support for requesting { INVALID_SAMPLE } alone (debugging): parse with a cheap V0 carrier, deliver only invalids, drop the valid carriers (deliver_valid).
• The parser context is stateless w.r.t. per-session v2 config — the requested record-kind set is passed per parse() call and the policy is derived there, which is also the natural extension point for future richer requests.

Specific findings

• correct_pc_address for V3/V4 / "add v3/v4 here?" — made method-aware. The GFX950 mid-macro correction reads the stochastic-only perf_snapshot_data1 register, so it must never run for host-trap samples (that offset aliases reserved bytes in the host-trap struct and would corrupt a valid PC). Only the STOCHASTIC GFX950 specializations apply it; V0/V1 stochastic now get corrected alongside V2/stochastic_v0, host-trap falls through to the no-op default.
• assert(buffer != nullptr) in alloc() — replaced with ROCP_FATAL_IF, so the null-output contract is enforced in release builds too.
• #include <optional> placement — moved into the standard-library include group.
• Inference docstring vs pc_sampling.h — realigned with the compatibility matrix (V0→prefer stochastic; V1/V3→prefer host-trap; V2/V4→stochastic; INVALID-only→no method implied, defaults to stochastic).
• Redundant memset ({} already zero-inits) — applied where flagged; the invalid-sample paths now return RecordT{}.
• Stale V0-V5 / V3-V5 reserved — corrected to V0-V4.
• nullptr return contract — now stated in the dispatch docstring.

Answers to the open questions

• "PREFER pre-scan seems redundant, we iterate configs later." It is not redundant: the pre-scan decides whether to filter at all. PREFER means "filter to the preferred method only if such a config exists; otherwise return everything." The later translation loop can't make that all-or-nothing decision mid-iteration, so the upfront scan sets filter_by_method. (REQUIRE and stochastic-only kinds skip the scan and filter unconditionally.)
• ext_data extraction — does it rely on field ordering matching between GFX9 and GFX12? Yes, by construction. ext_data is packed so that bit N corresponds to rocprofiler_pc_sampling_snapshot_ext_field_id_t value N. The per-architecture pack_snapshot_ext_data_gfx9 / gfx12 / gfx1250 helpers normalize each arch's raw HW bit layout into this canonical ID-indexed form at pack time, so extraction is a uniform (ext_data >> field_id) & 1 regardless of GFX. The arch differences are absorbed in packing, not extraction — so yes, first ID → bit 0, second → bit 1, etc.

Intentionally left as-is (suggestions)

• mark_sample_invalid keeps std::memset — it zeroes an existing object in place, which is what we want here.
• The pack_snapshot_ext_data per-bit set_bit lambda is kept for readability; it is not on a hot path, so the micro-optimization wasn't worth the clarity cost.

Verification

Clean build; the pc-sampling-v2 sample delivers samples with zero unexpected drops; all 13 parser unit tests pass (including the GFX950 mid-macro correction tests).

Note: dedicated unit tests for the new validators and end-to-end tests for the deliver/drop and INVALID-only paths will be tracked as a follow-up test PR (per the agreed plan).

[vlaindic]

First part of the review related to the sample.

Reply to the previous sample review (#4548 (review))

Thanks for the detailed pass over the sample. All comments are addressed in the latest commit
([rocprofiler-sdk] PC sampling v2: address sample review comments). Mapping below.

client.cpp

• Header reordering — done; project common/ headers now come before the SDK headers.
• Avoid assert in tool_fini — replaced with utils::pcs_assert(...) (prints + aborts,
  works under NDEBUG); also fixed the missing semicolon after the ROCPROFILER_CHECK(...) line
  and dropped the now-unused <cassert>.
• Empty unused shutdown() — removed. While there, I confirmed setup() is also dead in this
  sample (never called; the client is built as a preloaded shared library, so rocprofiler
  auto-discovers rocprofiler_configure and the force_configure path is unnecessary), so I
  removed setup() too.

main.cpp

• Remove run_migrate / run_scratch — removed both (calls, declarations, definitions) plus
  everything they exclusively referenced (test_page_migrate, test_kern_small/medium/large, and
  the now-unused <sstream> include). The transpose benchmark is retained as the workload.

pcs.cpp / pcs.hpp

• gpu_agents lifetime (raw pointer like buffer_ids) — gpu_agents is now a heap-allocated
  raw pointer created in init() and released in fini(), accessed via a new get_gpu_agents()
  accessor (mirrors get_pc_sampling_buffer_ids()), avoiding static-destruction-order issues.

• ext-fields query only makes sense for stochastic records / would fail for host-trap / printing
  should run only for stochastic-suitable records — handled at the API level rather than by
  special-casing host-trap in the sample (which shouldn't need to reason about methods). The library
  function rocprofiler_pc_sampling_query_snapshot_ext_fields now honors its documented contract:
  for record kinds that carry no snapshot information (host-trap suitable records V0/V1/V3, and
  INVALID_SAMPLE) it returns SUCCESS with an empty field list instead of INVALID_ARGUMENT.
  The sample therefore just calls it unconditionally and treats an empty result as "ext_data not
  applicable" — no host-trap/stochastic branching in the sample. The print paths already gate
  ext_data on a non-empty field list, so nothing extra prints for host-trap.

• Doc: function implicitly uses the most comprehensive record — added to the
  query_snapshot_ext_fields_for_agent docstring (operates on
  most_comprehensive_record_kind; must follow query_most_comprehensive_config_for_agent).

• Querying/configuring with INVALID_SAMPLE for host-trap; allow vs fail — we take the
  "allow, deliver none" route, and it is now safe by construction: INVALID_SAMPLE is
  method-agnostic, so it does not block the stochastic→host-trap fallback and does not fail
  host-trap configuration; host-trap sessions simply deliver no invalid records. Added clarifying
  comments at both the query and configure sites stating this.

• gfx1250 record selected on an older arch (top-down discovery) — added a comment near the
  discovery loop documenting the assumption (newer record kinds target newer architectures) and
  where to revisit if that ever changes; no behavior change, since this does not occur today. Also
  corrected a stale "V3-V5" reference to "V3/V4 on non-GFX12 agents".

• V2/V4 ext_data printing duplicated → helper — extracted the shared decode block into
  print_snapshot_ext_data(os, record_kind, sample, agent_info). While there, the duplicated
  snapshot_information block (wave issued/stalled + wave_count) is also factored into
  print_snapshot_information(os, snap). Both print_sample_v2 and print_sample_v4 now call
  these; v4 keeps its cluster and memory-counter blocks inline.

Verification

Clean build; pc-sampling-v2 runs to completion (exit 0) and the log shows correctly formatted
samples (wave issued/stalled, wave_count, full ext_data map) with zero dropped samples, confirming
the printing refactor is output-equivalent.

Tests for the sample / new paths will be tracked in the separate follow-up test PR.

[Copilot]

Pull request overview

This PR refactors the v2 PC sampling API to be ABI-stable and extensible across GPU architectures. It introduces versioned record kinds (V0–V4), a packed ext_data field with query/extract helpers, and API flags that decouple method selection from record layout. The legacy v1 entry points are preserved; v2 dispatch is added alongside via a separate parser parameter and per-(record_kind, method) template specializations.

Changes:

• New public v2 API in pc_sampling.h (*_configure_service_v2, *_query_agent_configurations_v2, *_query_snapshot_ext_fields, *_extract_snapshot_ext_field_values) plus new record/enum types.
• Parser refactor: copySample, correct_pc_address, is_invalid_sample, and _parse are now templated on (GFXIP, RecordT, Method); parse() takes a requested_record_kinds set and applies a deliver/drop policy.
• New pc_sampling_v2 sample demonstrating the top-down record-kind discovery, ext-field extraction, and buffer callback decoding.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
source_code_formatter.sh	New developer-local formatter helper script (appears unrelated).
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/pc_sampling.h	Adds v2 API surface, record kinds V0–V4, snapshot ext-field enum, query/extract entry points.
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/fwd.h	Adds V0–V4 record-kind enum values.
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/cxx/enum_string.hpp	Registers new record-kind labels and updates the LAST static_assert.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling.cpp	Implements v2 configure/query/extract entry points, flag/record validation, method inference, ext-field tables.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/service.{hpp,cpp}	Adds configure_pc_sampling_service_v2.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/types.hpp	Tracks requested_record_kinds on PCSAgentSession.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp	Forwards requested_record_kinds to parser->parse().
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/parser/pc_record_interface.{hpp,cpp}	Method-aware _parse, v2 alloc specializations, record-kind→dispatch table, deliver/drop policy.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/parser/translation.hpp	Adds (RecordT, Method)-templated copySample/correct_pc_address specializations and ext_data packing helpers per GFX.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/parser/correlation.hpp	Adds is_invalid_sample/mark_sample_invalid traits and Method-templated add_upcoming_samples.
projects/rocprofiler-sdk/samples/pc_sampling_v2/*	New v2 sample (client, pcs, utils, main, CMakeLists).
projects/rocprofiler-sdk/samples/CMakeLists.txt	Adds the new sample subdirectory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[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.

[github-actions]

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.

[therock-pr-bot]

❌ PR Check — Action Required

Check	Status	Details
📝 PR Description	❌ Fail	Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword. Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples: • JIRA ID : TESTAUTO-6039 • JIRA ID - #330 • JIRA ID #330 • JIRA ID (on separate line) ROCM-25757 • ISSUE ID : TESTUTO-3334 • ISSUE ID #3334 • ISSUE ID - TESTAUTO-3433 • ISSUE ID (on separate line) AIRUNTIME-2352 • ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234 • Closes #10 • Fixes octo-org/octo-repo#100 • Resolves: #123 • #123 • https://github.com/<org_name>/<repo_name>/issues/123 Current: no valid JIRA/ISSUE/closing-keyword reference found
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*). Current: code file(s) changed: projects/rocprofiler-sdk/samples/pc_sampling_v2/client.cpp, projects/rocprofiler-sdk/samples/pc_sampling_v2/main.cpp, projects/rocprofiler-sdk/samples/pc_sampling_v2/pcs.cpp, projects/rocprofiler-sdk/samples/pc_sampling_v2/pcs.hpp, projects/rocprofiler-sdk/samples/pc_sampling_v2/utils.cpp (+13 more); no test file found
🔎 pre-commit	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

• ❌ PR Description

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 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.

[github-actions]

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.