[PATCH] D25845: [CUDA] Ignore implicit target attributes during function template instantiation.

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: lib/Sema/SemaCUDA.cpp:99
+  if (!D->hasAttrs())
+    return false;
+  for (Attr *Attribute : D->getAttrs()) {
----------------
jlebar wrote:
> Is this early return necessary?
Yes. Otherwise D->getAttrs() will trigger assert(hasAttrs) if we don't have any attributes.


================
Comment at: lib/Sema/SemaCUDA.cpp:107
+  }
+  return false;
+}
----------------
jlebar wrote:
> Could we write this function as
> 
>   return llvm::any_of(D->getAttrs(), [&](Attr *Attribute) {
>     return isa<A>(Attribute) && (!IgnoreImplicitAttr || !Attribute->isImplicit()) });
Done.


================
Comment at: lib/Sema/SemaCUDA.cpp:896
+  // needed to ensure that FD and its template have the same
+  // effective target.
+  if (CUDAGlobalAttr *Attr = OldFD->getAttr<CUDAGlobalAttr>()) {
----------------
jlebar wrote:
> This comment is confusing because this function's implementation doesn't have anything to do with templates, but the first sentence says we're copying from a template to "FD".
> 
> In addition to cleaning this up, maybe we should move the comment into Sema.h?
I've changed second argument type to `const FunctionTemplateDecl*` to reflect my intent a bit better.
Moved the comment to Sema.h.


================
Comment at: lib/Sema/SemaTemplate.cpp:7048
       // target attributes into account, we perform target match check
       // here and reject candidates that have different target.
       if (LangOpts.CUDA &&
----------------
jlebar wrote:
> Missing some articles:
> 
>   Target attributes are part of the cuda function signature, so the deduced template's cuda target must match XXX [1].  Given that regular template deduction [2] does not take target attributes into account, we reject candidates here that have a different target.
> 
> [1] I am not sure what XXX should be.  The deduced template's cuda target must match what, exactly?
> 
> [2] What is "regular template deduction"?
[1] it must match the target of its template.
[2] "C++ template deduction"
Rephrased the comment based on  IRL conversation w/ jlebar at .


================
Comment at: lib/Sema/SemaTemplate.cpp:7173
+  if (LangOpts.CUDA)
+    inheritCUDATargetAttrs(FD, Specialization);
+
----------------
jlebar wrote:
> Isn't this backwards?  The function is `to, from`?  If this is a bug, can we add a test to catch this?
The arguments have different types now, one of them const&, so it should be unambiguous what's input and what's output. 
As for order of inputs/outputs, my mental model is `result=input` as in memcpy(dest, src). Style guide does not seem to say anything on order of input/output arguments.


https://reviews.llvm.org/D25845