Pass HIP version via a commandline argument if .hipVersion is missing

[awehrfritz]

trafficstars

Clang requires the HIP version information, passed either via a commandline argument or by reading it from the .hipVersion file (see clang/lib/Driver/ToolChains/AMDGPU.cpp). Since there is no FHS compliant location for .hipVersion where clang will still be able to find it, .hipVersion has been omitted in the downstream distribution packages (Debian/Fedora) and subsequently the hip version needs to be passed explicitly.

The issue and fix have been discussed on the Debian mailing list (https://lists.debian.org/debian-ai/2022/05/msg00020.html) and the patch in Debian is here: https://salsa.debian.org/rocm-team/rocm-hipamd/-/blob/0b11747d64820050b48c63cf6424529205ea932e/debian/patches/0013-hipcc-hip-version.patch

For a detailed discussion on the various options and their implications, see also this message from @cgmb on the Debian mailing list: https://lists.debian.org/debian-ai/2022/07/msg00014.html

@cgmb and @Mystro256 could you please have a look at this and forward it to the right people?

[cgmb]

It would be nice if clang could extract the version information directly from the library binary, but it's not obvious how that should be done. This is simpler and seems reasonable, though we'll have to see what the compiler folks think.

[yxsamliu]

I can help with option 1:

patch clang [1] to look in /usr/share/hip/.hipVersion (or perhaps /usr/share/hip/version).

Also I am considering emitting HIP version predefined macro from clang instead of hipcc. Then we do not need to update hipVars.

I don't recommend patching hipcc. We would like to keep hipcc as thin as possible and move all the logics to clang since hipcc is not good at handling arguments.

[awehrfritz]

Great, thanks a lot for your reply @yxsamliu!

I can help with option 1: patch clang [1] to look in /usr/share/hip/.hipVersion (or perhaps /usr/share/hip/version).

This sounds great! From a downstream distribution perspective, the former would a nonstarter, as hidden files are not allowed in an FHS-compliant layout. But the latter option should work very well.

Also I am considering emitting HIP version predefined macro from clang instead of hipcc. Then we do not need to update hipVars.

This sounds even better.

I don't recommend patching hipcc. We would like to keep hipcc as thin as possible and move all the logics to clang since hipcc is not good at handling arguments.

Unfortunately, patching hipcc is currently the only option to package this rather entangled setup in a way that is compliant with Debian/Fedora packaging guidelines. I don't think that the patch makes the hipcc.pl script much more complicated, but we can keep these patches also in the downstream packages, especially if you will fix this upstream properly anyway. At the moment, it's just that in Debian and Fedora, we need a lot of fixing and patching to get this packaged.

[yxsamliu]

Great, thanks a lot for your reply @yxsamliu!

I can help with option 1: patch clang [1] to look in /usr/share/hip/.hipVersion (or perhaps /usr/share/hip/version).

which path is used in your current release? I can check /usr/share/hip/version only, if that is the correct path for your release.

This sounds great! From a downstream distribution perspective, the former would a nonstarter, as hidden files are not allowed in an FHS-compliant layout. But the latter option should work very well.

Also I am considering emitting HIP version predefined macro from clang instead of hipcc. Then we do not need to update hipVars.

This sounds even better.

I don't recommend patching hipcc. We would like to keep hipcc as thin as possible and move all the logics to clang since hipcc is not good at handling arguments.

Unfortunately, patching hipcc is currently the only option to package this rather entangled setup in a way that is compliant with Debian/Fedora packaging guidelines. I don't think that the patch makes the hipcc.pl script much more complicated, but we can keep these patches also in the downstream packages, especially if you will fix this upstream properly anyway. At the moment, it's just that in Debian and Fedora, we need a lot of fixing and patching to get this packaged.

I am OK with the changes in hipcc.pl in downstream branches. Once the clang change is in place you can remove them.

[awehrfritz]

Great, thanks a lot for your reply @yxsamliu!

I can help with option 1: patch clang [1] to look in /usr/share/hip/.hipVersion (or perhaps /usr/share/hip/version).

which path is used in your current release? I can check /usr/share/hip/version only, if that is the correct path for your release.

For the Fedora packages, I consider /usr/share/hip/version the best option. Given that this suggestion comes from @cgmb and the Debian package maintainer (@emollier) seems to be in favour of this location as well, I think this will work for Fedora and Debian equally well. As far as I understand this, it is also fully FHS compliant.

In my experimental packages, I exclude the .hipVersion file and use the approach in this PR instead, Debian does the same.

[cgmb]

A check for /usr/share/hip/version would be good, although there will be a bit of logic required. In order to correctly handle multiple side-by-side HIP installs, Clang needs to ensure it reads the version file corresponding to the HIP library being used.

For example, if the HIP library were installed in /usr/lib, /usr/lib64, or /usr/lib/x86_64, then the corresponding place to look for the version would be /usr/share/hip/version. However, if HIP were installed in /usr/local/lib, /usr/local/lib64, or /usr/local/lib/x86_64, clang would need to look in /usr/local/share/hip/version.

[awehrfritz]

Yes, agreed, the check in clang should account for install locations in /usr/share/hip/ and /usr/local/share/hip/.

However, if I understand the current logic for finding .hipVersion correctly, then the checks are always relative to the HIP or ROCm installation directory. If this approach is also used for finding <HIP_INSTALL_DIR>/share/hip/version then those two cases should be handled automatically since in the first case the HIP installation directory would be /usr, whereas in the latter it would be /usr/local.

Obviously, this all hinges on the HIP installation directories clang considers in the search.

Or am I missing something here?

[yxsamliu]

Yes, agreed, the check in clang should account for install locations in /usr/share/hip/ and /usr/local/share/hip/.

However, if I understand the current logic for finding .hipVersion correctly, then the checks are always relative to the HIP or ROCm installation directory. If this approach is also used for finding <HIP_INSTALL_DIR>/share/hip/version then those two cases should be handled automatically since in the first case the HIP installation directory would be /usr, whereas in the latter it would be /usr/local.

Obviously, this all hinges on the HIP installation directories clang considers in the search.

Or am I missing something here?

I think your understanding is right. clang first tries to deduce the HIP intallation path relative to its own path. If clang is in /usr/bin, it tries to look for HIP under /usr. If clang is in /usr/local/bin, it tries to look for HIP under /usr/local. Therefore if clang looks for share/hip directory for HIP version, it will automatically choose /usr/share/hip or /usr/local/share/hip based on whether clang is in /usr/bin or /usr/local/bin.

[Mystro256]

I think your understanding is right. clang first tries to deduce the HIP intallation path relative to its own path. If clang is in /usr/bin, it tries to look for HIP under /usr. If clang is in /usr/local/bin, it tries to look for HIP under /usr/local. Therefore if clang looks for share/hip directory for HIP version, it will automatically choose /usr/share/hip or /usr/local/share/hip based on whether clang is in /usr/bin or /usr/local/bin.

If a data file is required like this, from my packaging experience, generally I would recommend:

• CMAKE provides install location, generally it should embed it into the binary and rely on this first if possible. E.g. if clang was setup so CMAKE_INSTALL_PREFIX=/usr, then it should try /usr/share first inherently, since it's expected to be installed in this location. Generally CMAKE_INSTALL_FULL_DATADIR will give you the full path you need (e.g. /usr/share)
• Then, at the digression of the developers, it could fall back to trying to detect based on the location it's currently installed to (should generally expected be the same as above)

Alternatively, it could just be stored in clang resource directory directly. I.e. hip package could call "clang -print-resource-dir" to figure out where to install this version file, since I believe hip is inherently tied to the clang it's built with and it allows multiple clang+hip combinations to be installed on the same system without conflict.

It would be nice if clang could extract the version information directly from the library binary

My gut suggestion is to add a public version function to the hip library, and have hipcc link against it so it can call the function. I assume the clang probably does this with "clang --version" and libclang, but I might be wrong and it might just embed the version right in the clang binary.

[yxsamliu]

I think your understanding is right. clang first tries to deduce the HIP intallation path relative to its own path. If clang is in /usr/bin, it tries to look for HIP under /usr. If clang is in /usr/local/bin, it tries to look for HIP under /usr/local. Therefore if clang looks for share/hip directory for HIP version, it will automatically choose /usr/share/hip or /usr/local/share/hip based on whether clang is in /usr/bin or /usr/local/bin.

Yes that can be done in clang.

If a data file is required like this, from my packaging experience, generally I would recommend:

• CMAKE provides install location, generally it should embed it into the binary and rely on this first if possible. E.g. if clang was setup so CMAKE_INSTALL_PREFIX=/usr, then it should try /usr/share first inherently, since it's expected to be installed in this location. Generally CMAKE_INSTALL_FULL_DATADIR will give you the full path you need (e.g. /usr/share)
• Then, at the digression of the developers, it could fall back to trying to detect based on the location it's currently installed to (should generally expected be the same as above)

Alternatively, it could just be stored in clang resource directory directly. I.e. hip package could call "clang -print-resource-dir" to figure out where to install this version file, since I believe hip is inherently tied to the clang it's built with and it allows multiple clang+hip combinations to be installed on the same system without conflict.

LLVM/clang and HIP are separate packages. We cannot install HIP files to clang resource directory.

It would be nice if clang could extract the version information directly from the library binary

My gut suggestion is to add a public version function to the hip library, and have hipcc link against it so it can call the function. I assume the clang probably does this with "clang --version" and libclang, but I might be wrong and it might just embed the version right in the clang binary.

This approach can save the version file, however, it makes the detection complicated. Since compiling HIP programs already requests HIP header files, one more version file does not seem to add too much overhead. Another advantage of using a version file is that it is visible to the users.

I opened a Phabricator review to implement the /usr/share/hip/version approach: https://reviews.llvm.org/D135796