Add rocprofiler-sdk support #1050
Conversation
|
Could someone remind me of the preferred method to run clang-tidy (or equivalent) on kineto code? |
Our internal CI runs it automatically, I can push whatever changes needed to the PR |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Full service, lovely. The roctracer code has evolved quite a bit since my initial commit. I tried to swap this in with as few changes as possible. The specific implementation in RocprofLogger.cpp is very new and very different. There are a few comments in there, but overall rocprofiler-sdk is not super intuitive. Let me know what would help you or what you would like to see improved. |
|
@mwootton has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mwootton has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Quick update: we are still working on this to get our internal builds working. planning on having this in within the next couple weeks |
|
@mwootton has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mwootton has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mwootton Tried running a profile with this and got a segfault on this line: https://github.com/mwootton/kineto/blob/rocprof/libkineto/src/RocprofLogger.cpp#L731 It looks like I think it is because we need to call this function first but we never do: Do you mind fixing this up? |
Indeed that function is supposed to be called when hip loads. So 'automatic' to us, based on that symbol name. I'll investigate a bit. It works for me as is. However, it was unclear to me if I should also link 'librocprofiler-register.so'. Either way, this is a failure path that will happen on earlier versions of rocm that don't have rocprofiler-sdk support. So I'll guard against it. Do you happen to know what rocm version you were using when you saw the failure? |
|
@mwootton The ROCM version is 6.2.0, but I forgot to mention this is failing specifically in our internal test(s). In these tests, we are only linking libamdhip64.so and librocprofiler-sdk.so and the test builds but then comes across the segfault. Do we need to add librocprofiler-register.so to the build of our tests? Will it automatically run that function if we link this? |
|
@mwootton has updated the pull request. You must reimport the pull request before landing. |
|
@mwootton has updated the pull request. You must reimport the pull request before landing. |
#1049
Convert to using rocprofiler-sdk instead of roctracer for collecting hip api calls and AMD gpu activity.
Reuses most existing roctracer infrastructure with a name for name replacement. Simultaneous support for both roctracer and rocprofiler-sdk was deemed impractical. This would require a whole new set of #ifdefs, a major refactor of the roctracer code, and additional build support. Even then, only one could be active at a time (and you wouldn't want both active).
In homage to the abandoned refactor, RocLogger.cpp/h were created to contain the rocprofbase classes and the api filter.
Roctracer has no established end date.
Rocprofiler-sdk is in rocm_3.1 forward.
This will create a dependency where (newest kineto on old rocm) and (old kineto on newest rocm) could fail to build with AMD gpu support. That window is already over 1 year wide.