-
Notifications
You must be signed in to change notification settings - Fork 81
[rocprofiler-systems] Fix roctx wall clock tree, change timemory push/pop to use proper category, and add roctx as valid domain choice #2062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
c8b9e47 to
8c9545c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the wall clock tree generation for ROCTX markers by addressing several interconnected issues: incorrect callback flow control, improper category usage in timemory tracing, and a missing domain configuration that caused segfaults.
Key Changes:
- Fixed wall clock tree accuracy by changing
breaktoreturnin the ROCTX callback handler to prevent incorrect timemory state tracking - Corrected timemory push/pop calls to use proper category types (
CategoryTandcategory::rocm_ompt_api) instead of hardcodedcategory::rocm_marker_api - Added
roctxas a valid domain choice to prevent segfaults whenROCPROFSYS_ROCM_DOMAINS=roctxis set
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp | Fixed callback control flow and category usage in tracing callbacks |
| projects/rocprofiler-systems/source/lib/core/rocprofiler-sdk.cpp | Added roctx as valid domain configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also feel it was a bug, so changes look good to me.
…ctive callback category
… roctx as a valid domain choice
ffc2bd5 to
dc0b8ef
Compare
Motivation
Closes 569092
When doing this ticket, I also noticed the program would SEGFAULT when
ROCPROFSYS_ROCM_DOMAINS=roctxeven though the docs tell us we can do this. Went ahead and fixed that.Also noticed that timemory push/pop in
rocprofiler-sdk.cppwas always usingcategory::rocm_marker_apiinstead ofCategoryT. Fixed that as well.Technical Details
breaktoreturnintool_tracing_callback_start. (See comment I left)CategoryTas opposed tocategory::rocm_marker_apiroctxas a valid domain choice. This avoids the SEGFAULT.Note: This change means that we will NOT see
roctxRangePopin the wall clock anymore. However, this shouldn't have been true in the first place considering perfetto output does not show this.Test Plan
Checked against openmp-vv-host, roctx and custom marker program.
Test Result
With changes, wall_clock tree is now as it should be. HIP, HSA and OpenMP calls are also present in the wall clock.
Submission Checklist