Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

AMDGPUCompiler::addCompilationFlags unconditionally adds system include directories #49

Closed
awehrfritz opened this issue Aug 23, 2022 · 4 comments

Comments

@awehrfritz
Copy link

awehrfritz commented Aug 23, 2022

For installations in /usr, such as in the Fedora, Debian or Gentoo packages, ROCMIncludePath and HIPIncludePath are both set to /usr/include. Adding them to the system include directories messes up the order and leads to compilation errors, as discussed e.g. on the Debian mailing list.

If ROCMIncludePath and HIPIncludePath are set to /usr/include or some other path already in the search path of clang, they should not be added. Here the respective code:
https://github.com/RadeonOpenCompute/ROCm-CompilerSupport/blob/846cd67177bcf91dbbb941d525fff99d04364a0b/lib/comgr/src/comgr-compiler.cpp#L1017-L1020
In the Gentoo package build, they simply delete those lines to fix the issues.

Also, ClangIncludePath and ClangIncludePath2 are not set correctly for such installation:
https://github.com/RadeonOpenCompute/ROCm-CompilerSupport/blob/846cd67177bcf91dbbb941d525fff99d04364a0b/lib/comgr/src/comgr-compiler.cpp#L991-L995
Gentoo has a patch to fix this: https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-libs/rocm-comgr/files/rocm-comgr-5.1.3-Find-CLANG_RESOURCE_DIR.patch

On my Fedora 36 system, clang -print-resource-dir yields /usr/lib64/clang/14.0.0 which would be the correct ClangIncludePath. However, that is already included in the compiler command, and adding it again may mess up the include order, similar as with ROCMIncludePath and HIPIncludePath.

@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

@awehrfritz in the meantime, I've pushed a patch to the Fedora package:
https://src.fedoraproject.org/rpms/rocm-compilersupport/c/88655a00df9719a91dca13e8e0a4dc3eb78ad073?branch=rawhide

If that looks fine to you, I can cherry-pick it to Fedora 36 when I rebuild everything against LLVM 15.

littlewu2508 added a commit to littlewu2508/ROCm-CompilerSupport that referenced this issue Nov 18, 2022
By doing so, TheDriver can get the correct resource dir.

Closes: ROCm#48
Reference: ROCm#49
@lamb-j
Copy link
Collaborator

lamb-j commented Mar 31, 2023

@awehrfritz Thanks for bringing this up!

In general the way Comgr interacts with the environment for Clang/ROCm/HIP Include Paths needs an overhaul. We're working on deprecating the AMD_COMGR_ACTION_COMPILE_SOURCE_TO_FATBIN action (1a15280), and once it's removed we may be able to remove the ROCm/HIP paths entirely.

I'll try to keep this issue updated, with the hopes that at some point in the future we may not need custom patches like @Mystro256's in this context

imguoguo pushed a commit to fedora-riscv/rocm-compilersupport that referenced this issue Sep 19, 2023
Relating to this upstream bug:
ROCm/ROCm-CompilerSupport#49

- remove redundant includes
- fix libdir path
@lamb-j
Copy link
Collaborator

lamb-j commented Feb 13, 2024

This should be resolved by ROCm/llvm-project@6eb9d12

Please open a new issue at https://github.com/ROCm/llvm-project if you're still able to recreate this though!

Thanks!

@lamb-j lamb-j closed this as completed Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants