New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass HIP version via a commandline argument if .hipVersion is missing #2937
base: develop
Are you sure you want to change the base?
Conversation
d62c6f0
to
f2f8f78
Compare
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. |
f2f8f78
to
becc136
Compare
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. |
Great, thanks a lot for your reply @yxsamliu!
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.
This sounds even better.
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. |
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.
I am OK with the changes in hipcc.pl in downstream branches. Once the clang change is in place you can remove them. |
For the Fedora packages, I consider In my experimental packages, I exclude the |
A check for For example, if the HIP library were installed in |
Yes, agreed, the check in clang should account for install locations in However, if I understand the current logic for finding 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 |
If a data file is required like this, from my packaging experience, generally I would recommend:
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.
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. |
Yes that can be done in clang.
LLVM/clang and HIP are separate packages. We cannot install HIP files to clang resource directory.
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 |
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?