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

[Yaxun Liu via Phabricator via cfe-commits]

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:
> > tra wrote:
> > > yaxunl wrote:
> > > > tra wrote:
> > > > > My impression, after reading the problem description, is that the actual issue is that we're using `-internal-isystem` for the HIP SDK includes , not that we add the include path to them.
> > > > > 
> > > > > Instead of trying to guess whether we happen to match some hardcoded path where we think the standard headers may live, I'd rather use `-I` or its internal equivalent, if we have it. Hardcoded paths *will* be wrong for someone. E.g. I'm pretty sure `/usr/anything` is not going to work on windows. Of for our internal builds.
> > > > changing `-internal-isystem` to `-I` is a solution, as the same path showing up first with both `-I` then with `-internal-isystem` will be treated as `-internal-isystem` and placed in the latter position.
> > > > 
> > > > However, one drawback is that this may cause regression due to warnings emitted for HIP headers.
> > > > 
> > > > I think I may let AddHIPIncludeArgs return the HIP include path instead of adding it right away, then let clang add it after the system include paths. I may rename AddHIPIncludeArgs as AddHIPWrapperIncludeArgsAndGetHIPIncludePath. What do you think? Thanks. 
> > > > one drawback is that this may cause regression due to warnings emitted for HIP headers.
> > > 
> > > If I understand the patch description correctly, allowing these warnings was the purpose. Is that not the case?
> > > 
> > > > [current state] prevents warnings related to things like reserved identifiers when including the HIP headers even when ROCm is installed in a non-system directory, such as /opt/rocm.
> > > 
> > > I'm fine with separating include paths of wrappers and the SDK headers. I think we already do something similar for CUDA (though they are still added with -isystem-include).
> > Some warnings are unnecessary for HIP header files e.g. warnings about macro definitions starting with `__`. Some applications use -Wpedantic -Werror, which can cause unnecessary errors in HIP header. Also we have customers who use the latest clang with older HIP runtime. If we switch to `-I`, we may get regressions.
> > 
> > AFAIK all language headers e.g. CUDA, OpenMP, are included by `-internal-isystem` by clang, therefore I think HIP better follow this convention.
> I may have misinterpreted the patch description. 
> 
> So `-include-isystem` is not the problem. The problem is that when HIP SDK includes are installed under /usr, their inclusion along with the wrappers messes with the standard c/c++ header inclusion order . Separating the wrappers and HIP SDK include paths, and moving HIP includes towards the end of the search path is the way to fix it. HIP SDK headers will still be still included via `-isystem-include`.
> 
> Did I get that right?
> 
Right.

Since HIP headers does not use include_next, it does not matter whether HIP include path is before or after standard C++ include path. However, HIP include path may be the same as /usr or /usr/local, which need to be after standard C++ include path. Therefore moving HIP include path to be after system path solves the issue.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120132/new/

https://reviews.llvm.org/D120132