Skip to content
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

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Conversation

awehrfritz
Copy link

@awehrfritz awehrfritz commented Aug 23, 2022

This fixes various issues around the lib path in hipvars.pm and hipcc.pl:

  • hipvars.pm: Detect the correct lib directory (i.e. 'lib64' or 'lib')
  • hipcc.pl: Use the correct lib path in the linker arguments
  • hipcc.pl: Set the DEVICE_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)?

@awehrfritz
Copy link
Author

@Mystro256, I would appreciate if you could have a look at this and forward it to the right people to get this fixed.

@Mystro256
Copy link

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.*/'

@awehrfritz
Copy link
Author

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.

@Mystro256
Copy link

@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.

@cgmb
Copy link

cgmb commented Sep 10, 2022

hipcc.pl: Set the DEVICE_LIB_PATH correctly for system installation (e.g. in /usr/lib64/amdgcn/bitcode/)

It would be nice to also handle /usr/lib/x86_64-linux-gnu/amdgcn/bitcode/, which is where the files are currently placed on Debian. Would that be supported by this change?

@awehrfritz
Copy link
Author

awehrfritz commented Sep 10, 2022

@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 .hipInfo and .hipVersion are not present anymore.

@cgmb at the moment, this patch does not handle the Debian case for DEVICE_LIB_PATH. With the latest change, I only replace the hardcoded 'lib' to $LIB which corresponds lib*/ without the trailing slash. It does not account for the additional multi-arch directory (i.e. x86_64-linux-gnu) in the Debian case. However, that could be added probably fairly easily, but as I am not really familiar with the Debian filesystem layout, I wasn't even aware of this.

The $LIB directory is also used for setting the search directories for the linker (via -L or -rpath). In the Fedora case, this leads to having -L/usr/lib64 added in various places, which is as far as I can tell not needed at all since that is in the default search path already anyway. I'm at a loss with this in the Debian case. Should the linker arguments correspond then to -L/usr/lib/x86_64-linux-gnu in Debian's multi-arch setup?

bin/hipvars.pm Outdated Show resolved Hide resolved
… and account for the Debian multi-arch filesystem layout
cgmb
cgmb previously approved these changes Sep 11, 2022
Copy link

@cgmb cgmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@awehrfritz awehrfritz changed the title Fix hipcc lib dir [WIP] Fix hipcc lib dir Sep 12, 2022
@awehrfritz awehrfritz marked this pull request as draft September 12, 2022 07:40
@awehrfritz
Copy link
Author

awehrfritz commented Sep 12, 2022

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 /usr. I previously thought the issues around lib and inlcude paths were somewhat separate, but it's just one big mess there.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants