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:
> > > 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.
> Good point. Looking at the code closer, it appears that what we're
> interested in is whether the kernel was internal and *became* externally
> visible due to it being a kernel.
>
> Right now we're checking if the function would normally be `GVA_Internal`
> (shouldn't we have considered GVA_DiscardableODR, too, BTW?)
> This is a somewhat indirect way of figuring out what we really need.
>
> The code that determines what we want is essentially this code in
> adjustGVALinkageForAttributes that we're trying to enable/disable with
> `ConsiderCudaGlobalAttr`.
>
> It can be easily extracted into a static function, which could then be used
> from both `adjustGVALinkageForAttributes`, (which would no longer need
> `ConsiderCudaGlobalAttr`) and from here.
>
> ```
> bool isInternalKernel(ASTContext *Context, Decl *D) {
> L=basicGVALinkageForFunction(Context, D);
> return (D->hasAttr<CUDAGlobalAttr>() &&
> (L == GVA_DiscardableODR || L == GVA_Internal));
> }
> ```
>
> This would both avoid logic duplication and would better match our intent.
>
> Does it make sense? Or did I miss something else?
GVA_DiscardableODR usually maps to linkonce_odr linkage in LLVM IR. It follows
the ODR, therefore we should not make them unique.
If we use isInternalKernel in adjustGVALinkageForAttributes, there will be two
calls of basicGVALinkageForFunction when GetGVALinkageForFunction is called,
which seems inefficient. I think we can keep GetGVALinkageForFunction as it
was, and use basicGVALinkageForFunction directly in mayExternalize.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124189/new/
https://reviews.llvm.org/D124189
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
