HIP: document variables, apply patches to older versions, fix build issues (#21852)

Since there are many variables being set I thought it would be a good idea to document them better and slightly simplify the logic for external vs not-external.

3 files changed, 94 insertions, 56 deletions

diff --git a/var/spack/repos/builtin/packages/hip/0003-Improve-compilation-without-git-repo.3.9.0.patch b/var/spack/repos/builtin/packages/hip/0003-Improve-compilation-without-git-repo.3.7.0.patch
index 2bb5074df2..2bb5074df2 100644
--- a/var/spack/repos/builtin/packages/hip/0003-Improve-compilation-without-git-repo.3.9.0.patch
+++ b/var/spack/repos/builtin/packages/hip/0003-Improve-compilation-without-git-repo.3.7.0.patch
diff --git a/var/spack/repos/builtin/packages/hip/0004-Drop-clang-rt-builtins-linking-on-hip-host.3.9.0.patch b/var/spack/repos/builtin/packages/hip/0004-Drop-clang-rt-builtins-linking-on-hip-host.3.7.0.patch
index 936b333277..936b333277 100644
--- a/var/spack/repos/builtin/packages/hip/0004-Drop-clang-rt-builtins-linking-on-hip-host.3.9.0.patch
+++ b/var/spack/repos/builtin/packages/hip/0004-Drop-clang-rt-builtins-linking-on-hip-host.3.7.0.patch
diff --git a/var/spack/repos/builtin/packages/hip/package.py b/var/spack/repos/builtin/packages/hip/package.py
index ac992785dc..c0fb028d7b 100644
--- a/var/spack/repos/builtin/packages/hip/package.py
+++ b/var/spack/repos/builtin/packages/hip/package.py
@@ -37,7 +37,8 @@ class Hip(CMakePackage):
         depends_on('rocm-device-libs@' + ver, type=('build', 'link', 'run'), when='@' + ver)
         depends_on('rocminfo@' + ver, type=('build', 'run'), when='@' + ver)
 
-    # Notice: most likely this will only be a hard dependency on 3.7.0
+    # hipcc likes to add `-lnuma` by default :(
+    # ref https://github.com/ROCm-Developer-Tools/HIP/pull/2202
     depends_on('numactl', when='@3.7.0:')
 
     # Note: the ROCm ecosystem expects `lib/` and `bin/` folders with symlinks
@@ -52,74 +53,107 @@ class Hip(CMakePackage):
     patch('0002-Fix-detection-of-HIP_CLANG_ROOT.patch', when='@:3.9.0')
 
     # See https://github.com/ROCm-Developer-Tools/HIP/pull/2218
-    patch('0003-Improve-compilation-without-git-repo.3.9.0.patch', when='@3.9.0')
+    patch('0003-Improve-compilation-without-git-repo.3.7.0.patch', when='@3.7.0:3.9.0')
     patch('0003-Improve-compilation-without-git-repo.3.10.0.patch', when='@3.10.0:4.0.0')
 
     # See https://github.com/ROCm-Developer-Tools/HIP/pull/2219
-    patch('0004-Drop-clang-rt-builtins-linking-on-hip-host.3.9.0.patch', when='@3.9.0')
+    patch('0004-Drop-clang-rt-builtins-linking-on-hip-host.3.7.0.patch', when='@3.7.0:3.9.0')
     patch('0004-Drop-clang-rt-builtins-linking-on-hip-host.3.10.0.patch', when='@3.10.0:4.0.0')
 
-    def get_rocm_prefix_info(self):
-        # External packages in Spack do not currently contain dependency
-        # information. External installations of hip therefore must compute
-        # necessary paths to other rocm components by relative paths. This
-        # assumes all components are installed under a single umbrella
-        # directory. Manual edits to `fallback_path` may be necessary if this
-        # assumption does not hold.
+    def get_paths(self):
         if self.spec.external:
-            # typically, self.spec.prefix is /opt/rocm/hip, so fallback_path
-            # will be /opt/rocm. The rocminfo executable is usually
-            # found at /opt/rocm/bin/rocminfo.
-            fallback_prefix = Prefix(os.path.dirname(self.spec.prefix))
-            if not os.path.isdir(fallback_prefix):
+            # For external packages we only assume the `hip` prefix is known,
+            # because spack does not set prefixes of dependencies of externals.
+            # We assume self.spec.prefix is /opt/rocm-x.y.z/hip and rocm has a
+            # default installation with everything installed under
+            # /opt/rocm-x.y.z
+            rocm_prefix = Prefix(os.path.dirname(self.spec.prefix))
+
+            if not os.path.isdir(rocm_prefix):
                 msg = "Could not determine prefix for other rocm components\n"
                 msg += "Either report a bug at github.com/spack/spack or "
-                msg += "manually edit fallback_prefix in the package file as "
+                msg += "manually edit rocm_prefix in the package file as "
                 msg += "a workaround."
                 raise RuntimeError(msg)
 
-            return {
-                'rocm-path': fallback_prefix,
-                'llvm-amdgpu': fallback_prefix.llvm,
-                'hsa-rocr-dev': fallback_prefix.hsa,
-                'rocminfo': fallback_prefix.bin,
-                'rocm-device-libs': fallback_prefix.lib,
-                'device_lib_path': fallback_prefix.lib
+            paths = {
+                'rocm-path': rocm_prefix,
+                'llvm-amdgpu': rocm_prefix.llvm,
+                'hsa-rocr-dev': rocm_prefix.hsa,
+                'rocminfo': rocm_prefix,
+                'rocm-device-libs': rocm_prefix
             }
         else:
-            mydict = dict((name, self.spec[name].prefix)
-                          for name in ('llvm-amdgpu', 'hsa-rocr-dev',
-                                       'rocminfo', 'rocm-device-libs'))
-            mydict['rocm-path'] = self.spec.prefix
-            if '@:3.8.0' in self.spec:
-                device_lib_path = mydict['rocm-device-libs'].lib
-            else:
-                device_lib_path = mydict['rocm-device-libs'].amdgcn.bitcode
-            mydict['device_lib_path'] = device_lib_path
-            return mydict
+            paths = {
+                'rocm-path': self.spec.prefix,
+                'llvm-amdgpu': self.spec['llvm-amdgpu'].prefix,
+                'hsa-rocr-dev': self.spec['hsa-rocr-dev'].prefix,
+                'rocminfo': self.spec['rocminfo'].prefix,
+                'rocm-device-libs': self.spec['rocm-device-libs'].prefix
+            }
+
+        # `device_lib_path` is the path to the bitcode directory
+        if '@:3.8.0' in self.spec:
+            paths['device_lib_path'] = paths['rocm-device-libs'].lib
+        else:
+            paths['device_lib_path'] = paths['rocm-device-libs'].amdgcn.bitcode
+
+        return paths
 
     def set_variables(self, env):
-        # Indirection for dependency paths because hip may be an external in
-        # Spack. See block comment on get_rocm_prefix_info .
+        # Note: do not use self.spec[name] here, since not all dependencies
+        # have defined prefixes when hip is marked as external.
+        paths = self.get_paths()
 
-        # NOTE: DO NOT PUT LOGIC LIKE self.spec[name] in this function!!!!!
-        # It DOES NOT WORK FOR EXTERNAL PACKAGES!!!! See get_rocm_prefix_info
-        rocm_prefixes = self.get_rocm_prefix_info()
+        # Used in hipcc, but only useful when hip is external, since only then
+        # there is a common prefix /opt/rocm-x.y.z.
+        env.set('ROCM_PATH', paths['rocm-path'])
 
-        env.set('ROCM_PATH', rocm_prefixes['rocm-path'])
-        env.set('HIP_COMPILER', 'clang')
+        # hipcc recognizes HIP_PLATFORM == hcc and HIP_COMPILER == clang, even
+        # though below we specified HIP_PLATFORM=rocclr and HIP_COMPILER=clang
+        # in the CMake args.
         env.set('HIP_PLATFORM', 'hcc')
-        env.set('HIP_CLANG_PATH', rocm_prefixes['llvm-amdgpu'].bin)
-        env.set('HSA_PATH', rocm_prefixes['hsa-rocr-dev'])
-        env.set('ROCMINFO_PATH', rocm_prefixes['rocminfo'])
-        env.set('DEVICE_LIB_PATH', rocm_prefixes['device_lib_path'])
-        env.set('HIP_PATH', rocm_prefixes['rocm-path'])
-        # this guy is used in comgr, see the following file:
+        env.set('HIP_COMPILER', 'clang')
+
+        # bin directory where clang++ resides
+        env.set('HIP_CLANG_PATH', paths['llvm-amdgpu'].bin)
+
+        # Path to hsa-rocr-dev prefix used by hipcc.
+        env.set('HSA_PATH', paths['hsa-rocr-dev'])
+
+        # This is a variable that does not exist in hipcc but was introduced
+        # in a patch of ours since 3.5.0 to locate rocm_agent_enumerator:
+        # https://github.com/ROCm-Developer-Tools/HIP/pull/2138
+        env.set('ROCMINFO_PATH', paths['rocminfo'])
+
+        # This one is used in hipcc to run `hipcc --hip-device-lib-path=...`
+        env.set('DEVICE_LIB_PATH', paths['device_lib_path'])
+
+        # And this is used in clang whenever the --hip-device-lib-path is not
+        # used (e.g. when clang is invoked directly)
+        env.set('HIP_DEVICE_LIB_PATH', paths['device_lib_path'])
+
+        # Just the prefix of hip (used in hipcc)
+        env.set('HIP_PATH', paths['rocm-path'])
+
+        # Used in comgr and seems necessary when using the JIT compiler, e.g.
+        # hiprtcCreateProgram:
         # https://github.com/RadeonOpenCompute/ROCm-CompilerSupport/blob/rocm-4.0.0/lib/comgr/src/comgr-env.cpp
-        # it's necessary on runtime when using hiprtcCreateProgram and such
-        env.set('LLVM_PATH', rocm_prefixes['llvm-amdgpu'])
-        env.set('HIPCC_COMPILE_FLAGS_APPEND',
-                '--rocm-path={0}'.format(rocm_prefixes['device_lib_path']))
+        env.set('LLVM_PATH', paths['llvm-amdgpu'])
+
+        # Finally we have to set --rocm-path=<prefix> ourselves, which is not
+        # the same as --hip-device-lib-path (set by hipcc). It's used to set
+        # default bin, include and lib folders in clang. If it's not set it is
+        # infered from the clang install dir (and they try to find
+        # /opt/rocm again...). If this path is set, there is no strict checking
+        # and parsing of the <prefix>/bin/.hipVersion file. Let's just set this
+        # to the hip prefix directory for non-external builds so that the
+        # bin/.hipVersion file can still be parsed.
+        # See also https://github.com/ROCm-Developer-Tools/HIP/issues/2223
+        if '@3.8.0:' in self.spec:
+            env.append_path('HIPCC_COMPILE_FLAGS_APPEND',
+                            '--rocm-path={0}'.format(paths['rocm-path']),
+                            separator=' ')
 
     def setup_run_environment(self, env):
         self.set_variables(env)
@@ -175,11 +209,15 @@ class Hip(CMakePackage):
 
     def cmake_args(self):
         args = [
-            '-DHIP_COMPILER=clang',
-            '-DHIP_PLATFORM=rocclr',
-            '-DHSA_PATH={0}'.format(self.spec['hsa-rocr-dev'].prefix),
-            '-DHIP_RUNTIME=ROCclr',
-            '-DLIBROCclr_STATIC_DIR={0}/lib'.format
-            (self.spec['hip-rocclr'].prefix)
+            self.define('HIP_COMPILER', 'clang'),
+            self.define('HIP_PLATFORM', 'rocclr'),
+            self.define('HSA_PATH', self.spec['hsa-rocr-dev'].prefix),
+            self.define('HIP_RUNTIME', 'ROCclr'),
         ]
+
+        # LIBROCclr_STATIC_DIR is unused from 3.6.0 and above
+        if '@3.5.0' in self.spec:
+            args.append(self.define('LIBROCclr_STATIC_DIR',
+                        self.spec['hip-rocclr'].prefix.lib))
+
         return args