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