[ROCm] upstream using rocprofiler-sdk (v3) for tracing AMD GPU events #29769
Conversation
|
Hi @xla-rotation this PR is important to us, and it's amd-only code, could you review it please? thanks! |
cj401-amd
force-pushed
the
ci_cj-rocprof-v3-roctracer-v1
branch
from
90fd457 to
c6080d1
Compare
cj401-amd
changed the title
cj401-amd
force-pushed
the
ci_cj-rocprof-v3-roctracer-v1
branch
from
c6080d1 to
8c23ab9
Compare
There was a problem hiding this comment.
Please #include in each source file only the files that are used in this file and all the files that are used in this file. E.g. rocm_tracer_utils.cc file at the moment includes only the corresponding header, but instead should include everything it uses directly. Similarly, rocm_tracer_utils.h should not include any files that it does not use directly. Please also do this for all the other files.
Another comment:
Do not use the <chrono> library. Use substitutes from absl instead.
cj401-amd
force-pushed
the
ci_cj-rocprof-v3-roctracer-v1
branch
from
63542a8 to
88fc01a
Compare
Hey @dimitar-asenov, please help have a look as we have addressed your comments now. btw, I wonder if it is possible to let me know what "Google internal checks FAILED" is? |
| #include <vector> | ||
| #include <time.h> | ||
| #include <unistd.h> | ||
| #include <chrono> |
There was a problem hiding this comment.
Please do not use <chrono>. Use alternatives from absl instead.
| #include <time.h> | ||
| #include <unistd.h> | ||
| #include <chrono> | ||
| #include <unistd.h> // For standard sysconf |
There was a problem hiding this comment.
This is already included a few lines above.
| #include "xla/backends/profiler/gpu/rocm_tracer_utils.h" | ||
|
|
||
| #include <cstdint> | ||
| #include <cstring> |
There was a problem hiding this comment.
Please keep the entries within the individual include blocks sorted alphabetically.
| case ROCPROFILER_HIP_RUNTIME_API_ID_hipMemcpyWithStream: | ||
| return true; | ||
| break; | ||
| default:; |
There was a problem hiding this comment.
Please use {} to more explicitly denote an empty statement, as opposed to just ;.
| OO(Memset) | ||
| OO(Synchronization) | ||
| OO(Generic) | ||
| default:; |
There was a problem hiding this comment.
Please use {} to more explicitly denote an empty statement, as opposed to just ;.
Thank you for make the changes. I've flagged additional changes from our internal checks. Sorry for the back and forth, I hope this is the last round. |
@xla-rotation, would you please kindly help review this PR?
We are phasing out development and support for roctracer/rocprofiler/rocprof/rocprofv2 in favor of rocprofiler-sdk (v3) in upcoming ROCm releases. rocprofielr-sdk (v3) also moves away from cupti.
This PR integrates rocprofiler-sdk (v3) into XLA for profiling GPU events on AMD GPUs.