rocm_smi: Update read me to note two cases of root path #325
Conversation
djwoun
force-pushed
the
rocm_smi_readme
branch
from
5b7be41 to
1f3212a
Compare
|
|
||
| The system will search the directories listed in **LD\_LIBRARY\_PATH**. You can add an additional path with a colon e.g. | ||
| The system will search the directories listed in **LD_LIBRARY_PATH**. You can add an additional path with a colon, e.g.: | ||
|
|
||
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/WhereALibraryCanBeFound |
There was a problem hiding this comment.
Can you change this to be export LD_LIBRARY_PATH=/WhereALibraryCanBeFound:$LD_LIBRARY_PATH?
| * [Known Limitations](#known-limitations) | ||
| * [FAQ](#faq) | ||
| *** | ||
| - [Enabling the ROCM_SMI Component](#enabling-the-rocm_smi-component) |
There was a problem hiding this comment.
Why switch from * to -?
| - [Known Limitations](#known-limitations) | ||
| - [FAQ](#faq) | ||
|
|
||
| --- |
There was a problem hiding this comment.
Same as above, why switch from *** to ---?
| @@ -21,40 +23,49 @@ Typically, the utility `papi_components_avail` (available in `papi/src/utils/pap | |||
| ## Environment Variables | |||
|
|
|||
| For ROCM_SMI, PAPI requires one environment variable: `PAPI_ROCMSMI_ROOT`. Note | |||
| in most installations, this is a subdirectory under the ROCM directory. This is | |||
| required at both compile and run time. | |||
| that in most installations this is a subdirectory under the ROCM directory. This | |||
There was a problem hiding this comment.
I would restructure this sentence. To me it doesn't seem clear what subdirectory is actually needed. Rather it sort of makes it seem that PAPI_ROCMSMI_ROOT is that subdirectory.
One idea could be:
For ROCM_SMI, PAPI requires the environment variable PAPI_ROCMSMI_ROOT to be set such that the shared objectlibrocm_smi64.so and the directory rocm_smi are found. This variable is required at both compile and run time.
....
If you have a clearer/more concise way go with that as this is mainly an example/idea.
|
|
||
| There are two common cases for setting this variable: | ||
|
|
||
| 1. **Case 1:** |
There was a problem hiding this comment.
We could possibly be more explicit when Case 1 and Case 2 are needed.
Based off their documentation, in ROCm 5.2 they restructured their filesystem to be inline with the Linux Filesystem Hierarchy Standard (https://rocm.docs.amd.com/en/latest/conceptual/file-reorg.html). Eventually in 6.0, they fully adopt the Linux Filesystem Hierarchy Standard and removed backwards compatibility.
Based off that, Case 1 should cover ROCm < 5.2 and Case 2 should cover ROCm >= 5.2.
Treece-Burgess
dismissed
their stale review
Approved, when I should have requested changes.
Pull Request Description
Updated read me to note two cases of root path
Author Checklist
Why this PR exists. Reference all relevant information, including background, issues, test failures, etc
Commits are self contained and only do one thing
Commits have a header of the form:
module: short descriptionCommits have a body (whenever relevant) containing a detailed description of the addressed problem and its solution
The PR needs to pass all the tests