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

[Artem Belevich via Phabricator via cfe-commits]

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


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

https://reviews.llvm.org/D120132