feat(roctxconnector): link rocprofiler-sdk-roctx as of ROCm 6.2
#278
Conversation
| # instead of the "old" one provided by roctracer. | ||
| # | ||
| # See also: https://rocm.docs.amd.com/projects/rocprofiler-sdk/en/amd-mainline/how-to/using-rocprofv3.html | ||
| find_package(rocprofiler-sdk-roctx CONFIG PATHS $ENV{ROCM_PATH} PATH_SUFFIXES "lib/cmake/rocprofiler-sdk-roctx") |
There was a problem hiding this comment.
Why are we specifying PATH_SUFFIXES? I expect that path will be constructed by CMake regardless.
There was a problem hiding this comment.
Also, why do you think we should add ROCM_PATH to the search paths?
There was a problem hiding this comment.
OK. The PATH_SUFFIXES shouldn't be needed indeed. I still have to check for the ROCM_PATH.
There was a problem hiding this comment.
I've just tested the need for the ROCM_PATH. If we don't specify it as a search path, then the package isn't found.
| @@ -14,7 +14,11 @@ | |||
| // | |||
| //@HEADER | |||
|
|
|||
| #include <roctx.h> | |||
| #ifdef KP_ROCTXCONNECTOR_HAVE_ROCPROFILER_SDK_ROCTX | |||
There was a problem hiding this comment.
Why do we need to define our own macro?
Does ROCprofiler-SDK not define anything we could leverage?
Could't we just use an __has_include expression?
There was a problem hiding this comment.
OK. The CMake config provided by rocprofiler-sdk-roctx doesn't seem to set a compiler definition. I've followed your suggestion of using __has_include.
| @@ -19,10 +19,10 @@ jobs: | |||
| - image: nvidia/cuda:12.2.0-devel-ubuntu22.04 | |||
| preset: Cuda | |||
| compiler: {cpp: g++-12, c: gcc-12} | |||
| - image: rocm/dev-ubuntu-22.04:5.4 | |||
| - image: rocm/dev-ubuntu-22.04:5.7 | |||
There was a problem hiding this comment.
Why do we want to bump that version?
There was a problem hiding this comment.
Just a habit of removing the oldest version in such a case :) We can keep the 5.4 release too. Just noting that on LUMI (the European supercomputer ~ Frontier), the ROCm 5.4 stack is no longer available.
There was a problem hiding this comment.
It is available on Frontier, so we should keep it.
There was a problem hiding this comment.
OK. No problem. Let's keep 5.4.
|
@Rombur plz review |
maartenarnst
force-pushed
the
rocprofiler-sdk-roctx
branch
2 times, most recently
from
e66c4b6 to
b588f60
Compare
maartenarnst
force-pushed
the
rocprofiler-sdk-roctx
branch
from
b588f60 to
3996a46
Compare
This PR updates the
CMakelogic forroctxconnectorso as to follow AMD's recommendation to link withrocprofiler-sdk-roctxas ofROCm6.2.For reference, the recommendation is in the "note" on this webpage:
Note that with this PR, we'll use the "new"
rocprofiler-sdk-roctxinstead of the "old" one when it is available. Another possibility would be to have an option de determine which one is linked.