[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

In D60967#1476069 <https://reviews.llvm.org/D60967#1476069>, @rjmccall wrote:

> In D60967#1476057 <https://reviews.llvm.org/D60967#1476057>, @scott.linder wrote:
>
> > In D60967#1476029 <https://reviews.llvm.org/D60967#1476029>, @rjmccall wrote:
> >
> > > Shouldn't it be an error if the user tries to give it hidden visibility?
> >
> >
> > We effectively consider the user explicitly specifying that a symbol is e.g. a `kernel` to also carry with it visibility information. We don't want to require the user to redundantly specify that a kernel is not hidden, when it is never meaningful for it to be hidden.
>
>
> I understand, but if the user explicitly gives it hidden visibility, you should still diagnose that.
>
> Also, shouldn't you just handle this by treating the kernel attribute as a source of explicit visibility at the Sema/AST level?


I agree that we should diagnose it, and I can update the patch accordingly, but I'm unsure how to go about emitting a diagnostic from this callback. As far as doing this at the AST level, this was my original approach in https://reviews.llvm.org/D53153, however this is really more of an AMDGPU implementation detail. I don't think it is necessarily the case that every OpenCL and Cuda implementation wants/needs require these symbols not have hidden visibility.

If we can involve the target in the AST linkage calculations, or agree that in general the `kernel` specifier should affect the visibility in this way, along with the `__device__` specifier on a variable and the `__global__` specifier on a function for Cuda, then moving this up to the AST level makes sense to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60967