search

ROCm / ROCm-CompilerSupport

The compiler support repository provides various Lightning Compiler related services.
47 stars 31 forks source link

`AMDGPUCompiler::addCompilationFlags` unconditionally adds system include directories #49

Closed awehrfritz closed 4 months ago

awehrfritz commented 1 year ago

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 commented 1 year ago

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

Mystro256 commented 1 year ago

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

lamb-j commented 1 year ago

@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 (https://github.com/RadeonOpenCompute/ROCm-CompilerSupport/commit/1a15280c08c73efb9e58f59ea98d9bbfc16789ad), 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

lamb-j commented 4 months ago

This should be resolved by https://github.com/ROCm/llvm-project/commit/6eb9d123a905464a2c20a5605e5dbf599af17fde

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

Thanks!