Hint the location of dependencies

[krzysz00]

If a ROCm package knows its installed under PACKAGE_PREFIX_DIR, it should search under that project prefix for related dependencies.

This prevents users from having to add that prefix to their CMake module, path, which is a known antipattern.

[pfultz2]

First, thats only used for when you are using cmake < 3.0, which I dont even think works anymore. Are you using cmake 2.8?

Secondly, I am confused why this is needed. If a rocm package is found at PACKAGE_PREFIX_DIR then the dependencies can be found at PACKAGE_PREFIX_DIR as well. Furthermore, putting an implicit HINT could mess up how the user wants a dependency to be found as it will search the HINT first over user search paths.

Thirdly, setting CMAKE_PREFIX_PATH is the proper way to tell cmake where your dependencies are at when they are installed in non-standard locations, and thats what many source-based package managers use as well, no need to use custom one-off variables like ROCM_ROOT or HIP_PATH etc.

Also, users should only set CMAKE_MODULE_PATH to directories in their source tree and not to external dependencies, but probably not relevant here.

[krzysz00]

Re "why is this needed" - consider that the user is itself a library of some sort that's passing in PATHS and HINTS to find some base ROCm library - then, the path will not be in CMAKE_PREFIX_PATH or CMAKE_MODULE_PATH but will still have been used to find the top-level entry point to ROCm

Consider for example this quoted bit from the MLIR build

    # A lot of the ROCm CMake files expect to find their own dependencies in
    # CMAKE_PREFIX_PATH and don't respect PATHS or HINTS :( .
    # Therefore, temporarily add the ROCm path to CMAKE_PREFIX_PATH so we can
    # load HIP, then remove it
    set(REAL_CMAKE_PREFIX_PATH "${CMAKE_PREFIX_PATH}")
    list(APPEND CMAKE_PREFIX_PATH ${ROCM_PATH} "${ROCM_PATH}/hip")
    find_package(hip REQUIRED)
    set(CMAKE_PREFIX_PATH "${REAL_CMAKE_PREFIX_PATH}")

This shouldn't be happening

That should just be find_package(hip REQUIRED HINTS "${ROCM_PATH}" PATHS "/opt/rocm")

But it can't be that because the ROCm packages themselves don't HINTS to their own install directory.

I'll replace this PR to handle CMake > 3.0

[krzysz00]

Similarly,

/opt/rocm/lib/cmake/hip-lang/hip-lang-config.cmake
71:  list(APPEND CMAKE_PREFIX_PATH "${PACKAGE_PREFIX_DIR}" "${PACKAGE_PREFIX_DIR}/llvm")81:  # Restore the original CMAKE_PREFIX_PATH and CMAKE_SIZEOF_VOID_P
84:  list(REMOVE_AT CMAKE_PREFIX_PATH -2 -1)

Why in the name of all this is holy is that a thing - that's what HINTS is for

[krzysz00]

Ping

[pfultz2]

HINTS should not be used here at all since it changes the user search path order since HINTS are searched first.

I dont understand the issue with doing list(APPEND CMAKE_PREFIX_PATH /opt/rocm) in the build scripts. This is the way most of the libraries already do this(although its usually not necessary since CMAKE_INSTALL_PREFIX is already set to the ROCM_PATH).

I also dont understand why MLIR is doing the whole REAL_CMAKE_PREFIX_PATH either since CMAKE_PREFIX_PATH is not a global-scoped variable. It could just be simply written as list(APPEND CMAKE_PREFIX_PATH ${ROCM_PATH} "${ROCM_PATH}/hip") although it would be best to moved that to where the ROCM_PATH variable is defined.

[krzysz00]

1. If you look at https://cmake.org/cmake/help/latest/command/find_package.html, you'll notice that HINTS is processed after all the user search paths like CMAKE_PREFIX_PATH - the only thing it preempts is system paths like /usr/lib/cmake
2. If a given internal ROCm dependency isn't within the user search path - that is, if we've reached the HINTS => CMAKE_SYSTEM_PREFIX_PATH => ... stage, we know the user hasn't explicitly pointed the system at the location of the ROCm libraries, so we should handle the lookup for internal dependencies by hinting it
3. Furthermore, the ROCm libraries are a system. Absent explicit setting of PREFIX_PATH etc. something like the hipblas from x.y.z should go find the nearby hiblas-common from x.y.z, instead of maybe going to whatever's hanging around in /usr/lib.

In short, I think you're wrong about this

[pfultz2]

If you look at https://cmake.org/cmake/help/latest/command/find_package.html, you'll notice that HINTS is processed after all the user search paths like CMAKE_PREFIX_PATH - the only thing it preempts is system paths like /usr/lib/cmake

Its still searched before CMAKE_INSTALL_PREFIX and CMAKE_STAGING_PREFIX which could cause problems.

Furthermore, the ROCm libraries are a system. Absent explicit setting of PREFIX_PATH etc. something like the hipblas from x.y.z should go find the nearby hiblas-common from x.y.z

If a component is tightly coupled to the version of another component then it should use an explicit version in the find_dependency. Most components are not tightly coupled as such.

instead of maybe going to whatever's hanging around in /usr/lib.

But then it will use /usr/lib at runtime anyways since we dont update the library path.

Furthermore, not all components use rocm-cmake and some components(like third-party dependencies) would probably never support such extension, which means you will need to set the prefix path anyway. This also assumes that any dependency we use in rocm will be in the same directory which is not true either. Third-party dependencies sometimes are taken from the system.

Cmake's HINTS and PATHS only searches the immediate dependency and is not transitive. If you want to set the paths for all dependencies then you need to append the prefix path, which is the common way this is done(as you can see across rocm components). If you dont like this, then you should open an issue with cmake to enable a transitive HINTS or PATHS.

[krzysz00]

The specific claim I'm making is that every single CMake file we put in /opt/rocm that has a dependency on some other file in /opt/rocm should be HINTS-ing towards that file, so that once someone's build process finds one of the ROCm libraries - maybe they put a PATHS on their find_package(hip), all of HIP's transitive dependencies come along for the ride.

That is to say,

find_package(hip HINTS "${ROCM_PATH}" PATHS "/opt/rocm")

should not only work by default but should be what we're telling people to do

[pfultz2]

maybe they put a PATHS on their find_package(hip), all of HIP's transitive dependencies come along for the ride.

But thats not the way cmake works. You need to put it in the CMAKE_PREFIX_PATH, and that is the documented way to do this as well.

[krzysz00]

But thats not the way cmake works. You need to put it in the CMAKE_PREFIX_PATH, and that is the documented way to do this as well.

Documented by who? Where? And why have all this PATHS and HINTS stuff if you're not meant to use it?

Also, consider - instead of the CMAKE_PREFIX_PATH thing, one could just set the path to HIP directly - and that should be enough for hip to find its transitive dependencies

Like, if this were about non-ROCm components - libblas or libm or something - I'd agree that the user has to tell us where to find them. However, these are internal dependencies of ROCm components that are packages as a group - it'd be much less error-prone for find_package(hip) to pull in all the libraries that sit next to whereever hip is, for example.

[pfultz2]

Documented by who? Where?

Its documented here: https://rocm.docs.amd.com/en/latest/conceptual/cmake-packages.html

And its based on how cmake documents how to find dependencies here: https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html#config-file-packages

Which both document using CMAKE_PREFIX_PATH to find dependencies, especially for packages under /opt directories:

Packages will not be found automatically without help if they are in locations not known to CMake, such as /opt/mylib or $HOME/dev/prefix. This is a normal situation, and CMake provides several ways for users to specify where to find such libraries.
The CMAKE_PREFIX_PATH variable provides convenience in cases where multiple prefixes need to be specified, or when multiple packages are available under the same prefix.

[krzysz00]

1. The thing that the ROCm docs tells you to do should not be required - I, as a person writing projects that depend on ROCm components, should be able to PATHS or HINTS my way into finding the top level of my dependency tree.
2. /opt/rocm is a well-knows path for ROCm packages, such that people shipping projects that link against ROCm projects can safely just PATHS it to save users from having to manually -DCMAKE_PREFIX_PATH`
3. The recommendation to set up a prefix path like that applies to user code - these are, I emphasize again, internal inter-dependencies that are shipped as a group, which should know how to find each other

To pull in previous sources motivating this change, https://reviews.llvm.org/D134753 , https://reviews.llvm.org/D142391 , @arsenm and @joker-eph

Also, 4, I've seen people's builds break from the lack of this because something like vLLM will find the top-level package - possibly through something like FindHIP, but then that package won't be set up to find its dependencies.

Overall, I don't see the problem here

[pfultz2]

The thing that the ROCm docs tells you to do should not be required - I, as a person writing projects that depend on ROCm components, should be able to PATHS or HINTS my way into finding the top level of my dependency tree.

It is required because PATHS or HINTS is not transitive. I dont really like the idea of hacking something to try to emulate different behavior as it can be fragile. Even this change doesnt remove the need for CMAKE_PREFIX_PATH, and it assumes that the installed path was the path passed to PATHS which is not the case. This can be confusing for users. If you want it to work differently then make a change in upstream cmake, which would help ensure consistent behavior for users.

/opt/rocm is a well-knows path for ROCm packages, such that people shipping projects that link against ROCm projects can safely just PATHS it to save users from having to manually -DCMAKE_PREFIX_PATH`

But you can just write list(APPEND CMAKE_PREFIX_PATH /opt/rocm) at the top of your build script like every other component does. Its not that much more to write than PATHS /opt/rocm, and its actually less to write when you have to handle more than 2 rocm components.

I emphasize again, internal inter-dependencies that are shipped as a group,

They are not always installed in the same prefix especially for source-based package managers.

which should know how to find each other

Components are not always installed relative to each other and they need to be "relocatable", so we cannot use absolute paths either. The only way for a component to find another component is for the user to provide its location through the various cmake variables including CMAKE_PREFIX_PATH.

Since /opt/rocm is a common location but not a standard location we can do list(APPEND CMAKE_PREFIX_PATH /opt/rocm) so users dont always have to set the prefix path when its in a common location.

[arsenm]

1. The thing that the ROCm docs tells you to do should not be required - I, as a person writing projects that depend on ROCm components, should be able to PATHS or HINTS my way into finding the top level of my dependency tree.

+1. This is certainly not the norm among cmake builds. If I try to build a single rocm project repository against a rocm installed in a default location, I should have to set 0 flags to get the paths correct. If I do, the build is just bad and user hostile

[krzysz00]

This change makes the rather basic assumption that if you find component A at [ROOT_PATH]/lib/cmake/A/, you'll find its dependency somewhere under [ROOT_PATH], probably [ROOT_PATH]/lib/cmake/B/ for internal ROCm-to-ROCm dependencies

It's very very weird that we're not checking "nearby" inside whatever prefix we got installed to before moving on to system paths like /usr/lib

This seems like an extremely reasonable thing for our build to be doing and it saves users from having to set up flags once they've discovered ROCM_PATH by any means they have available to them

[pfultz2]

This is certainly not the norm among cmake builds.

The norm is to set CMAKE_PREFIX_PATH to any dependencies in non-standard locations, which /opt/rocm is a non-standard location.

If I try to build a single rocm project repository against a rocm installed in a default location, I should have to set 0 flags to get the paths correct. If I do, the build is just bad and user hostile

Components do add list(APPEND CMAKE_PREFIX_PATH /opt/rocm) to there CMakeLists.txt so users dont have to set flags if rocm is found in its common location of /opt/rocm. However they will still need to set flags to handle the default location since that changes with every version of rocm.

[pfultz2]

This change makes the rather basic assumption that if you find component A at [ROOT_PATH]/lib/cmake/A/, you'll find its dependency somewhere under [ROOT_PATH], probably [ROOT_PATH]/lib/cmake/B/ for internal ROCm-to-ROCm dependencies

And that assumption is not true for dependencies like find_dependency(Threads) and find_dependency(TBB). And its also not true for source-based package managers.

[krzysz00]

And that assumption is not true for dependencies like find_dependency(Threads) and find_dependency(TBB).

1. Do we have those?
2. If we do, HINTSing to check ${ROCM_PATH}, where they definitionally won't exist, is harmless
3. I don't see the issue with source-based package managers, especially if they'll be setting up CMAKE_PREFIX_PATH, which takes priority over HINTS.

[krzysz00]

Also, adding /opt/rocm to the prefix path internally is like a PATHS argument (not event HINTS) but worse, because it hard-codes a search path in a spot users aren't expecting it

If there's any ROCm CMake file that's manipulating the prefix path internally, I'm pretty sure that's a bug