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

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.

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

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.

dbhaskaran updated this revision to Diff 385398.Sun, Nov 7, 9:22 PM

Updated with review comments.

JonChesterfield added a comment.EditedMon, Nov 8, 4:53 AM

I don't know about the MLIR parts, but replacing

find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm)

with

find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS ${ROCM_PATH})

seems an improvement, where ROCM_PATH is ideally a CMake variable spelled the same way in other components, such that someone can build an LLVM with the components all hooked into a specific ROCm location

edit:

device libs and comgr (possibly an old version) are using find_package(ROCM PATHS "/opt/rocm"), rocr looks in find_package(hsakmt 1.0 REQUIRED HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm)

Suggest the rest of the rocm stack comes up with some preferred cmake variable for 'look at this rocm' and we then use that variable here. Not at all keen on configuring the build using environment variables.

JonChesterfield requested changes to this revision.Mon, Nov 8, 5:01 AM
JonChesterfield added a reviewer: ronl.

Marking as request changes because:
if (DEFINED ENV{ROCM_PATH})
Looking at environment variables is strictly worse than looking at cmake variables. I think we should replace /opt/rocm with a cmake variable that points to somewhere to find rocr/roct, and have no strong feelings about what the cmake variable should be called

This revision now requires changes to proceed.Mon, Nov 8, 5:01 AM