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

[Artem Belevich via cfe-commits]

tra added inline comments.


================
Comment at: include/clang/Sema/Sema.h:9396
+  CUDAFunctionTarget IdentifyCUDATarget(const FunctionDecl *D,
+                                        bool IgnoreImplicitHDAttr = false);
   CUDAFunctionTarget IdentifyCUDATarget(const AttributeList *Attr);
----------------
jlebar wrote:
> We usually call them "target attributes", not "HD attributes"?
Implementation only ignores H and D attributes. Global is never explicit (AFAICT) and we never ignore InvalidTarget even if it's implicit. 


================
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,
----------------
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.


================
Comment at: lib/Sema/SemaTemplate.cpp:7168
+  if (LangOpts.CUDA)
+    mergeCUDATargetAttributes(FD, Specialization);
   return false;
----------------
jlebar wrote:
> I am unsure of whether or why we need this anymore, since I don't see this declared in any header, and also since I thought we weren't merging attrs anymore?
It's gone as it's not needed any more. It used to be needed when I disabled copying of target attributes from the base template. 


https://reviews.llvm.org/D25845