Skip to content

[review-only] PR3: feat(rocm): port sync-API metadata + inter-stream deps to roctracer backend - #3

Closed
ajassani wants to merge 9 commits into
pr2/interstream-deps-rebasedfrom
pr3/roctracer-interstream-deps
Closed

[review-only] PR3: feat(rocm): port sync-API metadata + inter-stream deps to roctracer backend#3
ajassani wants to merge 9 commits into
pr2/interstream-deps-rebasedfrom
pr3/roctracer-interstream-deps

Conversation

@ajassani

@ajassani ajassani commented May 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Fork-only PR for Adeem's review. Do not merge — this PR exists so we can
discuss inline before sending to pytorch/kineto:main.
Base of this PR is pr2/interstream-deps-rebased (PR2) so the diff shows
only PR3's content.

Heads-up before review

1. a0fc873 shim — already dropped from the stack

(PR3 was always clean here — the shim commit lived on PR2's branch.
2026-05-23 update: it's been removed from PR2 too. PR3 no longer carries
any descendant of it.)

2. Build-cleanup commits remaining on this PR

  • 0de775d "silence pre-existing -Wunused-parameter on roctracer
    backend" — pure build hygiene fix exposed by the rest of PR3.
  • 01a6101 "let caller override USE_ROCPROFILER_SDK on ROCm 6.4+" —
    removes an unconditional override in libkineto/CMakeLists.txt so the
    caller can actually pick the roctracer backend on newer ROCm. Real
    bug, should survive review.

3. Test infrastructure unification

e769860 drops the #ifndef ROCTRACER_FALLBACK guards from PR2's 5
inter-stream-dep tests by introducing a SyncMapLogger type alias.
This is a nice cleanup but also a precondition for the rest of PR3 being
testable on both backends. We've validated it works (L1 unit tests on
both backends pass).

Original PR body

feat(rocm): port sync-API metadata + inter-stream deps to the roctracer backend

Problem

PR2 added inter-stream-dependency metadata under the rocprofiler-sdk
backend only. Existing PyTorch ROCm wheels still ship the legacy
roctracer backend, so until the rocprofiler-sdk wheel rolls out (and on
older ROCm releases that don't have rocprofiler-sdk), users see traces
that lack the new fields. The two backends produce visibly different
JSON for the same workload.

This PR closes that gap so a Kineto trace from either backend has the
same hip_event / hip_stream / wait_on_* fields with identical
field names.

Design

Same design as PR2, applied to RoctracerLogger.cpp:

  • File-scope g_eventMap + EventMapEntry (sorted vector, mutex-guarded).
  • Static helpers RoctracerLogger::recordEvent / resolveWait /
    clearEventMap, signature-identical to RocprofLogger's.
  • api_callback cases for the 5 sync APIs:
    • HIP_API_ID_hipEventRecord → recordEvent + emit
      rocprofEventRecordRow.
    • HIP_API_ID_hipStreamWaitEvent → resolveWait against the event's
      producer + emit rocprofSyncRow with ROCPROF_SYNC_STREAM_WAIT_EVENT.
    • HIP_API_ID_hipEventSynchronize → resolveWait + emit
      rocprofSyncRow with ROCPROF_SYNC_EVENT_SYNCHRONIZE.
    • HIP_API_ID_hipStreamSynchronize → emit
      rocprofSyncRow(ROCPROF_SYNC_STREAM_SYNCHRONIZE).
    • HIP_API_ID_hipDeviceSynchronize → emit
      rocprofSyncRow(ROCPROF_SYNC_DEVICE_SYNCHRONIZE).

The row types (rocprofEventRecordRow, rocprofSyncRow) are already in
RocLogger.h from PR2; this PR reuses them so the JSON shape is byte-
identical between backends.

RoctracerActivity_inl.h adds the two RuntimeActivity<T>::metadataJson
template specializations, copied from RocprofActivity_inl.h. Same field
names, same CUPTI-parity ordering.

RocmActivityProfiler::onResetTraceData now calls
RoctracerLogger::clearEventMap() on the roctracer code path (was
guarded with #ifndef ROCTRACER_FALLBACK so it was previously a no-op).

RocmActivityProfiler::handleRoctracerActivity dispatches the two new
row types to handleRuntimeActivity<T> so the template specializations
get instantiated.

Tests / evidence

L1: Both unit-test runs pass:

  • 5/5 PASSED with KINETO_BACKEND=rocm USE_ROCPROFILER_SDK=ON (rocprofiler-sdk)
  • 5/5 PASSED with KINETO_BACKEND=rocm USE_ROCPROFILER_SDK=OFF (roctracer)

L2: PyTorch wheel rebuild against PR3, then captured single_proc_streams.py
traces under both backends. 3-way diff (stock-roctracer / PR3-roctracer /
PR3-rocprofsdk-patched) shows identical hip_event/hip_stream/
wait_on_* field presence and resolution.

L3: 4-rank DDP run on MI210 with PR3-roctracer backend; trace shows full
sync metadata, zero dangling references, matches PR2-rocprofsdk trace
field-for-field.

Tests run on both backends after this PR

The fifth commit (a7ff5c7) drops the #ifndef ROCTRACER_FALLBACK
guards from the 5 PR2 inter-stream-dep tests:

  • InterStreamDependencyTest
  • StreamWaitEventFutureCorrelation
  • EventMapClearedOnReset
  • EventSynchronizeResolvesProducer
  • UnresolvedWaitStillEmitsEventId

Mechanism:

  • Add HIP_EVENT_RECORD_ID, HIP_STREAM_WAIT_EVENT_ID,
    HIP_EVENT_SYNCHRONIZE_ID macros mirroring the existing
    HIP_LAUNCH_KERNEL pattern. Each expands to the right SDK constant
    per backend.
  • Introduce a SyncMapLogger type alias resolving to RocprofLogger
    under rocprofsdk and RoctracerLogger under roctracer. The two
    classes expose signature-identical recordEvent / resolveWait /
    clearEventMap, so one test body covers both backends.

The result: whatever backend the test binary is built against, these
5 tests run and verify the production lookup path.

Out of scope

  • A standalone "roctracer-only mock" infrastructure. The macro / alias
    approach above gets us full coverage without a second mock surface.

ajassani and others added 8 commits May 23, 2026 09:27
@ajassani @cursoragent
feat: vector-backed g_eventMap + sync-API attribution helpers in Roct…
d57a670 
…racerLogger

Adds RoctracerLogger::recordEvent / resolveWait / clearEventMap mirroring the RocprofLogger PR2 design. Backs them with a sorted-vector map keyed by hipEvent_t so handle reuse is handled correctly: a hipStreamWaitEvent at correlation N resolves to the most recent hipEventRecord whose correlation is strictly less than N.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
feat: emit hip_event/hip_stream + wait_on_* metadata on roctracer JSO…
5a0b51e 
…N output

Mirrors the rocprofiler-sdk metadataJson specializations for rocprofEventRecordRow and rocprofSyncRow. Same field names as CUPTI (wait_on_stream, wait_on_hip_event_record_corr_id, wait_on_hip_event_id) so trace consumers use one code path across all three backends.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
fix: also clearEventMap() on roctracer backend reset
9e884b4 
RocmActivityProfiler: :onResetTraceData was conditionally clearing only the rocprofiler-sdk g_eventMap. With sync-API attribution now in the roctracer path too (this PR), the same hygiene is needed there to prevent prior-session producer records from being returned as the source of a wait in the next session.
Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
build: silence pre-existing -Wunused-parameter on roctracer backend
0de775d 
PyTorch's build system propagates -Wall -Wextra -Werror to kineto. The roctracer code has 3 pre-existing unused parameters (RoctracerLogger::api_callback / ::activity_callback / RoctracerActivityApi::setMaxBufferSize) that have lived there since the file was written but never tripped a build before, because nobody had built the roctracer backend under these flags. PR3 is the first thing that exercises this combination; annotate the params with [[maybe_unused]] so the build is clean. No behaviour change.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
fix: dispatch ROCTRACER_ACTIVITY_EVENT_RECORD / _SYNC on the roctrace…
2ec953b 
…r fallback path

Without these case branches in handleRoctracerActivity, the new rocprofEventRecordRow / rocprofSyncRow records emitted by RoctracerLogger fall through to the default branch and get dropped, and the RuntimeActivity<T>::metadataJson template specializations are never instantiated. Mirror the rocprofsdk dispatch so both backends produce identical wait-event metadata in the trace JSON.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
test: enable PR2 inter-stream tests on the roctracer backend too
e769860 
Drops the #ifndef ROCTRACER_FALLBACK guards around InterStreamDependencyTest, StreamWaitEventFutureCorrelation, EventMapClearedOnReset, EventSynchronizeResolvesProducer, UnresolvedWaitStillEmitsEventId. Adds HIP_EVENT_RECORD_ID / HIP_STREAM_WAIT_EVENT_ID / HIP_EVENT_SYNCHRONIZE_ID macros mirroring the existing HIP_LAUNCH_KERNEL pattern, and a SyncMapLogger alias that resolves to RocprofLogger on rocprofsdk and RoctracerLogger on roctracer. Because PR3 makes the two backends produce identical wait-event JSON, one set of tests now covers both code paths.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
build: let caller override USE_ROCPROFILER_SDK on ROCm 6.4+
01a6101 
Previously, on ROCm >= 6.4 the rocm-backend setup unconditionally set USE_ROCPROFILER_SDK=ON, ignoring any -DUSE_ROCPROFILER_SDK=OFF the caller passed. That made it impossible to test the roctracer fallback path on modern ROCm without patching kineto. Wrap the auto-detect in 'if(NOT DEFINED USE_ROCPROFILER_SDK)' so the auto value is a default, not an override. Same conventions as KINETO_BACKEND itself.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani @cursoragent
fix: don't call RocprofLogger::ensureRegistered() on the roctracer ba…
b5e52fc 
…ckend

RocprofLogger: :ensureRegistered() is rocprofiler-sdk-specific (calls rocprofiler_force_configure). On the roctracer fallback path RocprofLogger.cpp isn't compiled, so init.cpp's reference to that symbol failed to link. Guard the call with #ifndef ROCTRACER_FALLBACK so the roctracer-backed libkineto links cleanly. Roctracer doesn't need a force-configure step.
Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani
ajassani force-pushed the pr2/interstream-deps-rebased branch from d862fbf to e097ad6 Compare May 23, 2026 13:27
@ajassani
ajassani force-pushed the pr3/roctracer-interstream-deps branch from 7994033 to b5e52fc Compare May 23, 2026 13:27
@ajassani @cursoragent
build: silence pre-existing -Wunused-private-field on RoctracerActivi…
017f8f7 
…tyApi

Upstream dead code: RoctracerActivityApi::registered_ is declared but never used. The standalone kineto build picks up -Wall -Wextra -Werror when KINETO_BUILD_TESTS=ON, and this combination has never been exercised before on the roctracer backend because the test suite was previously rocprofiler-sdk-only. PR3's test-unification (e769860) is the first thing that compiles RoctracerActivityApi.cpp under -Werror, exposing this latent issue.

Annotate the field with [[maybe_unused]]; no behaviour change. The matching field in RocprofActivityApi.h has the same shape but isn't reached by the current rocprofiler-sdk test build path, so leaving it alone for now (separate trivial fix if it ever fires).

Co-authored-by: Cursor <cursoragent@cursor.com>
@ajassani

ajassani commented May 23, 2026

Copy link
Copy Markdown
Owner Author

Superseded by a clean standalone branch: #4 (or next) — see https://github.com/ajassani/kineto/tree/pr3-standalone/roctracer-sync-deps. This PR was the old stacked PR1+PR2+PR3 version; restructured to roctracer-only as the first upstream submission.

@ajassani ajassani closed this May 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ajassani