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

[Justin Lebar via cfe-commits]

jlebar added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5628
   case AttributeList::AT_CUDAHost:
-    handleSimpleAttributeWithExclusions<CUDAHostAttr, CUDAGlobalAttr>(S, D,
-                                                                      Attr);
+    if (!D->hasAttr<CUDAHostAttr>())
+      handleSimpleAttributeWithExclusions<CUDAHostAttr, CUDAGlobalAttr>(S, D,
----------------
tra wrote:
> jlebar wrote:
> > Is this a functional change in this patch?  We don't check for duplicates of most other attributes, so I'm not sure why it should matter with these.  If it does matter, we should have comments...
> This function copies over attributes from template to instantiation, so when we already had explicit attributes we would end up with another one which has potential to mess with our attempt to ignore implicit attributes. getAttr() would return the first attribute it finds and if it happens to be implicit one, explicit one would be ignored.
> 
> Plus, it was rather weird to see duplicate attributes hanging off FunctionDecls in AST.
> This function copies over attributes from template to instantiation

It does?  I thought this just handled seeing an attribute in the source?

I am all for having a pretty AST, but

 * if the user explicitly puts two `__host__` attributes on the function, clearly we don't care about having the attribute twice on the function, and alternatively
 * if the user explicitly puts a `__host__` attribute on the function and we also somehow got an implicit attr there, this check seems wrong, since it will let the implicit one win over the explicit one?


================
Comment at: lib/Sema/SemaTemplate.cpp:7046
       if (LangOpts.CUDA &&
-          IdentifyCUDATarget(Specialization) != IdentifyCUDATarget(FD)) {
+          IdentifyCUDATarget(Specialization, true) !=
+              IdentifyCUDATarget(FD, true)) {
----------------
jlebar wrote:
> http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html  :)
> 
> At least please do
> 
>   IdentifyCUDATarget(Specialization, /* IgnoreImplicitTargetAttrs = */ true);
Please put the equals sign.  Without it, who knows whether the following call is sync or async:

  make_call(/* async */ false);

Also using the equals sign lets a clang-tidy check ensure that we're using the right name.


================
Comment at: lib/Sema/SemaTemplate.cpp:8119
     if (LangOpts.CUDA &&
-        IdentifyCUDATarget(Specialization) != IdentifyCUDATarget(Attr)) {
+        IdentifyCUDATarget(Specialization, true) != IdentifyCUDATarget(Attr)) {
       FailedCandidates.addCandidate().set(
----------------
jlebar wrote:
> Same here wrt bool param.
Same here.


https://reviews.llvm.org/D25845