[PATCH] D120132: [HIP] Fix HIP include path

This revision was automatically updated to reflect the committed changes. Closed by commit rG6730b44480fc: [HIP] Fix HIP include path (authored by yaxunl). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120132/new/

[PATCH] D120132: [HIP] Fix HIP include path

tra accepted this revision. tra added a comment. This revision is now accepted and ready to land. I still don't quite understand your reluctance to use `AddClangSystemIncludeArgs` for adding HIP path, but if this change works for HIP, I'm fine with it. Also, thank you for fixing the issue with

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added a comment. I found a simple fix. Use -idirafter instead of -isystem-internal. It is still system include path but will be added after all other system include paths. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120132/new/ https://reviews.llvm.org/D120132

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl updated this revision to Diff 414100. yaxunl added a comment. use -idirafter to include HIP include path CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120132/new/ https://reviews.llvm.org/D120132 Files: clang/lib/Driver/ToolChains/AMDGPU.cpp

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added a comment. Herald added a project: All. In D120132#3352255 , @tra wrote: > In D120132#3351999 , @yaxunl wrote: > >> In D120132#3351853 , @tra wrote: >> >>>

[PATCH] D120132: [HIP] Fix HIP include path

tra added a comment. In D120132#3351999 , @yaxunl wrote: > In D120132#3351853 , @tra wrote: > >> In D120132#3351391 , @yaxunl wrote: >> >>> >> >> >> >>> If any input

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added a comment. In D120132#3351853 , @tra wrote: > In D120132#3351391 , @yaxunl wrote: > >> > > > >> If any input file is HIP program, clang driver will use HIP offload kind for >> all inputs. This

[PATCH] D120132: [HIP] Fix HIP include path

tra added a comment. In D120132#3351391 , @yaxunl wrote: > > If any input file is HIP program, clang driver will use HIP offload kind for > all inputs. This behavior is similar as cuda-clang. I do not think this is the case as illustrated by the

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added a comment. In D120132#3350020 , @tra wrote: > In D120132#3349936 , @yaxunl wrote: > >> Users may use clang driver to compile HIP program and C++ program with one >> clang driver invocation, e.g. >>

[PATCH] D120132: [HIP] Fix HIP include path

tra added a comment. In D120132#3349936 , @yaxunl wrote: > Users may use clang driver to compile HIP program and C++ program with one > clang driver invocation, e.g. > > clang --offload-arch=gfx906 a.hip b.cpp > > Clang driver will create job actions

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added a comment. In D120132#3349538 , @tra wrote: > In D120132#3345534 , @yaxunl wrote: > >> I just found one issue with the current patch. It adds HIP include path for >> non-HIP programs. >> >> We

[PATCH] D120132: [HIP] Fix HIP include path

tra added a comment. In D120132#3345534 , @yaxunl wrote: > I just found one issue with the current patch. It adds HIP include path for > non-HIP programs. > > We should only add HIP include path for JobAction with HIP offloading kind. > However,

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl planned changes to this revision. yaxunl added a comment. I just found one issue with the current patch. It adds HIP include path for non-HIP programs. We should only add HIP include path for JobAction with HIP offloading kind. However, AddClangSystemIncludeArgs is not per job action.

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl updated this revision to Diff 411290. yaxunl added a comment. revised by Artem's comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120132/new/ https://reviews.llvm.org/D120132 Files: clang/lib/Driver/ToolChains/AMDGPU.cpp clang/lib/Driver/ToolChains/CrossWindows.cpp

[PATCH] D120132: [HIP] Fix HIP include path

tra added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:486 -void RocmInstallationDetector::AddHIPIncludeArgs(const ArgList , - ArgStringList ) const { +std::string

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl updated this revision to Diff 411150. yaxunl edited the summary of this revision. yaxunl added a reviewer: linjamaki. yaxunl added a comment. Herald added subscribers: mstorsjo, emaste. add HIP include path after adding other system include paths CHANGES SINCE LAST ACTION

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531-532 + DriverArgs.hasArg(options::OPT_nostdlibinc)) { +CC1Args.push_back("-internal-isystem"); +CC1Args.push_back(HipIncludePath); + } tra wrote: > yaxunl wrote: >

[PATCH] D120132: [HIP] Fix HIP include path

tra added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531-532 + DriverArgs.hasArg(options::OPT_nostdlibinc)) { +CC1Args.push_back("-internal-isystem"); +CC1Args.push_back(HipIncludePath); + } yaxunl wrote: > tra wrote: > >

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531-532 + DriverArgs.hasArg(options::OPT_nostdlibinc)) { +CC1Args.push_back("-internal-isystem"); +CC1Args.push_back(HipIncludePath); + } tra wrote: > yaxunl wrote: >

[PATCH] D120132: [HIP] Fix HIP include path

tra added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531-532 + DriverArgs.hasArg(options::OPT_nostdlibinc)) { +CC1Args.push_back("-internal-isystem"); +CC1Args.push_back(HipIncludePath); + } yaxunl wrote: > tra wrote: > >

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531-532 + DriverArgs.hasArg(options::OPT_nostdlibinc)) { +CC1Args.push_back("-internal-isystem"); +CC1Args.push_back(HipIncludePath); + } tra wrote: > My impression,

[PATCH] D120132: [HIP] Fix HIP include path

tra added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:531-532 + DriverArgs.hasArg(options::OPT_nostdlibinc)) { +CC1Args.push_back("-internal-isystem"); +CC1Args.push_back(HipIncludePath); + } My impression, after reading the

[PATCH] D120132: [HIP] Fix HIP include path

yaxunl created this revision. yaxunl added a reviewer: tra. Herald added subscribers: kerbowa, jvesely. yaxunl requested review of this revision. The clang compiler prepends the HIP header include paths to the search list using -internal-isystem when building for the HIP language. This prevents