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:
> > 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?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120132/new/
https://reviews.llvm.org/D120132
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
