[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Decl.h:2212-2213
 
+  bool isCpuDispatchMultiVersion() const;
+  bool isCpuSpecificMultiVersion() const;
+
----------------
Pedantic nit: CPU instead of Cpu?


================
Comment at: include/clang/Basic/Attr.td:850
+def CpuSpecific : InheritableAttr {
+  let Spellings = [GCC<"cpu_specific">];
+  let Args = [VariadicIdentifierArgument<"Cpus">];
----------------
Does GCC really support this spelling? I couldn't find documentation to suggest it, and a quick test suggests it's not supported there. Perhaps this should use the Clang spelling instead?


================
Comment at: include/clang/Basic/Attr.td:851
+  let Spellings = [GCC<"cpu_specific">];
+  let Args = [VariadicIdentifierArgument<"Cpus">];
+  let Subjects = SubjectList<[Function]>;
----------------
Be sure to add a test for using this attribute with the C++ spelling, as I'm not certain how well we would parse something like `[[gnu::cpu_specific(ivybridge)]]` currently (it may just work, however).

Also, why an identifier instead of a string literal?


================
Comment at: include/clang/Basic/Attr.td:864
+def CpuDispatch : InheritableAttr {
+  let Spellings = [GCC<"cpu_dispatch">];
+  let Args = [VariadicIdentifierArgument<"Cpus">];
----------------
Same here.


================
Comment at: include/clang/Basic/AttrDocs.td:225-226
+as a parameter (like ``cpu_specific``). However, ``cpu_dispatch`` functions
+may not have a body in its definition. An empty definition is permissible for
+ICC compatibility, and all other definitions will have their body ignored.
+
----------------
How empty is empty? e.g., is it lexically empty (no tokens whatsoever) or notionally empty (empty after preprocessing)?
```
__attribute__((cpu_dispatch(atom))) void multi_cpu_1(void) {
  /* Is this empty enough? */
}

__attribute__((cpu_dispatch(atom))) void multi_cpu_2(void) {
  #if 0
    #error How about this?
  #endif
}
```


================
Comment at: lib/Parse/ParseDecl.cpp:209
 
-/// Determine whether the given attribute has an identifier argument.
+/// Determine whether the given attribute has a variadic identifier argument.
 static bool attributeHasIdentifierArg(const IdentifierInfo &II) {
----------------
This comment seems to be in the wrong place.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1877-1878
+static void handleCpuSpecificAttr(Sema &S, Decl *D, const AttributeList &AL) {
+  FunctionDecl *FD = D->getAsFunction();
+  assert(FD && "CPU Specific/Dispatch only valid on a Function Decl");
+
----------------
Better to just use `cast<FunctionDecl>(D)` here as it already asserts properly.


================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:2130
+  std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *Attr : Attrs) {
+    // Determine whether the first argument is a variadic identifier.
----------------
Please don't use `Attr` as a local name; it's a type.


https://reviews.llvm.org/D47474