Repository navigation
Conversation
Freeze the current schema under versions/3.0.4 so it stays loadable once the top-level files move on to 3.0.5, matching how 3.0.3 was frozen when the HIP event table landed. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…schema 3.0.5 Kernel replay executes one dispatch once per counter group and every pass reports the same dispatch_id, so dispatch_id stopped identifying a single execution. Add rocpd_kernel_dispatch.replay_pass to complete the key, left NULL for runs that did not replay. counters_collection now aggregates per event rather than per dispatch_id. Grouping on dispatch_id summed a counter that appears in several counter groups across every pass that collected it, so a sanity counter repeated in three groups read three times high. Each execution owns one event, so grouping on the event keeps the passes apart. Outside kernel replay there is exactly one event per dispatch_id, which makes this grouping identical to the 3.0.4 grouping. counters_collection and pmc_events also expose replay_pass. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…xport The export keyed dispatch events on dispatch_id alone, which kernel replay breaks in two ways. Dispatch records for passes after the first hit an early return and were dropped, taking their timestamps with them. Counter records for every pass then resolved to the first pass's event, so the passes' counters piled onto one event with nothing recording which pass produced them, and the counters_collection view summed them. Key the event map on the dispatch execution -- dispatch_id plus replay pass -- so each pass keeps its own dispatch row and its counters attach to it. The pass index is already on tool_counter_record_t; the kernel dispatch buffer records carry no pass index, so on that path the n-th record seen for a dispatch_id is taken as its n-th pass, which holds because replay runs its passes in sequence and the buffer preserves completion order. Runs without kernel replay are unchanged: nothing reports a non-zero pass, so the flat vector, the row ids and the emitted columns all stay as they were. Also restores the duplicate-dispatch warning, which sat after an unconditional early return and could never fire. It warns rather than using ROCP_CI_LOG so that a condition which has been silently tolerated does not become a CI abort. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
A replayed run now writes one counter row per pass, so a CSV without the pass index shows rows that look like duplicates of one dispatch. Emit the column when the database holds replay data and leave the existing columns untouched otherwise, so non-replay exports are unchanged. Register replay_pass in the schema feature table so databases written before 3.0.5, whose counters_collection has no such column, are not queried for it. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Drive the shipped schema SQL against SQLite with no GPU and no profiling run, and assert the two things the schema has to get right: a counter repeated across counter groups must not be summed across the passes that collected it, and a run that did not replay must report exactly what the previous schema reported. The old-schema case is built from the 3.0.4 snapshot so the multiplication the fix removes is demonstrated rather than asserted. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…3.0.5 The generated database now holds a dispatch executed over two replay passes so the counter export is exercised with the new column, and the CSV assertions follow the existing absent/present pattern: pre-3.0.5 databases must convert without a replay_pass column, the latest must carry one. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Record what the export got wrong, why the fix keys on the dispatch execution rather than the dispatch_id, why a separate replay table modelled on the SPM rocpd_sample table was rejected, and what the change leaves open. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
The example showed --kernel-replay-beta-enabled alone, which acknowledges the beta without selecting replay and so would have collected the counter groups by application replay. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
🟡 Changes recommended
The updated rocpd export can crash or mis-handle replayed counter records if the (dispatch_id, replay_pass) mapping is missing, and the CSV replay detection currently queries an aggregated view in a way that can be unnecessarily expensive.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the RocPD export and schema to correctly represent kernel replay runs where multiple executions (“passes”) share the same dispatch_id, preventing dispatch-row collapsing and incorrect counter aggregation across passes.
Changes:
- Bumps rocpd schema to 3.0.5 by adding
rocpd_kernel_dispatch.replay_passand changingcounters_collectionaggregation to group by event id (per-execution) rather thandispatch_id. - Updates C++ rocpd export to key dispatch→event mapping by
(dispatch_id, replay_pass)and emit per-pass kernel dispatch rows (with surrogate dispatch-row IDs under replay). - Adds schema-level SQLite tests plus extends rocpd back-compat coverage/docs for the replay-pass behavior.
File summaries
| File | Description |
|---|---|
| projects/rocprofiler-sdk/tests/rocpd-kernel-replay-schema/validate.py | New SQLite-driven tests validating replay-pass separation + backward compatibility vs 3.0.4. |
| projects/rocprofiler-sdk/tests/rocpd-kernel-replay-schema/pytest.ini | Pytest configuration for the new schema-only test suite. |
| projects/rocprofiler-sdk/tests/rocpd-kernel-replay-schema/CMakeLists.txt | CMake wiring for the new rocpd kernel replay schema integration test. |
| projects/rocprofiler-sdk/tests/rocpd-back-compat/validate.py | Adds CSV expectations for schema 3.0.5 replay_pass presence/absence. |
| projects/rocprofiler-sdk/tests/rocpd-back-compat/make_db.py | Extends synthetic DB generator to model replay-pass data for schema ≥ 3.0.5. |
| projects/rocprofiler-sdk/tests/rocpd-back-compat/CMakeLists.txt | Adds 3.0.5 to the set of schemas covered by back-compat tests. |
| projects/rocprofiler-sdk/tests/CMakeLists.txt | Registers the new rocpd-kernel-replay-schema test directory. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/CMakeLists.txt | Adds the 3.0.4 snapshot directory to the rocpd schema versions build. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/CMakeLists.txt | New build registration for the 3.0.4 schema snapshot files. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/rocpd_tables.sql | New 3.0.4 snapshot of rocpd table definitions. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/rocpd_views.sql | New 3.0.4 snapshot of base rocpd views. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/data_views.sql | New 3.0.4 snapshot including counters_collection grouping-by-dispatch_id behavior. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/summary_views.sql | New 3.0.4 snapshot of summary views. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/rocpd_metadata.sql | New 3.0.4 snapshot of metadata insertion SQL. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions/3.0.4/rocpd_indexes.sql | New 3.0.4 snapshot of index definitions. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/versions.yml | Adds 3.0.4 snapshot mappings and documents 3.0.5 replay_pass changes. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/rocpd_tables.sql | Updates live schema to 3.0.5 and adds replay_pass column to rocpd_kernel_dispatch. |
| projects/rocprofiler-sdk/source/share/rocprofiler-sdk-rocpd/data_views.sql | Exposes replay_pass and switches counters_collection grouping to E.id (per-event). |
| projects/rocprofiler-sdk/source/lib/python/rocpd/features.py | Registers replay_pass as a schema-gated feature starting at 3.0.5. |
| projects/rocprofiler-sdk/source/lib/python/rocpd/csv.py | Conditionally adds a Replay_Pass column to counter CSV export when replay data exists. |
| projects/rocprofiler-sdk/source/lib/output/generateRocpd.cpp | Updates export logic to preserve per-pass dispatch rows and map counters via (dispatch_id, replay_pass). |
| projects/rocprofiler-sdk/source/docs/conceptual/kernel_replay/kernel_replay_rocpd_schema.md | New documentation explaining the failure mode and schema/export fix. |
| projects/rocprofiler-sdk/source/docs/conceptual/kernel_replay/index.md | Links the new rocpd schema page from kernel replay docs index. |
| projects/rocprofiler-sdk/source/docs/_toc.yml.in | Adds the new rocpd schema page to the documentation TOC. |
| projects/rocprofiler-sdk/CHANGELOG.md | Documents the rocpd schema 3.0.5 replay_pass change and the kernel replay pass-collapsing fix. |
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Every pass of a replayed dispatch reports the same dispatch_id, so the pass | ||
| // index is what routes these counters to the execution that produced them. | ||
| auto evt_id = dispatch_evt_ids.get(dispatch_id, record.replay_pass); | ||
| for(const auto& count : record.read()) |
| # Kernel replay executes a dispatch once per counter group, so a replayed run has several rows | ||
| # per dispatch_id and the pass index is the only thing separating them. Runs that did not | ||
| # replay leave the column NULL and keep their existing set of CSV columns, as do databases | ||
| # written before the column existed. | ||
| has_replay_passes = ( | ||
| "replay_pass" in importData.supported_features | ||
| and importData.execute( | ||
| 'SELECT EXISTS (SELECT 1 FROM "counters_collection" WHERE replay_pass IS NOT NULL)' | ||
| ).fetchone()[0] | ||
| ) |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Co-authored-by: amd-vkale <291155109+amd-vkale@users.noreply.github.com>
Addressed in dabc89e. |
Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…NDS target rocprofiler_add_integration_execute_test rejects a COMMAND without DEPENDS with SEND_ERROR, which failed CMake configure for every job that builds the tests, so none of this PR's tests ran. Depend on rocprofiler-sdk::rocprofv3, as the rocpd-back-compat tests do. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
top_kernels, top and rocpd summary aggregate every rocpd_kernel_dispatch row, so with per-pass rows a replayed kernel's calls and total duration include every pass, matching rocprofv3's --stats. Tools that rank kernels from a replayed run (kerncap's profile step, for one) should count application dispatches; give the query, and note the uneven distortion when a kernel filter limits replay to some kernels. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
…th-cursor-fixes-help Brings the branch up to date with develop (1,887 commits). There were no conflicts. The only rocpd change on develop since the branch point is #10550's environment-variable filter in generateRocpd.cpp, which merges alongside the replay-pass changes. The rocpd schema SQL is unchanged on develop, so the frozen 3.0.4 copy still matches it. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
The CHANGELOG grew an Unreleased section after this branch was cut, so the merge left the schema 3.0.5 entry under ROCprofiler-SDK 1.4.1 and the collapsed-passes fix under that release's Known issues. Move both to Unreleased, under Changed and Resolved issues. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
|
@copilot resolve the merge conflicts on this branch. |
counters_collection now averages a counter that kernel replay collects once per group, instead of summing it and instead of emitting one row per pass. A repeated SQ_WAVES of 512 reports 512. The individual readings stay in rocpd_pmc_event. Develop's schema 3.0.5 SPM location change is kept; the replay_pass column is not. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
Identical readings of one counter, the kernel replay case, report that value once. Readings that differ are hardware instances and still sum. Averaging every repeated row made rocprofv3-test-rocpd time out: the counter CSV no longer matched the sum of the raw records. Co-authored-by: Vivek Kale <vivek.kale@amd.com>



Motivation
Kernel replay (#7960) executes one dispatch
once per counter group. Each of those executions is a real dispatch with its own timing and its
own counter readings, and each reports the same
dispatch_id— that is what makes them passes ofone dispatch rather than separate dispatches. The rocpd export predates replay and treats
dispatch_idas the identity of an execution, so it merges things that are not the sameexecution.
Two separate failures follow from that single assumption.
Dispatch rows are dropped.
process_dispatch()returned early whenever it saw adispatch_idit had already recorded, so only the first pass produced a
rocpd_kernel_dispatchrow and thetimestamps of every later pass went with it. The drop was silent: the duplicate-dispatch warning
sat after the unconditional early return and could never be reached.
Counter rows are merged and then summed. The counter path had no duplicate check at all, so
every pass's
rocpd_pmc_eventrows were written — but they all resolved to the first pass'sevent_id, because that was the only event the map held. Nothing on the row recorded which passproduced it.
counters_collectionthen aggregated withSUM(PMC_E.value)grouped bydispatch_id, summing a counter across every pass that collected it.The second failure is the more important and key one. Dropping dispatch rows loses data, which is visible to
anyone who counts them. Summing counters across passes produces a number that is wrong but looks
ordinary, and it hits hardest on the counters people are most likely to trust, because the usual
way to write counter groups repeats a sanity counter in each group:
rocprofv3 --pmc SQ_WAVES,SQ_INSTS_VALU,GRBM_COUNT \ --pmc SQ_WAVES,SQ_INSTS_VALU,GRBM_GUI_ACTIVE \ --pmc SQ_WAVES,SQ_INSTS_VALU,SQ_INSTS_SALU \ --replay-mode kernel --kernel-replay-beta-enabled -- ./appSQ_WAVESis collected three times, once per pass, each time reporting the same 512 waves.Grouped by
dispatch_idthat reads as 1536. Counters appearing in only one group were neveraffected, which is why the corruption is uneven and hard to spot in review of the output.
Technical Details
A dispatch execution is identified by
dispatch_idtogether with the replay pass that producedit. The event map is keyed on that pair, so each pass keeps its own event and its own
rocpd_kernel_dispatchrow, and the counters of a pass attach to the execution that produced themthrough the
event_idforeign key that was already present.Schema — 3.0.5.
rocpd_kernel_dispatchgains a nullablereplay_passcolumn, leftNULLforruns that did not replay, where a
dispatch_ididentifies a single execution on its own.counters_collectionchanges itsGROUP BYfromK.dispatch_idtoE.id. Outside kernel replaythere is exactly one event per
dispatch_id, so this grouping returns exactly what grouping ondispatch_idreturned — that equivalence is the whole point of the design, and it is asserteddirectly in the tests rather than argued.
counters_collectionandpmc_eventsadditionallyexpose
replay_pass.The live schema files were at 3.0.4 (HIP event tracing), so they are first snapshotted under
versions/3.0.4/— the same way 3.0.3 was frozen when the HIP event table landed — and the newtop-level version is 3.0.5. Databases written by earlier releases stay readable.
Export —
source/lib/output/generateRocpd.cpp. A smalldispatch_event_mapreplaces the flatstable_vector<uint64_t, 512>. First-pass entries stay in the vector, so the non-replay pathperforms exactly as it did; passes beyond the first go into a
std::mapkeyed on(dispatch_id, replay_pass), which keeps the rare case out of the hot structure. Because severalrows can now share a
dispatch_id,dispatch_idcan no longer double as therocpd_kernel_dispatchprimary key: replay runs allocate a surrogate row id from a counter onrocpd_db, and runs without replay keepid == dispatch_idexactly as before. Theduplicate-dispatch warning is restored to a position where it can actually fire, and deliberately
as
ROCP_WARNING_IFrather thanROCP_CI_LOG, so a condition that has been silently tolerateddoes not become a CI abort the moment it becomes observable.
The pass index on the counter path is the value the SDK computed (
tool_counter_record_talreadycarries
replay_pass). The kernel dispatch buffer records carry no pass field, so on that path then-th record seen for a
dispatch_idis taken as its n-th pass. That holds because replay executesthe passes of a dispatch in sequence and the buffer preserves completion order, but it is an
inference rather than a reported value.
rocprofiler_kernel_dispatch_info_thasreserved_padding[56]available for an explicit field; adding it is an SDK change rather than anoutput-layer one, and it is called out in the docs as the part of this change most worth replacing.
CSV —
source/lib/python/rocpd/csv.py. The counter export emits aReplay_Passcolumn whenthe database actually holds replay data, and leaves the existing column set untouched otherwise.
replay_passis registered infeatures.pywithmin_version=3.0.5, alongsidegraph_launch,spm_counters, andhip_event, so databases written before the column existed are never queriedfor it.
Docs.
source/docs/conceptual/kernel_replay/kernel_replay_rocpd_schema.mdrecords what wentwrong, the fix, and the alternatives that were considered and dropped — most notably a separate
replay table keyed on
dispatch_id, modelled on therocpd_sampletable SPM added in 3.0.3. Thatwas rejected because a replay pass carries one integer rather than a payload of its own, because
the 1:N relationship is dispatch-to-execution rather than counter-to-pass and
rocpd_kernel_dispatchis already the per-execution table, and because putting the pass onrocpd_pmc_eventwould stop the summing while leaving the dropped dispatch rows and the lostper-pass timestamps in place. The page also records what remains open: the derived pass index
above, whether the default view should return one row per pass for a replayed run (a product
question rather than a technical one), and the absence of end-to-end hardware coverage.
Issue Tracking
No GitHub issue or JIRA ID — this was found while working on range replay
(#10864) and is being fixed directly. Happy to
file one and link it if that is preferred for release tracking.
Test Plan
New:
tests/rocpd-kernel-replay-schema. Drives the shipped schema SQL against SQLitedirectly — no GPU, no profiling run, no built SDK — by loading
rocpd_tables.sql,rocpd_views.sql, anddata_views.sqlinto an in-memory database and inserting constructed rows.Seven tests:
test_kernel_dispatch_has_replay_pass_column— the pass index has somewhere to live.test_shared_counter_is_not_multiplied_across_passes—SQ_WAVES, repeated in all threecounter groups, must read 512 per pass rather than 1536 once, and the per-pass values must
still sum to what the old view reported, so the fix is shown to separate the passes rather
than discard any of them.
test_old_schema_reproduces_the_multiplication— guards the claim the fix rests on by buildingthe same data under the 3.0.4 snapshot and confirming it really does report 1536.
test_group_unique_counters_are_unaffected— counters that appear in only one group were nevermultiplied and must not change.
test_each_pass_keeps_its_own_dispatch_row— each pass is a real execution with its owntiming, so it gets its own row.
test_plain_run_matches_previous_schema— a non-replay run is compared row for row, over 16columns, against the same data built under 3.0.4. This is the backward-compatibility claim,
tested rather than asserted.
test_plain_run_leaves_replay_pass_null— the column carries no information outside replay.Extended:
tests/rocpd-back-compat. 3.0.5 is added toROCPD_COMPAT_TESTED_SCHEMAS, which theharness derives its old/latest sets from automatically.
make_db.pynow builds, for 3.0.5 andlater, a dispatch executed over two replay passes with a counter attached to each pass's own event
— these are the first
rocpd_pmc_eventrows in that harness withsample_id IS NULL, so they arethe first to reach
counters_collectionand exercise the counter CSV path at all. Two CSVassertions follow the harness's documented absent/present pattern, including the version guard that
keeps the "absent" test correct once 3.0.5 itself becomes an old schema.
Not covered: that a real replayed run on hardware populates
replay_passend to end. That needsan integration test on the rocprofv3 side and is recorded as an open item in the docs.
Test Result
All 7 schema tests pass locally against the ported schema:
test_old_schema_reproduces_the_multiplicationpassing is the notable one: the inflated countervalue this PR removes is demonstrated against the frozen 3.0.4 schema, not just asserted about.
test_plain_run_matches_previous_schemapassing means the view change is confirmed to be a no-opfor every database without replay data.
The C++ export change is left to CI to compile — the development VM has no ROCm — and is
format-clean under the project's clang-format. The rocpd back-compat and CSV paths need a built
tree and likewise run in CI.
Submission Checklist
Motivation
Kernel replay runs one dispatch once per
--pmcgroup, and every pass reports the samedispatch_id. The counter readings are stored on that dispatch.counters_collectionsummed them, so a sanity counter repeated in three groups was reported three times too high:SQ_WAVEScollected as 512, 512, and 512 came out as 1536.The default view should stay one number per dispatch. When every reading is the same, that number is the reading. Hardware instances of one counter are not the same reading, and those still sum.
Technical Details
counters_collectionkeeps one row per dispatch per counter name. The combined value is:SUM, when the grouped rows differ (hardware instances)The individual readings stay in
rocpd_pmc_event. Later passes do not get their own dispatch row or timestamps.There is no
replay_passcolumn and no per-pass CSV column. Develop's schema 3.0.5 SPM location change is unchanged.rocprofv3 --pmc SQ_WAVES,SQ_INSTS_VALU,GRBM_COUNT \ --pmc SQ_WAVES,SQ_INSTS_VALU,GRBM_GUI_ACTIVE \ --pmc SQ_WAVES,SQ_INSTS_VALU,SQ_INSTS_SALU \ --replay-mode kernel --kernel-replay-beta-enabled -- ./appIssue Tracking
Related: #15178 (kernel replay and range replay).
Test Plan
tests/rocpd-kernel-replay-schemaloads the shipped schema SQL into SQLite with no GPU:SQ_WAVESrepeated three times at 512 reads 512 once, not 1536.rocpd_pmc_event.SUMof those same rows is still 1536.tests/rocprofv3/rocpd/validate.pycompares the counter CSV to the JSON and database rows with the same rule.Test Result
No ROCm device here, so this was not run as a rocprofv3 profile. The CI failure this addresses is
tests.integration.validate.rocprofv3-test-rocpdtiming out at 120 seconds on every GPU job after the view averaged every repeated row.Submission Checklist