[Openmp-commits] [PATCH] D109885: [MLIR][[amdgpu-arch]][OpenMP] Remove direct dependency on /opt/rocm

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:13
+
+find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATH ${ROCM_PATH})
 if (NOT ${hsa-runtime64_FOUND})
----------------
JonChesterfield wrote:
> arsenm wrote:
> > I also think CMAKE_INSTALL_PREFIX in the search hints is bad practice. I don't recall ever seeing a project do this. Depending on the install path for anything leads to flaky builds
> This was copied from the amdgpu plugin. I can't remember where I copied that from. Alternatives welcome - what's the proper way to indicate 'this library? it's probably next to all the other llvm libraries'
CMAKE_INSTALL_PREFIX should be removed.
If that search location is intended, pass it via <PackageName>_ROOT, CMAKE_PREFIX_PATH or CMAKE_LIBRARY_PATH.
See searching order https://cmake.org/cmake/help/latest/command/find_library.html

The search line should be as simple as
```
find_package(hsa-runtime64 QUIET 1.2.0 PATH /opt/rocm)
```
`/opt/rocm` can be kept as the lowest priority searching guess.

There is no good reason to use `ENV{ROCM_PATH}` nor `ROCM_PATH`. I have enough pain with `XXX_ROOT`, `XXX_HOME`, `XXX_PATH` environment variables. If you need `ROCM_PATH` because it comes from modules (environment modules, lmod), ask the maintainer to add proper CMAKE_PREFIX_PATH. If hsa is not pulled in via modules, it is user responsible to tell CMake where the hsa installation is.

In addition, please ask the hsa maintainer to move hsa-runtime64 cmake files from "/opt/rocm/lib/cmake/hsa-runtime64" to the hsa library "/opt/rocm/hsa/lib/cmake/hsa-runtime64" and then softlink to /opt/rocm/lib not the other way round. In such way, the hsa library from /opt/rocm can be used as an independent library without pulling the whole /opt/rocm if needed.


================
Comment at: mlir/lib/Dialect/GPU/CMakeLists.txt:137
   set(CMAKE_MODULE_PATH "${HIP_PATH}/cmake" ${CMAKE_MODULE_PATH})
   find_package(HIP)
   if (NOT HIP_FOUND)
----------------
dbhaskaran wrote:
> ye-luo wrote:
> > Both ROCM_PATH HIP_PATH are used as hints without verification.
> > But they are used later for generating include paths. Overall logic is broken.
> > 
> > if ROCM_PATH takes the precedence over everything else
> > You can do this
> > if ROCM_PATH defined
> >     find_path(
> >       HIP_MODULE_FILE_DIR FindHIP.cmake
> >       HINTS ${ROCM_PATH}
> >       PATH_SUFFIXES hip/cmake REQUIRED
> >       NO_DEFAULT_PATH)
> > else
> >     find_path(
> >       HIP_MODULE_FILE_DIR FindHIP.cmake
> >       HINTS $ENV{ROCM_PATH} /opt/rocm
> >       PATH_SUFFIXES hip/cmake REQUIRED)
> > endif
> > 
> > by doing this, you can verify that ROCM_PATH is correct if provided and the path the hip module file has been verified. then it is safe to do
> > set(CMAKE_MODULE_PATH "${HIP_MODULE_FILE_DIR}" ${CMAKE_MODULE_PATH})
> > find_package(HIP)
> The primary intention here is to avoid fallback to a default ROCM path  to avoid unwanted issues related to multiple installation, however I agree the we should use paths with verification so I have accommodated the changes for HIP excluding the hint. Thanks for the code snippets. 
Do you know by chance that whether MLIR really need the hip runtime library or it only needs the HSA as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109885/new/

https://reviews.llvm.org/D109885