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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.
Herald added a project: All.

In D120132#3352255 <https://reviews.llvm.org/D120132#3352255>, @tra wrote:

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

I created two patches for fixing the existing issues about CUDA/HIP include path

https://reviews.llvm.org/D120910 - HIP specific issue for "-c" with mixed HIP/C++ programs

https://reviews.llvm.org/D120911 - common CUDA/HIP issue for linking with mixed HIP/C++ programs

These issues are due to active offload kind of job actions not set correctly for mixed CUDA or HIP programs and C++ programs.

These issues are orthogonal to the issue which the current patch is trying to resolve.

Even with the above two issues fixed, my argument for the current patch still holds. And it is not just for mixed-language. It happens with single language too. 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.


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

https://reviews.llvm.org/D120132