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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D120132#3351999 <https://reviews.llvm.org/D120132#3351999>, @yaxunl wrote:

> In D120132#3351853 <https://reviews.llvm.org/D120132#3351853>, @tra wrote:
>
>> In D120132#3351391 <https://reviews.llvm.org/D120132#3351391>, @yaxunl wrote:
>>
>>> 
>>
>>
>>
>>> If any input file is HIP program, clang driver will use HIP offload kind for all inputs. This behavior is similar as cuda-clang.
>>
>> I do not think this is the case as illustrated by the example above. CUDA paths are only added to CUDA compilation. C++ compilation of `b.cc` does not have any of them.
>
> I noticed that with "-c" HIP/CUDA include path is not used for C++ program. However without "-c" HIP/CUDA include path is used for C++ program. Probably this is a bug.

This is, indeed, a bug. cc1 invocation for a C++ file should have remained the same, yet we're passing not only include paths but also a bunch of other CUDA-related options that are not relevant for C++ compilation.

> In the current patch, AddClangSystemIncludeArgs is modified to add HIP include path. However, there is no good way to know if the current job action is HIP or C++ program.
>
> The signature of AddClangSystemIncludeArgs is
>
>   void Linux::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
>                                         ArgStringList &CC1Args) const ;
>
> In the case of two input files `a.hip` and `b.cpp`, DriverArgs contain both `a.hip` and `b.cpp`. Clang does not know if the call of AddClangSystemIncludeArgs is for `a.hip` or `b.cpp`. The current patch adds HIP include path for both HIP and C++ programs.
>
> To fix this issue, we either need to add a JobAction argument to AddClangSystemIncludeArgs to let clang know the current call of AddClangSystemIncludeArgs is for HIP program or C++ program, or we need to add HIP include path in a location where the current JobAction is known.

I think we first  need to figure out first why compilation w/o `-c` behaves incorrectly and what we can do about it.
I suspect if we fix it and make clang add options consistently regardless of whether we use `-c` or not, then we would not need to pass job info around. That is, unless the fix is to pass that info around. :-/

IMO mixed-language compilation falls into a grey area where it may sometimes work, but I doubt it's expected to work in general. I don't think it's worth complicating things over it.


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

https://reviews.llvm.org/D120132