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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
