Added run_HeCBench.sh to aomp/bin. - #2396
Conversation
mhalk
left a comment
mhalk
left a comment
There was a problem hiding this comment.
Generally: make sure this script is shellcheck-compliant.
IMHO the VERBOSE-handling makes the script much harder to read and constantly "breaks the flow".
| _AOMP_USER_SET=0 | ||
| _ROCM_USER_SET=0 | ||
| [ -n "${AOMP+x}" ] && _AOMP_USER_SET=1 | ||
| [ -n "${ROCM+x}" ] && _ROCM_USER_SET=1 |
There was a problem hiding this comment.
Sorry my fault, $ROCM_PATH is the canonical EnVar AFAICT, see:
https://rocm.docs.amd.com/en/latest/reference/environment-variables/index.html#hipcc-environment-variables
| # shellcheck source=aomp_common_vars | ||
| _AOMP_USER_SET=0 | ||
| _ROCM_USER_SET=0 | ||
| [ -n "${AOMP+x}" ] && _AOMP_USER_SET=1 | ||
| [ -n "${ROCM+x}" ] && _ROCM_USER_SET=1 | ||
| # shellcheck disable=SC1091 | ||
| . "$thisdir/aomp_common_vars" | ||
| # --- end standard header ---- | ||
|
|
||
| # Setup AOMP / ROCM (see header for rules) | ||
| if [ "$_AOMP_USER_SET" -eq 1 ]; then | ||
| if [ "$_ROCM_USER_SET" -eq 0 ]; then | ||
| ROCM=$(realpath -m "$(realpath -m "$AOMP")"/../..) | ||
| fi | ||
| else | ||
| export AOMP=/opt/rocm/lib/llvm | ||
| if [ "$_ROCM_USER_SET" -eq 0 ]; then | ||
| export ROCM=/opt/rocm | ||
| fi | ||
| fi |
There was a problem hiding this comment.
This seems more code and more complicated than it need to be?
There was a problem hiding this comment.
Setting the default for ROCM_PATH and then exporting it is a bit more complicated than setting the default for AOMP and then exporting it. Hence added lines.
| @@ -49,31 +48,24 @@ thisdir=$(dirname "$realpath") | |||
| export AOMP_USE_CCACHE=0 | |||
There was a problem hiding this comment.
How is this used in this script?
| export ROCM=/opt/rocm | ||
| fi | ||
| # Setup AOMP / ROCM_PATH (see header for rules) | ||
| [ -z "$_had_aomp" ] && export AOMP=/opt/rocm/lib/llvm |
There was a problem hiding this comment.
Could this simply be : "${AOMP:=/opt/rocm/lib/llvm}" or AOMP="${AOMP:-/opt/rocm/lib/llvm}" and then also remove line 51?
Same thing for ROCM_PATH.
I think this should prefer to error out if neither of those two envars is set instead of silently running with something it finds in the environment.
There was a problem hiding this comment.
The thought was to use the server's compiler if AOMP was not specified. If AOMP is set, we assume that is the path to "bin" and "lib" where the compiler to be tested resides.
There was a problem hiding this comment.
I think for our use case picking up "something" from the environment is undesirable.
So, IMHO if AOMP is not set, it should default to /opt/rocm/llvm or /COD/LATEST/aomp/llvm or something. But it should not pick up something that happens to be in the environment.
I think the same is true for ROCM_PATH.
@mhalk what are your thoughts here?
| fi | ||
| # Setup AOMP / ROCM_PATH (see header for rules) | ||
| [ -z "$_had_aomp" ] && export AOMP=/opt/rocm/lib/llvm | ||
| if [ -z "$_had_rocm_path" ]; then |
There was a problem hiding this comment.
Why is here an if and in the line above is none?
There was a problem hiding this comment.
The "[ -z "$_had_aomp" ] && export AOMP=/opt/rocm/lib/llvm" is a single line. The "if [ -z "$_had_rocm_path" ]; then" allows for multiple lines (which there are).
There was a problem hiding this comment.
I personally find this quite confusing TBH
| [ -z "$_had_aomp" ] && export AOMP=/opt/rocm/lib/llvm | ||
| if [ -z "$_had_rocm_path" ]; then | ||
| ROCM_PATH=$(realpath -m "$(realpath -m "$AOMP")"/../..) | ||
| export ROCM_PATH |
There was a problem hiding this comment.
Why is it exported here and again in line 64?
There was a problem hiding this comment.
The second export of ROCM_PATH (and AOMP) was deleted.
3 participants
Motivation
Allow developers to easily run HeCBench (a suite of benchmarks) with hip and aomp. They can run individual
benchmarks with run_HeCBench.sh.
Technical Details
Added run_HeCBench.sh