Page MenuHomePhabricator

[MLIR][[amdgpu-arch]][OpenMP] Remove direct dependency on /opt/rocm
Needs RevisionPublic

Authored by dbhaskaran on Sep 16 2021, 7:29 AM.

Details

Summary

This avoids undefined behavior/issues resulting out of unconventional
or co-existing ROCm installations.

Diff Detail

Event Timeline

dbhaskaran created this revision.Sep 16 2021, 7:29 AM
dbhaskaran requested review of this revision.Sep 16 2021, 7:29 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript

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?

ye-luo requested changes to this revision.Sep 16 2021, 11:04 AM
ye-luo added a subscriber: ye-luo.

The fallback opt/rocm is desired. If a module system needs to point to the specific rocm installation. Set CMAKE_PREFIX_PATH=<unconventional rocm> in the module file.
If you would like to honor ROCM_PATH, then do one find_library with explicit ROCM_PATH and another with all the defaults.

mlir/lib/Dialect/GPU/CMakeLists.txt
137

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)

This revision now requires changes to proceed.Sep 16 2021, 11:04 AM