[Sushma1203]

Motivation

Add gfx1250 support

Technical Details

Summary

• Extend SQTT/QTT analysis support by teaching the ATT waitcnt decoder about gfx1250-related async and tensor wait-count behavior.
• Add gfx1250 PC Sampling support in rocprofiler-sdk, including gfx1250-specific stochastic parsing, API updates for new sample fields, kernel/IOCTL enablement, and new parser/test coverage.
• Add supporting gfx1250 and infrastructure updates, including fallback code object decode, variable-width counter dimension encoding, extended HSA dispatch packet handling, and formatting/tooling improvements.

Key Changes

SQTT / ATT

• extend the ATT GFX12 waitcnt decoder to recognize:
  • async memory operations
  • tensor operations
  • s_wait_asynccnt
  • s_wait_tensorcnt
• track the new async/tensor dependency classes through waitcnt unroll logic
• add ATT waitcnt test coverage for the new async/tensor wait-count behavior

PC Sampling

• add gfx1250-specific parser support and route GFX12.5 traffic through a dedicated parser path
• add gfx1250 stochastic translation support for:
  • sampling_lock_error
  • async_cnt
  • tensor_cnt
  • xnack_cnt
  • chiplet-aware sample decoding
• update rocprofiler_pc_sampling_snapshot_v0_t and rocprofiler_pc_sampling_memory_counters_t with the new gfx1250 fields
• export the new PC Sampling fields through serialization
• enable stochastic PC Sampling for gfx1250 in IOCTL and test gating logic
• update HSA PC Sampling buffer sizing path to use the gfx target version and a larger buffer
• add dedicated gfx1250 parser unit tests and extend shared parser test scaffolding
• update PC Sampling tests, validation scripts, and sample output formatting for gfx1250 hardware fields

Other Features

• add fallback gfx1250 code object disassembly when COMGR decode fails
• replace fixed-width counter dimension encoding with variable-width allocation per dimension type
• tighten dimension selection/reduction logic to use explicit dimension masks and single-index selection
• extend HSA queue interception and packet handling to support AMD extended kernel dispatch packets
• add formatting support for newer HIP/HSA packet and enum types
• improve tool-side error handling when counter metadata for a GPU agent is missing

JIRA ID

Test Plan

Test Result

Submission Checklist

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

[ApoKalipse-V]

ATT part LGTM

[ammarwa]

Submitted the verified PR #5436 findings inline.

[randomizedcoder]

PR #5436 — [rocprofiler-sdk] Add support for gfx1250

@Sushma1203 — ran the nix-based static-analysis sweep against develop + this PR head (5185a1288c). Headline: the PR is well-scoped and reads cleanly; no critical defects detected, but I found a cluster of small things worth tightening before merge — mostly typos, a couple of comment/code mismatches, several unresolved FIXME/TODO markers, and one architecturally-significant change I want to flag for awareness.

This is a sizable PR (37 files, +1734/-200), so I focused on the new files (gfx1250.hpp, gfx1250test.cpp, gfxtest.hpp) and the substantively changed C++ files (translation.hpp, id_decode.hpp, evaluate_ast.cpp, hsa/queue.cpp, hsa/details/fmt.hpp). I did not deep-dive share/rocprofiler-sdk/config.yaml (279 added lines of counter definitions — that wants subject-matter review, not a static-analysis pass).

Heads-up on mixed-GPU systems (related PR #4936)

One thing worth flagging up-front for anyone planning to test this on a real box: as of develop, gfx1250 will not initialize correctly when an older AMD card is also present in the same system. That's a bug in rocr-runtime's doorbell-type handling (deprecated hsa_amd_aql_pm4_packet_t doorbell field) which surfaces specifically in mixed-architecture systems — exactly the kind of test environment people will use to validate gfx1250 alongside an existing gfx9/gfx10/gfx11 card.

We have a fix queued at #4936 — rocr-runtime: fix multi-GPU init failure from deprecated doorbell type. It was opened 2026-04-12 (13 days ago), already approved by @dayatsin-amd, and the only thing that held it up was a flaky CI test (which has since been resolved on develop). It is currently in BLOCKED → REVIEW_REQUIRED state pending one more approver. If anyone reviewing #5436 has merge rights, getting #4936 across the finish line will materially improve the testability of #5436 itself — otherwise reviewers on multi-GPU rigs will hit the doorbell crash before they get to exercise any gfx1250-specific code paths.

Tangentially: while I'm here, I have a handful of other small, focused PRs (mostly bug fixes and CI cleanup) that have been sitting waiting for review attention — listed at https://github.com/ROCm/rocm-systems/pulls/randomizedcoder. If any reviewer has bandwidth, even a quick triage pass would unblock a few things. None are urgent individually, but several are at the "approved or one nit away from approved" stage.

🟡 §1 — Typos in PR-introduced code

codespell --ignore-words-list=hsa,VALU plus a manual scan of new comments:

File	Line	Token	Fix
source/lib/rocprofiler-sdk/pc_sampling/parser/translation.hpp	429	instroduced	introduced
source/lib/rocprofiler-sdk/pc_sampling/parser/translation.hpp	(new line)	chpiplets	chiplets
source/lib/rocprofiler-sdk/pc_sampling/parser/translation.hpp	(new line)	took to long	took too long
source/lib/rocprofiler-sdk/pc_sampling/parser/translation.hpp	(new line)	is valid if (double space)	is valid if
source/lib/rocprofiler-sdk/pc_sampling/parser/tests/gfx1250test.cpp	203	overriden	overridden
source/lib/rocprofiler-sdk/pc_sampling/parser/tests/gfx1250test.cpp	269	hadler	handler (2nd-level trap handler)
source/lib/rocprofiler-sdk/pc_sampling/parser/tests/gfx1250test.cpp	364	becuase	because
source/lib/rocprofiler-sdk/pc_sampling/parser/tests/gfxtest.hpp	403	overriden	overridden

(I cross-checked every one against git blame so this list excludes pre-existing typos like Derrived in evaluate_ast.cpp:598 and accross in hsa/queue.cpp:448.)

🟡 §2 — Wrong comment on xnack_cnt extraction

source/lib/rocprofiler-sdk/pc_sampling/parser/translation.hpp (in the new copySample<GFX1250, ...stochastic_v0_t>):

    // counters available in perf_snapshot_data
    ret.memory_counters.async_cnt  = EXTRACT_BITS(perf_snapshot_data, 25, 20);
    ret.memory_counters.tensor_cnt = EXTRACT_BITS(perf_snapshot_data, 31, 26);
    // counters available in perf_snapshot_data    ← WRONG
    ret.memory_counters.xnack_cnt = EXTRACT_BITS(perf_snapshot_data1, 31, 26);

The second // counters available in perf_snapshot_data is misleading — xnack_cnt actually reads from perf_snapshot_data1. Either fix the comment to perf_snapshot_data1 or drop it. This is the kind of thing that will trip up the next person reading the code.

🟡 §3 — Unresolved FIXME on the new GFX1250 class

source/lib/rocprofiler-sdk/pc_sampling/parser/gfx1250.hpp:25:

// FIXME: check what can be inherited
class GFX1250 : public GFX12
{
public:
    static constexpr uint32_t gfx_ip_minor = 5;
};

The GFX1250 class is currently a one-line specialization of GFX12 with only gfx_ip_minor set. The // FIXME indicates the author isn't sure whether members of GFX12 actually apply to GFX1250. Recommend either:

1. Resolve before merge (audit GFX12's enums like inst_type_issued/reason_not_issued against gfx1250's regspec and either inherit silently or override), or
2. Convert to // TODO(SWDEV-NNNNN): audit inherited gfx12 enums against gfx1250 regspec with an actual ticket reference so it doesn't get lost.

A bare FIXME with no ticket tends to outlive the original author's context.

🟡 §4 — Nine new // TODO markers in the PR — two land on hot paths

grep -nE "^\+.*\b(TODO|FIXME)\b" against git diff $(merge-base)..pr-5436 returned 9 new TODOs/FIXMEs. Most are inside the new test (gfx1250test.cpp, gfxtest.hpp) and are reasonable, but two are in production code:

translation.hpp:429-ish:
+    // TODO: we need to handle new instruction type I think

gfx1250.hpp:25:
+// FIXME: check what can be inherited

The translation.hpp one matters because right after that comment, the code reuses translate_inst<GFX12>(EXTRACT_BITS(perf_snapshot_data, 5, 2)) — i.e. it explicitly translates gfx1250's inst_type through gfx12's table. If gfx1250 introduces a new instruction type (the comment says "I think"), the table is wrong silently. Worth confirming with HW spec before merge, otherwise the gfx1250 stochastic samples may misclassify instructions.

(Tests appear to cover inst_type translation, but the test fixture also uses gfx12 expectations — see §6.)

🟢 §5 — Architectural note: dimension bit-allocation change in id_decode.hpp

This is not a defect, but it is worth calling out explicitly so reviewers don't miss it:

source/lib/rocprofiler-sdk/counters/id_decode.hpp introduces a per-dimension DIM_BIT_LENGTHS[] table replacing the previous uniform DIM_BIT_LENGTH / ROCPROFILER_DIMENSION_LAST (= 48 / 8 = 6 bits per dimension):

XCC:           6 bits     (was 6)
AID:           6 bits     (was 6)
SHADER_ENGINE: 6 bits     (was 6)
AGENT:         6 bits     (was 6)
SHADER_ARRAY:  6 bits     (was 6)
WGP:           6 bits     (was 6)
INSTANCE:     10 bits     (was 6 — widened from 64 → 1024)

Two things to verify:

1. rocprofiler_counter_instance_id_t encoding is now different. If any consumer persists, transmits, or version-skews this value across processes/agents (tracelog format, MPI gathers, file-on-disk, etc.) you're crossing a binary-compat line. From a quick grep this looks internal and ephemeral — but worth a confirmation.

2. Any downstream code that did value & 0x3F (6-bit mask) for INSTANCE will silently truncate values 64-1023 to their low 6 bits and produce wrong counter IDs. The new get_dim_mask() helper is the right shape and the file adds:

   static_assert(DIM_BIT_OFFSETS[ROCPROFILER_DIMENSION_INSTANCE] +
                     DIM_BIT_LENGTHS[ROCPROFILER_DIMENSION_INSTANCE] <=
                 DIM_BIT_LENGTH,
                 "Dimension bit allocation exceeds 48-bit limit");

   which is exactly the right gate. Nice.

The new code also adds runtime CHECK(value <= ((1ULL << bit_length) - 1)) in set_dim_in_rec which is strictly better than the old code's silent value < (1 << 6) range. Net improvement.

🟡 §6 — tmp_disable_for_gfx12 duplicated across two test files

Both tests/rocprofv3/pc-sampling/stochastic/exec-mask-manipulation/validate.py and tests/rocprofv3/pc-sampling/stochastic/transpose-multiple-agents/validate.py define an identical tmp_disable_for_gfx12(...) helper:

def tmp_disable_for_gfx12(input_agent_info_csv: pd.DataFrame):
    """
    If any of the agents are from GFX12 family, we temporarily disable
    stochastic sampling specific checks, because they're not fully implemented.
    """
    return input_agent_info_csv["Name"].str.contains("gfx12").any()

If the implementation later diverges between the two files this becomes a silent inconsistency. Since the tmp_ prefix already signals "this should go away", consider hoisting into tests/pytest-packages/pc_sampling/__init__.py (or _helpers.py) so removing it later is a one-liner.

Also, str.contains("gfx12") will match gfx1250 too (substring match). If the goal is "skip while gfx12 stochastic is unimplemented but eventually re-enable for gfx1250 once gfx1250 stochastic IS implemented", this regex will keep the gfx1250 tests skipped indefinitely. Consider ~Name.str.match(r"gfx12\d?$") or an explicit allow-list.

🟢 §7 — Static-analysis tool sweep (negative results)

Tool	Version	Coverage	New PR-introduced findings
cppcheck	2.18.3	All changed C++ files	None beyond EXTRACT_BITS(x, N, N) noise (false positives — macro signature requires both args).
codespell	2.4.2	All changed files	5 typos — see §1.
ruff	0.15.10	All changed Python files	0 PR-introduced (7 F401 unused imports are pre-existing).
semgrep (cpp)	1.152.0 (local rules)	New + changed C++	0 PR-introduced (pre-existing c-style-pointer-cast at fmt.hpp:48 from 2024).
semgrep (python)	1.152.0 (local rules)	New + changed Python	1 — nix.analysis.rules.py-open-user-input on the new input_agent_info_csv fixture (conftest.py:110). False positive: it's a pytest fixture reading a path provided via --input-agent-info-csv CLI option, identical pattern to the existing input_csv fixture. No action.
clang-tidy	clang-tools 22	n/a	Skipped — needs full HIP/HSA compile_commands.json; without it the diagnostics are dominated by missing-include errors. Worth running in CI where the build env exists.
flawfinder	2.0.19	n/a	Skipped (no security-critical paths in the diff).

Summary

The PR is mostly clean from a static-analysis standpoint. To unblock merge from my side I'd want:

• §1 typos fixed (5-minute pass).
• §2 comment on xnack_cnt corrected (it reads from perf_snapshot_data1, not perf_snapshot_data).
• §3 // FIXME: check what can be inherited in gfx1250.hpp either resolved or converted to a tracked TODO with a ticket reference.
• §4 // TODO: we need to handle new instruction type I think in translation.hpp confirmed against the gfx1250 regspec — currently the gfx1250 stochastic path silently uses gfx12's translate_inst table.
• §6 tmp_disable_for_gfx12 deduplicated and the substring match ("gfx12" matches "gfx1250") tightened.

§5 is a heads-up rather than a request. §7 is informational.

Reproducibility (run on basic-static-analysis after git fetch upstream pull/5436/head:pr-5436 && git merge pr-5436):

nix-shell -p cppcheck --run \
  "cppcheck --enable=all --inconclusive --std=c++17 --language=c++ --force \
   --suppress=missingIncludeSystem --suppress=preprocessorErrorDirective \
   <changed C++ files>"

nix-shell -p codespell --run \
  "codespell --ignore-words-list=hsa,VALU <changed files>"

nix-shell -p ruff --run \
  "ruff check <changed Python files>"

nix-shell -p semgrep --run \
  "semgrep --config=nix/analysis/rules/semgrep-cpp.yaml --metrics=off <changed C++ files>"

nix-shell -p semgrep --run \
  "semgrep --config=nix/analysis/rules/semgrep-python.yaml --metrics=off <changed Python files>"

Thanks for the gfx1250 enablement work 🤙

[vlaindic]

PC sampling part LGTM, so approving.

[bing-ma]

Looks good to me. Please don't merge until changelog is updated.

[cfallows-amd]

Confirmed with Sushma offline that rocm-compute-reviewers is not required for this PR. Compute team will disregard this review request as there are no compute-related diffs. Thanks

[github-actions]

Code Coverage Report

Code Coverage Report

Tests Only

Samples Only

Tests + Samples