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

[DineshKumar Bhaskaran via Phabricator via cfe-commits]

dbhaskaran added a comment.

In D109885#3004269 <https://reviews.llvm.org/D109885#3004269>, @JonChesterfield wrote:

> Dropping PATHS /opt/rocm from the openmp parts will break people using an existing rocm install with trunk llvm. I think it would be reasonable to look at whatever ROCM_PATH is set to instead of assuming /opt/rocm. Is that a variable we can pass to find_package?

Added ROCM_PATH environment variable.



================
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)
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109885