[PATCH] D124189: [CUDA][HIP] Externalize kernels with internal linkage

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12300
+         (D->hasAttr<CUDAGlobalAttr>() &&
+          GetGVALinkageForFunction(cast<FunctionDecl>(D),
+                                   /*IgnoreCUDAGlobalAttr=*/true) ==
----------------
tra wrote:
> yaxunl wrote:
> > tra wrote:
> > > Perhaps we don't need to change the public AST API and plumb `IgnoreCUDAGlobalAttr` through.
> > > We cold create CUDA-aware static version of `GetGVALinkageForCudaKernel` instead, which would call `adjustGVALinkageForExternalDefinitionKind(..., adjustGVALinkageForAttributes(IgnoreCUDAGlobalAttr=true))`.
> > We could have a static function but it would be GetGVALinkageForCUDAKernelWithoutGlobalAttr since we need to know the linkage of the kernel assuming it has no `__global__` attribute.
> > 
> > If you think it is OK I can make the change.
> No point making public what's of no use to anybody other than this particular instance.
> 
> To think of it, we don't even need a function and could just do 
> ```
>   if (D->hasAttr<CUDAGlobalAttr>() ) {
>     bool OriginalKernelLinkage = adjustGVALinkageForExternalDefinitionKind(..., adjustGVALinkageForAttributes(IgnoreCUDAGlobalAttr=true));
>     return OriginalKernelLinkage == GVA_Internal;
>   }
>   return (IsStaticVar &&....)
> ```
> 
> 
One disadvantage of this approach is that it duplicates the code in GetGVALinkageForFunction. In the future, if GetGVALinkageForFunction changes, the same change needs to be applied to the duplicated code, which is error-prone.


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

https://reviews.llvm.org/D124189