rbt2.4 calc TSC once, fixes BW calcs #60
Conversation
|
@rerrabolu @ashutom could you please review and merge this soon so it can be picked up for the upcoming 3.7 release? |
| @@ -109,9 +109,15 @@ elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "x86") | |||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -m32") | |||
| endif() | |||
|
|
|||
| find_package(hsa-runtime64 REQUIRED | |||
| PATHS /opt/rocm | |||
| if (DEFINED ROCM_HOME) | |||
There was a problem hiding this comment.
No need to add extra variables. To find hsa-runtime64-config.cmake from a path other than /opt/rocm, CMAKE_PREFIX_PATH can be used.
CMAKE_PREFIX_PATH="custom path for hsa"
This will function same was as ROCM_HOME="custom path for hsa or rocm"
There was a problem hiding this comment.
No need to add extra variables
What is the optimization of you get out of "No need to add extra variables" or what is the downside? The CMAKE_PREFIX_PATH and other cmake defined paths have side-effects and using those variables to get what user wants cmake to do is incorrect application of those variables, which leads to more headaches.
Read https://cmake.org/cmake/help/v3.0/variable/CMAKE_INSTALL_PREFIX.html
CMAKE_PREFIX_PATH is to specify destination dir. That is not the purpose of the change here.
There was a problem hiding this comment.
What you are referring here is CMAKE_INSTALL_PREFIX.
CMAKE_PREFIX_PATH gives a different purpose.
https://cmake.org/cmake/help/v3.0/variable/CMAKE_PREFIX_PATH.html
There was a problem hiding this comment.
Ok, my mistake. But please see my comments on line 170+ as to avoid "deduction logic" and providing user-defined parameter to use with runpath, etc.
My changes allow DevOps and other cmake experts to continue to use CMAKE_PREFIX_PATH to do a find as well as simple users who want to specify the lcoation to use, and use that in runpath.
| // uint64_t CoarseTimestampUs(); | ||
| // uint64_t MeasureTSCFreqHz(); |
There was a problem hiding this comment.
Good catch! How about the cleanup of comments be taken care of in the next change in the interest of time? Let's get this integrated so users can relying on RBT to generate correct results can get the fix and build and use RBT for performance tuning.
| @@ -161,6 +167,10 @@ if( DEFINED ENV{ROCM_RPATH}) | |||
| set ( CMAKE_EXE_LINKER_FLAGS "-Wl,--enable-new-dtags -Wl,--rpath,$ENV{ROCM_RPATH}" ) | |||
| endif() | |||
|
|
|||
| if (DEFINED ROCM_HOME) | |||
| set ( CMAKE_EXE_LINKER_FLAGS "-Wl,--enable-new-dtags -Wl,--rpath,${ROCM_HOME}/hsa/lib:${ROCM_HOME}/lib64:${ROCM_HOME}/lib" ) | |||
There was a problem hiding this comment.
Path for hsa library is already known to the cmake from find_package(hsa_runtime64).
${hsa-runtime64_DIR} will give the path for /lib/cmake/hsa-runtime64
So it will be better to deduce the path from there rather than adding a new variable. That will ensure RPATH is the same as the one linked against.
There was a problem hiding this comment.
So it will be better to deduce the path..
No, @paulfreddy. The changes allow the user to specify the parameter value to use instead o relying on cmake crap to find stuff, and code in deduction paths in cmakefiles. This changes take a keep it simple approach, and leaves existing users/jenkins/devops who rely on cmake gymnastics for their environment to continue with them.
I had to trace cmake logic to figure what was going wrong with runpath settings - no one should have to go through that IMHO.
So, the user specifiable define parameter, just like compiler defines makes it easier to specify the parameters needed to generate the makefiles. IOW, relying on another packages cmake files, their relative locations, etc. to build an "application" is not "better" approach as it relies on cmake too much.
What
Add ROCM_HOME parameter to specify the ROCm installed root on cmake. For example, to configure build for ROCm 3.7.0 APIs/libraries, use:
cmake -DROCM_HOME=/opt/rocm-3.7.0
The default "/opt/rocm" will be used when ROCM_HOME is not specified.
Remove giant triple-nested loops chewing up CPU cycles when running benchmark and skewing timing of interrupts/signals causing erratic results on certain platforms. The TSCFreq calculation, which uses a huge loop is calculated once per run instead of for every iteration/transaction.
Calculating TSCFreq makes the rocm-bandwidth-test 2.4.0 run much faster!
Update version to 2.4.0
Why
Multiple ROCm releases can be installed on a system. Each ROCm release under their own top level directory, such as /opt/rocm-3.3.0, /opt/rocm-3.5.0, etc. It is important to set runpath when building applications to link with the specific ROCm version it is intended to, and load those specific versions of libraries at runtime as well to avoid impact of any incompatible changes in libraries/APIs.
How
Use ROCM_HOME to specify the top level ROCm installed directory. The cmake find_package uses it to find hsa-runtime64 cmake targets.
After building the applications, run
ldd rocm-bandwidth-test
to verify that the libraries are picked from the ROCM_HOME location.
At runtime, a user shall use LD_LIBRARY_PATH to direct loader if needed.