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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
