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