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

[Justin Lebar via cfe-commits]

jlebar added a comment.

Doesn't look like we changed any testcases when we changed this behavior?  The change in behavior may be unobservable, but even still it seems worthwhile to have tests that check that we're not doing any of the alternatives we considered.



================
Comment at: lib/Sema/SemaCUDA.cpp:99
+  if (!D->hasAttrs())
+    return false;
+  for (Attr *Attribute : D->getAttrs()) {
----------------
Is this early return necessary?


================
Comment at: lib/Sema/SemaCUDA.cpp:107
+  }
+  return false;
+}
----------------
Could we write this function as

  return llvm::any_of(D->getAttrs(), [&](Attr *Attribute) {
    return isa<A>(Attribute) && (!IgnoreImplicitAttr || !Attribute->isImplicit()) });


================
Comment at: lib/Sema/SemaCUDA.cpp:893
+
+void Sema::inheritCUDATargetAttrs(FunctionDecl *NewFD, FunctionDecl *OldFD) {
+  // Propagate CUDA target attributes from template to FD. This is
----------------
Perhaps `ToFD`, `FromFD` would be better names than "new" and "old".


================
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>()) {
----------------
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?


================
Comment at: lib/Sema/SemaCUDA.cpp:912
+    }
+  }
+}
----------------
Could this be written as

  template<typename AttrTy>
  static void copyAttrIfPresent(FunctionDecl *NewFD, FunctionDecl *OldFD) {
    if (A *Attribute = OldFD->getAttr<A>()) {
      ...
    }
  }

  void Sema::inheritCUDATargetAttrs(...) {
    copyAttrIfPresent<CUDAGlobalAttr>(NewFD, OldFD);
    ...
  }

?  In particular I am not sure why we need this particular control flow in this function.


================
Comment at: lib/Sema/SemaDecl.cpp:8372
+    // may end up with different effective targets. Instead,
+    // specializations inherit target attributes from template in
+    // CheckFunctionTemplateSpecialization() call below.
----------------
"from their templates" "in *the* foo() call below".

Although because "from their template__s__" is kind of confusing, I might rewrite to be singular:

  Instead, a specialization inherits its target attributes from its template ...


================
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 &&
----------------
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"?


================
Comment at: lib/Sema/SemaTemplate.cpp:7171
+  // must inherit in order to have the same effective target as its
+  // template.
+  if (LangOpts.CUDA)
----------------
Suggest

  A function template specialization inherits the target attributes of its template.  (We require the attributes explicitly in the code to match, but a template may have implicit attributes by virtue e.g. of being constexpr, and it passes these implicit attributes on to its specializations.)


================
Comment at: lib/Sema/SemaTemplate.cpp:7173
+  if (LangOpts.CUDA)
+    inheritCUDATargetAttrs(FD, Specialization);
+
----------------
Isn't this backwards?  The function is `to, from`?  If this is a bug, can we add a test to catch this?


https://reviews.llvm.org/D25845