-
Notifications
You must be signed in to change notification settings - Fork 527
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
[WIP] Fix hipcc lib dir #2871
base: develop
Are you sure you want to change the base?
[WIP] Fix hipcc lib dir #2871
Conversation
@Mystro256, I would appreciate if you could have a look at this and forward it to the right people to get this fixed. |
Neat idea, but something to note about hipinfo is that the Debian packagers actually remove this file in their test packages, since it violates the Linux FHS (I think) and is not strictly required. Since Fedora has this policy too, I would advise against detecting the libdir using this logic, since it would be best to use "%exclude %{_libdir}/.hipInfo" in the spec file. Alternatively the script can be updated to look for "libhsa-runtime64*" in /usr/lib and /usr/lib64, which should also work for Debian, since they use /usr/lib/x86_64-linux-gnu. I'm not sure how insane of an idea of doing this is: find /usr/lib /usr/lib64 -name libhsa-runtime64* | grep -m1 -o '^/usr/lib.*/' |
Good point about the hidden hipInfo file. Ideally we would get rid of those altogether and set those variables during the build process. By maybe that is another problem. I was toying around with a similar idea of searching for the actual libraries already earlier. If I find the time I will revise the patches to do that. Also happfor somebody else to take the lead. |
@cgmb Do you know how the Debian guys deal with this? I figure it might have come up, but I'm less versed in HIP. |
…IB_PATH correctly for system installation (e.g. in /usr/lib64/amdgcn/bitcode/)
866b3b7
to
6148151
Compare
It would be nice to also handle |
@Mystro256 I have updated this branch now to adopt your suggestion to look for the HSA runtime library. This works well in my Fedora packages where @cgmb at the moment, this patch does not handle the Debian case for The |
…rary instead of .hipInfo
fe356f4
to
453cd91
Compare
… and account for the Debian multi-arch filesystem layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
e5f3bf6
to
03fa438
Compare
Thanks for your review and comments @cgmb. I have converted this PR to a draft since I realised that there are still various entangled issues in the perl scripts for hipcc related to an installation in I have now a fully working version of all this, and was able to compile and run the HIP examples using only Fedora packages. But this required some major surgery, and will require local patches either way, as ROCM 5.2 doesn't work with upstream Clang 14 anymore and requires AMD's Clang or additional patches for hipcc. I will push my changes when I find time to clean this all up. |
This fixes various issues around the lib path in
hipvars.pm
andhipcc.pl
:hipvars.pm
: Detect the correct lib directory (i.e. 'lib64' or 'lib')hipcc.pl
: Use the correct lib path in the linker argumentshipcc.pl
: Set theDEVICE_LIB_PATH
correctly for system installation (e.g. in /usr/lib64/amdgcn/bitcode/)Note that this does not yet fix the issues around the include directories, which requires not only changes in HIP, but also ROCm-CompilerSupport.
@raramakr, this addresses some of the issues mentioned in ROCm/hipamd#43 (comment). Could you please have a look and merge those changes as you have been working on updating HIP to use GNUInstallDirs (ROCm/hipamd@c92a12f)?